diff mbox series

[15/16] Revert "drm/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()"

Message ID 20211002154542.15800-15-sean@poorly.run (mailing list archive)
State New, archived
Headers show
Series [01/16] Revert "drm: cleanup: remove drm_modeset_(un)lock_all()" | expand

Commit Message

Sean Paul Oct. 2, 2021, 3:45 p.m. UTC 
From: Sean Paul <seanpaul@chromium.org>

This reverts commit 399190e70816886e2bca1f3f3bc3d9c544af88e7.

This patchset breaks on intel platforms and was previously NACK'd by
Ville.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Fernando Ramos <greenfoo@u92.eu>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/i915/display/intel_display.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Ville Syrjälä Oct. 4, 2021, 9:41 a.m. UTC | #1 
On Sat, Oct 02, 2021 at 11:45:41AM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> This reverts commit 399190e70816886e2bca1f3f3bc3d9c544af88e7.
> 
> This patchset breaks on intel platforms and was previously NACK'd by
> Ville.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Fernando Ramos <greenfoo@u92.eu>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Yeah, best to try again from the start I think.

For the series
Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 2bf01416d656..134a6acbd8fb 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -43,7 +43,6 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_rect.h>
> -#include <drm/drm_drv.h>
>  
>  #include "display/intel_audio.h"
>  #include "display/intel_crt.h"
> @@ -13477,13 +13476,22 @@ void intel_display_resume(struct drm_device *dev)
>  	if (state)
>  		state->acquire_ctx = &ctx;
>  
> -	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> +	drm_modeset_acquire_init(&ctx, 0);
>  
> -	ret = __intel_display_resume(dev, state, &ctx);
> +	while (1) {
> +		ret = drm_modeset_lock_all_ctx(dev, &ctx);
> +		if (ret != -EDEADLK)
> +			break;
>  
> -	intel_enable_ipc(dev_priv);
> +		drm_modeset_backoff(&ctx);
> +	}
> +
> +	if (!ret)
> +		ret = __intel_display_resume(dev, state, &ctx);
>  
> -	DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> +	intel_enable_ipc(dev_priv);
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
>  
>  	if (ret)
>  		drm_err(&dev_priv->drm,
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
Sean Paul Oct. 4, 2021, 3:56 p.m. UTC | #2 
On Mon, Oct 04, 2021 at 12:41:00PM +0300, Ville Syrjälä wrote:
> On Sat, Oct 02, 2021 at 11:45:41AM -0400, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > This reverts commit 399190e70816886e2bca1f3f3bc3d9c544af88e7.
> > 
> > This patchset breaks on intel platforms and was previously NACK'd by
> > Ville.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Fernando Ramos <greenfoo@u92.eu>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> 
> Yeah, best to try again from the start I think.

Pushed the revert set (and left the TODO item out for now). Thanks for raising
the issue.

@Fernando, hopefully you can revise and post again. Thank you for your patches
and your effort!

Sean

> 
> For the series
> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 2bf01416d656..134a6acbd8fb 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -43,7 +43,6 @@
> >  #include <drm/drm_plane_helper.h>
> >  #include <drm/drm_probe_helper.h>
> >  #include <drm/drm_rect.h>
> > -#include <drm/drm_drv.h>
> >  
> >  #include "display/intel_audio.h"
> >  #include "display/intel_crt.h"
> > @@ -13477,13 +13476,22 @@ void intel_display_resume(struct drm_device *dev)
> >  	if (state)
> >  		state->acquire_ctx = &ctx;
> >  
> > -	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> > +	drm_modeset_acquire_init(&ctx, 0);
> >  
> > -	ret = __intel_display_resume(dev, state, &ctx);
> > +	while (1) {
> > +		ret = drm_modeset_lock_all_ctx(dev, &ctx);
> > +		if (ret != -EDEADLK)
> > +			break;
> >  
> > -	intel_enable_ipc(dev_priv);
> > +		drm_modeset_backoff(&ctx);
> > +	}
> > +
> > +	if (!ret)
> > +		ret = __intel_display_resume(dev, state, &ctx);
> >  
> > -	DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> > +	intel_enable_ipc(dev_priv);
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> >  
> >  	if (ret)
> >  		drm_err(&dev_priv->drm,
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> 
> -- 
> Ville Syrjälä
> Intel
Fernando Ramos Oct. 4, 2021, 8:31 p.m. UTC | #3 
On 21/10/04 11:56AM, Sean Paul wrote:
> @Fernando, hopefully you can revise and post again. Thank you for your patches
> and your effort!

No problem :)

Just to be sure I do the right thing this time (and to better understand the
process), please confirm that this is the correct sequence of events:

  1. I fix the lock issue and test on my local machine.

  2. I then post this new patch set (v3) rebased on top of drm-tip (instead of
     drm-next). This will automatically trigger tests on intel hardware (and
     maybe in other hardwares?)

        NOTE: I originally chose drm-next because that's what is mentioned here:
        https://01.org/linuxgraphics/gfx-docs/drm/gpu/introduction.html#contribution-process
        Maybe this doc should be updated?

  3. Once reviewed and approved, someone (Sean?) merges them into "somewhere"
     (drm-next? drm-misc-next? drm-intel-next? How is this decided?).

  4. Eventually, that other branch from the previous point is merged into
     drm-tip.

  5. ??

  6. The branch is merged into linux-next.


There must be something wrong in my description above, as it doesn't make sense
to post the patch series based on "drm-tip" only to later have one of the
mainteiners merge them into a different branch that will eventually be merged
back into "drm-tip".

Sorry for being completely lost! Is there a document explaining how all of this
works so that I can learn for the next time?

Thanks!
Daniel Vetter Oct. 13, 2021, 11:49 a.m. UTC | #4 
On Mon, Oct 04, 2021 at 10:31:04PM +0200, Fernando Ramos wrote:
> On 21/10/04 11:56AM, Sean Paul wrote:
> > @Fernando, hopefully you can revise and post again. Thank you for your patches
> > and your effort!
> 
> No problem :)
> 
> Just to be sure I do the right thing this time (and to better understand the
> process), please confirm that this is the correct sequence of events:
> 
>   1. I fix the lock issue and test on my local machine.
> 
>   2. I then post this new patch set (v3) rebased on top of drm-tip (instead of
>      drm-next). This will automatically trigger tests on intel hardware (and
>      maybe in other hardwares?)
> 
>         NOTE: I originally chose drm-next because that's what is mentioned here:
>         https://01.org/linuxgraphics/gfx-docs/drm/gpu/introduction.html#contribution-process
>         Maybe this doc should be updated?
> 
>   3. Once reviewed and approved, someone (Sean?) merges them into "somewhere"
>      (drm-next? drm-misc-next? drm-intel-next? How is this decided?).
> 
>   4. Eventually, that other branch from the previous point is merged into
>      drm-tip.
> 
>   5. ??
> 
>   6. The branch is merged into linux-next.

This part should happen automatically, plus/minus right around the merge
window. At least not your problem.

Otherwise don't worry, and don't sweat it too much. We know that our CI
situation just isn't great yet for drm contributors :-/ There's plans to
improve it though, but it all takes time.

> There must be something wrong in my description above, as it doesn't make sense
> to post the patch series based on "drm-tip" only to later have one of the
> mainteiners merge them into a different branch that will eventually be merged
> back into "drm-tip".
> 
> Sorry for being completely lost! Is there a document explaining how all of this
> works so that I can learn for the next time?

drm-tip is just linux-next for drm area, it's the same principle. If there
are conflicts while merging, maintainers will sort these out. And yeah
that's a bit a speciality of linux with the multi-branch model for
development.
-Daniel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 2bf01416d656..134a6acbd8fb 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -43,7 +43,6 @@ 
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_rect.h>
-#include <drm/drm_drv.h>
 
 #include "display/intel_audio.h"
 #include "display/intel_crt.h"
@@ -13477,13 +13476,22 @@  void intel_display_resume(struct drm_device *dev)
 	if (state)
 		state->acquire_ctx = &ctx;
 
-	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
+	drm_modeset_acquire_init(&ctx, 0);
 
-	ret = __intel_display_resume(dev, state, &ctx);
+	while (1) {
+		ret = drm_modeset_lock_all_ctx(dev, &ctx);
+		if (ret != -EDEADLK)
+			break;
 
-	intel_enable_ipc(dev_priv);
+		drm_modeset_backoff(&ctx);
+	}
+
+	if (!ret)
+		ret = __intel_display_resume(dev, state, &ctx);
 
-	DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
+	intel_enable_ipc(dev_priv);
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
 
 	if (ret)
 		drm_err(&dev_priv->drm,