[PATCH] Revert "drm/msm: dpu: Add modeset lock checks where applicable"

[PATCH] Revert "drm/msm: dpu: Add modeset lock checks where applicable"

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

This reverts commit 1dfdb0e107dbe6ebff3f6bbbe4aad0b5aa87bba4.

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. I'm not entirely sure if these have always been
isolated to the commit path, but they seem to be now.

[1]- https://lists.freedesktop.org/archives/dri-devel/2019-October/239441.html

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>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index db6c9ccf3be26..c645dd201368b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -282,8 +282,6 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
 		return INTF_MODE_NONE;
 	}
 
-	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
-
 	/* TODO: Returns the first INTF_MODE, could there be multiple values? */
 	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
 		return dpu_encoder_get_intf_mode(encoder);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e393a423d7d7a..0e68e20d19c87 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -305,7 +305,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

Re: [PATCH] Revert "drm/msm: dpu: Add modeset lock checks where applicable"

[Daniel Vetter]

On Thu, Oct 10, 2019 at 5:13 PM Sean Paul <sean@poorly.run> wrote:
>
> From: Sean Paul <seanpaul@chromium.org>
>
> This reverts commit 1dfdb0e107dbe6ebff3f6bbbe4aad0b5aa87bba4.
>
> 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. I'm not entirely sure if these have always been
> isolated to the commit path, but they seem to be now.
>
> [1]- https://lists.freedesktop.org/archives/dri-devel/2019-October/239441.html
>
> 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>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 --
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 1 -
>  2 files changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index db6c9ccf3be26..c645dd201368b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -282,8 +282,6 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>                 return INTF_MODE_NONE;
>         }
>
> -       WARN_ON(!drm_modeset_is_locked(&crtc->mutex));

This one is worse ... it's used in two places:
- debugfs, where you actually want to make sure you're holding this lock
- atomic_check, where this is broken since you're supposed to look at
the free-standing states only, except if you really know what you're
doing. Given that there's no comment here, I suspect that's not the
case. Note that for atomic_check you're guaranteed to hold the modeset
lock.

I'd put a FIXME here, but leave the WARN_ON until this is fixed properly.
> -
>         /* TODO: Returns the first INTF_MODE, could there be multiple values? */
>         drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
>                 return dpu_encoder_get_intf_mode(encoder);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index e393a423d7d7a..0e68e20d19c87 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -305,7 +305,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));

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

but only for this hunk here.
-Daniel

>         drm_for_each_crtc(crtc, dev) {
>                 if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder)))
>                         continue;
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Sean Paul]

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>
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

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

[Sean Paul]

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