Re: [PATCH v2] drm/msm: Sanitize the modeset_is_locked checks in dpu

[Sean Paul <sean@poorly.run>]

On Thu, Oct 10, 2019 at 02:17:44PM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> As Daniel mentions in his email [1], non-blocking commits don't hold the
> modeset locks, so we can safely access state as long as these functions
> are in the commit path. So remove the WARN_ON in dpu_kms_encoder_enable.
> 
> In dpu_crtc_get_intf_mode, things are a bit more complicated. So keep
> the WARN_ON, but add a comment explaining the situation and hope someone
> comes along and fixes the issue.
> 
> [1]- https://lists.freedesktop.org/archives/dri-devel/2019-October/239441.html
> 
> Link to v1: https://patchwork.freedesktop.org/patch/msgid/20191010151351.126735-1-sean@poorly.run
> 
> Changes in v2:
> - Restored the WARN_ON in get_intf_mode and added a clarifying comment (Daniel)
> 
> Fixes: 1dfdb0e107db ("drm/msm: dpu: Add modeset lock checks where applicable")
> Cc: Jeykumar Sankaran <jsanka@codeaurora.org>
> Cc: Rob Clark <robdclark@chromium.org>
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Partially-Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Applied to msm-next with danvet's full irc r-b, thanks for the report and
review!

Sean

> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 1 -
>  2 files changed, 9 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 0b9dc042d2e22..f197dce545761 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -271,6 +271,15 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>  		return INTF_MODE_NONE;
>  	}
>  
> +	/*
> +	 * TODO: This function is called from dpu debugfs and as part of atomic
> +	 * check. When called from debugfs, the crtc->mutex must be held to
> +	 * read crtc->state. However reading crtc->state from atomic check isn't
> +	 * allowed (unless you have a good reason, a big comment, and a deep
> +	 * understanding of how the atomic/modeset locks work (<- and this is
> +	 * probably not possible)). So we'll keep the WARN_ON here for now, but
> +	 * really we need to figure out a better way to track our operating mode
> +	 */
>  	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
>  
>  	/* TODO: Returns the first INTF_MODE, could there be multiple values? */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index b1645ad83a1e1..6c92f0fbeac98 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -316,7 +316,6 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder)
>  	if (funcs && funcs->commit)
>  		funcs->commit(encoder);
>  
> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  	drm_for_each_crtc(crtc, dev) {
>  		if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder)))
>  			continue;
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS