linux-kernel - Re: [PATCH 20/21] drm/msm/dpu: Reorder encoder kickoff for CWB

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: <ghzsvz37j7tgj2c6kkixpnyo2ezc33vd53zrgynet5krk75ut4@lcxhpuruv6ut>
Date: Fri, 30 Aug 2024 20:35:38 +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 20/21] drm/msm/dpu: Reorder encoder kickoff for CWB

On Thu, Aug 29, 2024 at 01:48:41PM GMT, Jessica Zhang wrote:
> Add a helper that will handle the correct order of the encoder kickoffs
> for concurrent writeback.
> 
> For concurrent writeback, the realtime encoder must always kickoff last
> as it will call the trigger flush and start.
> 
> This avoids the following scenario where the writeback encoder
> increments the pending kickoff count after the WB_DONE interrupt is
> fired:
> 
> If the realtime encoder is kicked off first, the encoder kickoff will
> flush/start the encoder and increment the pending kickoff count. The
> WB_DONE interrupt then fires (before the writeback encoder is kicked
> off). When the writeback encoder enters its kickoff, it will skip the
> flush/start (due to CWB being enabled) and hit a frame done timeout
> as the frame was kicked off (and the WB_DONE interrupt fired) without
> the pending kickoff count being incremented.
> 
> In addition, the writeback timer should only start after the realtime
> encoder is kicked off to ensure that we don't get timeouts when the
> system has a heavy load (ex. when debug logs are enabled)
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 67 ++++++++++++++++++++++++++------
>  1 file changed, 55 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index daf5f751f584..d2f91e89eba7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -928,6 +928,42 @@ static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc)
>  	return rc;
>  }
>  
> +static int dpu_crtc_kickoff_clone_mode(struct drm_crtc *crtc)
> +{
> +	struct drm_encoder *encoder;
> +	struct drm_encoder *rt_encoder = NULL, *wb_encoder;
> +
> +	/* Find encoder for real time display */
> +	drm_for_each_encoder_mask(encoder, crtc->dev,
> +				  crtc->state->encoder_mask) {
> +		if (encoder->encoder_type == DRM_MODE_ENCODER_VIRTUAL)
> +			wb_encoder = encoder;
> +		else
> +			rt_encoder = encoder;
> +	}
> +
> +	if (!rt_encoder || !wb_encoder) {
> +		DRM_DEBUG_ATOMIC("real time or wb encoder not found\n");
> +		return -EINVAL;
> +	}
> +
> +	dpu_encoder_prepare_for_kickoff(wb_encoder);
> +	dpu_encoder_prepare_for_kickoff(rt_encoder);
> +
> +	/*
> +	 * Kickoff real time encoder last as it's the encoder that
> +	 * will do the flush
> +	 */
> +	dpu_encoder_kickoff(wb_encoder);
> +	dpu_encoder_kickoff(rt_encoder);
> +
> +	/* Don't start frame done timers until the kickoffs have finished */
> +	dpu_encoder_start_frame_done_timer(wb_encoder);
> +	dpu_encoder_start_frame_done_timer(rt_encoder);
> +
> +	return 0;
> +}
> +
>  void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>  {
>  	struct drm_encoder *encoder;
> @@ -952,13 +988,25 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>  			goto end;
>  		}
>  	}
> -	/*
> -	 * Encoder will flush/start now, unless it has a tx pending. If so, it
> -	 * may delay and flush at an irq event (e.g. ppdone)
> -	 */
> -	drm_for_each_encoder_mask(encoder, crtc->dev,
> -				  crtc->state->encoder_mask)
> -		dpu_encoder_prepare_for_kickoff(encoder);
> +
> +	if (drm_crtc_in_clone_mode(crtc->state)) {
> +		if (dpu_crtc_kickoff_clone_mode(crtc))
> +			goto end;
> +	} else {
> +		/*
> +		 * Encoder will flush/start now, unless it has a tx pending.
> +		 * If so, it may delay and flush at an irq event (e.g. ppdone)
> +		 */
> +		drm_for_each_encoder_mask(encoder, crtc->dev,
> +				crtc->state->encoder_mask)
> +			dpu_encoder_prepare_for_kickoff(encoder);
> +
> +		drm_for_each_encoder_mask(encoder, crtc->dev,
> +				crtc->state->encoder_mask) {
> +			dpu_encoder_kickoff(encoder);
> +			dpu_encoder_start_frame_done_timer(encoder);
> +		}
> +	}
>  
>  	if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
>  		/* acquire bandwidth and other resources */
> @@ -970,11 +1018,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>  
>  	dpu_vbif_clear_errors(dpu_kms);
>  
> -	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) {
> -		dpu_encoder_kickoff(encoder);
> -		dpu_encoder_start_frame_done_timer(encoder);
> -	}
> -

I think I understand why you wanted to move the kickoff before the
frame_pending, etc. However at the same time dpu_vbif_clear_errors()
should be called before kickoff.

>  	reinit_completion(&dpu_crtc->frame_done_comp);
>  
>  end:
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry