linux-kernel - Re: [PATCH 07/21] drm/msm/dpu: Check CRTC encoders are valid clones

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <uqtlpynjdszqyyikj64uxwuqnk3lmzma7kd2vwxipnj4fg2eje@7toj5kww7vk7>
Date: Fri, 30 Aug 2024 20:00:56 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jessica Zhang <quic_jesszhan@...cinc.com>
Cc: Rob Clark <robdclark@...il.com>, quic_abhinavk@...cinc.com, 
	Sean Paul <sean@...rly.run>, Marijn Suijten <marijn.suijten@...ainline.org>, 
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, quic_ebharadw@...cinc.com, linux-arm-msm@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	Rob Clark <robdclark@...omium.org>
Subject: Re: [PATCH 07/21] drm/msm/dpu: Check CRTC encoders are valid clones

On Thu, Aug 29, 2024 at 01:48:28PM GMT, Jessica Zhang wrote:
> Check that each encoder in the CRTC state's encoder_mask is marked as a
> possible clone for all other encoders in the encoder_mask and that only
> one CRTC is in clone mode at a time
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 36 +++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 5ec1b5a38922..bebae365c036 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>   * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
>   * Copyright (C) 2013 Red Hat
>   * Author: Rob Clark <robdclark@...il.com>
> @@ -1204,6 +1204,36 @@ static struct msm_display_topology dpu_crtc_get_topology(
>  	return topology;
>  }
>  
> +static bool dpu_crtc_has_valid_clones(struct drm_crtc *crtc,
> +		struct drm_crtc_state *crtc_state)
> +{
> +	struct drm_encoder *drm_enc;
> +	struct drm_crtc *temp_crtc;
> +	int num_cwb_sessions = 0;
> +
> +	drm_for_each_crtc(temp_crtc, crtc->dev)
> +		if (drm_crtc_in_clone_mode(temp_crtc->state))

No, get the state from drm_atomic_state. temp_crtc->state might be
irrelevant.

> +			num_cwb_sessions++;

Even simpler:
if (temp_crtc != crtc && drm_crtc_in_clone_mode(...))
	return false;

> +
> +	/*
> +	 * Only support a single concurrent writeback session running
> +	 * at a time

If it is not a hardware limitation, please add:
FIXME: support more than one session

> +	 */
> +	if (num_cwb_sessions > 1)
> +		return false;
> +
> +	drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc_state->encoder_mask) {
> +		if ((crtc_state->encoder_mask & drm_enc->possible_clones) !=
> +				crtc_state->encoder_mask) {

Align to opening bracket, please. Granted that other drivers don't
perform this check, is it really necessary? Doesn't
validate_encoder_possible_clones() ensure the same, but during the
encoder registration?

> +			DPU_ERROR("crtc%d failed valid clone check for mask 0x%x\n",

DPU_DEBUG, don't let users spam dmesg.

> +				crtc->base.id, crtc_state->encoder_mask);
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  static int dpu_crtc_assign_resources(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state)
>  {
>  	struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_CRTC];
> @@ -1287,6 +1317,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>  			return rc;
>  	}
>  
> +	if (drm_crtc_in_clone_mode(crtc_state) &&
> +			!dpu_crtc_has_valid_clones(crtc, crtc_state))

Align to opening bracket.

> +		return -EINVAL;
> +
>  	if (!crtc_state->enable || !drm_atomic_crtc_effectively_active(crtc_state)) {
>  		DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip atomic_check\n",
>  				crtc->base.id, crtc_state->enable,
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry