[PATCH] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock

[PATCH] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock

[Fabio M. De Francesco]

drm_modeset_lock_all() is not needed here, so it is replaced with
drm_modeset_lock(). The crtc list around which we are looping never
changes, therefore the only lock we need is to protect access to
crtc->state.

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 671ec1002230..bce8f6793d8e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1439,17 +1439,16 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
 	if (amdgpu_device_has_dc_support(adev)) {
 		struct drm_crtc *crtc;
 
-		drm_modeset_lock_all(drm_dev);
-
 		drm_for_each_crtc(crtc, drm_dev) {
+			drm_modeset_lock(&crtc->mutex, NULL);
 			if (crtc->state->active) {
 				ret = -EBUSY;
-				break;
 			}
+			drm_modeset_unlock(&crtc->mutex);
+			if (ret < 0)
+				break;
 		}
 
-		drm_modeset_unlock_all(drm_dev);
-
 	} else {
 		struct drm_connector *list_connector;
 		struct drm_connector_list_iter iter;
-- 
2.31.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock

[Fabio M. De Francesco]

drm_modeset_lock_all() is not needed here, so it is replaced with
drm_modeset_lock(). The crtc list around which we are looping never
changes, therefore the only lock we need is to protect access to
crtc->state.

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 671ec1002230..bce8f6793d8e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1439,17 +1439,16 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
 	if (amdgpu_device_has_dc_support(adev)) {
 		struct drm_crtc *crtc;
 
-		drm_modeset_lock_all(drm_dev);
-
 		drm_for_each_crtc(crtc, drm_dev) {
+			drm_modeset_lock(&crtc->mutex, NULL);
 			if (crtc->state->active) {
 				ret = -EBUSY;
-				break;
 			}
+			drm_modeset_unlock(&crtc->mutex);
+			if (ret < 0)
+				break;
 		}
 
-		drm_modeset_unlock_all(drm_dev);
-
 	} else {
 		struct drm_connector *list_connector;
 		struct drm_connector_list_iter iter;
-- 
2.31.1

Re: [PATCH] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock

[Matthew Wilcox]

On Thu, Apr 22, 2021 at 05:59:51PM +0200, Fabio M. De Francesco wrote:
> -		drm_modeset_lock_all(drm_dev);
> -
>  		drm_for_each_crtc(crtc, drm_dev) {
> +			drm_modeset_lock(&crtc->mutex, NULL);
>  			if (crtc->state->active) {
>  				ret = -EBUSY;
> -				break;
>  			}
> +			drm_modeset_unlock(&crtc->mutex);
> +			if (ret < 0)
> +				break;
>  		}
>  
> -		drm_modeset_unlock_all(drm_dev);
> -

I might remove the {} around ret = -EBUSY, but this is good.

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock

[Matthew Wilcox]

On Thu, Apr 22, 2021 at 05:59:51PM +0200, Fabio M. De Francesco wrote:
> -		drm_modeset_lock_all(drm_dev);
> -
>  		drm_for_each_crtc(crtc, drm_dev) {
> +			drm_modeset_lock(&crtc->mutex, NULL);
>  			if (crtc->state->active) {
>  				ret = -EBUSY;
> -				break;
>  			}
> +			drm_modeset_unlock(&crtc->mutex);
> +			if (ret < 0)
> +				break;
>  		}
>  
> -		drm_modeset_unlock_all(drm_dev);
> -

I might remove the {} around ret = -EBUSY, but this is good.

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Re: [Outreachy kernel] Re: [PATCH] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock

[Daniel Vetter]

On Thu, Apr 22, 2021 at 05:50:34PM +0100, Matthew Wilcox wrote:
> On Thu, Apr 22, 2021 at 05:59:51PM +0200, Fabio M. De Francesco wrote:
> > -		drm_modeset_lock_all(drm_dev);
> > -
> >  		drm_for_each_crtc(crtc, drm_dev) {
> > +			drm_modeset_lock(&crtc->mutex, NULL);
> >  			if (crtc->state->active) {
> >  				ret = -EBUSY;
> > -				break;
> >  			}
> > +			drm_modeset_unlock(&crtc->mutex);
> > +			if (ret < 0)
> > +				break;
> >  		}
> >  
> > -		drm_modeset_unlock_all(drm_dev);
> > -
> 
> I might remove the {} around ret = -EBUSY, but this is good.
> 
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Yup patch looks good, but it's not cc'ed to drm/amdgpu maintainers/m-l, so
likely won't get picked up. Can you pls check scripts/get_maintainers for
anything you've missed, add those and resend with Willy's r-b tag
included?

Then Alex can pick it up for merging.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] Re: [PATCH] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock

[Daniel Vetter]

On Thu, Apr 22, 2021 at 05:50:34PM +0100, Matthew Wilcox wrote:
> On Thu, Apr 22, 2021 at 05:59:51PM +0200, Fabio M. De Francesco wrote:
> > -		drm_modeset_lock_all(drm_dev);
> > -
> >  		drm_for_each_crtc(crtc, drm_dev) {
> > +			drm_modeset_lock(&crtc->mutex, NULL);
> >  			if (crtc->state->active) {
> >  				ret = -EBUSY;
> > -				break;
> >  			}
> > +			drm_modeset_unlock(&crtc->mutex);
> > +			if (ret < 0)
> > +				break;
> >  		}
> >  
> > -		drm_modeset_unlock_all(drm_dev);
> > -
> 
> I might remove the {} around ret = -EBUSY, but this is good.
> 
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Yup patch looks good, but it's not cc'ed to drm/amdgpu maintainers/m-l, so
likely won't get picked up. Can you pls check scripts/get_maintainers for
anything you've missed, add those and resend with Willy's r-b tag
included?

Then Alex can pick it up for merging.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Outreachy kernel] Re: [PATCH] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock

[Fabio M. De Francesco]

On Monday, April 26, 2021 6:11:11 PM CEST Daniel Vetter wrote:
> On Thu, Apr 22, 2021 at 05:50:34PM +0100, Matthew Wilcox wrote:
> > On Thu, Apr 22, 2021 at 05:59:51PM +0200, Fabio M. De Francesco wrote:
> > > -		drm_modeset_lock_all(drm_dev);
> > > -
> > > 
> > >  		drm_for_each_crtc(crtc, drm_dev) {
> > > 
> > > +			drm_modeset_lock(&crtc->mutex, NULL);
> > > 
> > >  			if (crtc->state->active) {
> > >  			
> > >  				ret = -EBUSY;
> > > 
> > > -				break;
> > > 
> > >  			}
> > > 
> > > +			drm_modeset_unlock(&crtc->mutex);
> > > +			if (ret < 0)
> > > +				break;
> > > 
> > >  		}
> > > 
> > > -		drm_modeset_unlock_all(drm_dev);
> > > -
> > 
> > I might remove the {} around ret = -EBUSY, but this is good.
> > 
> > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> Yup patch looks good, but it's not cc'ed to drm/amdgpu maintainers/m-l, so
> likely won't get picked up. Can you pls check scripts/get_maintainers for
> anything you've missed, add those and resend with Willy's r-b tag
> included?
> 
> Then Alex can pick it up for merging.
> 
> Thanks, Daniel
>
I had already submitted a v2 of this patch with an added 'Review-by' Matthew 
Wilcox under my name. It removed the unnecessary braces that willy pointed 
out. However I see that not all maintainers had been cc'ed, so I'm going to  
resend it as v3.

Thanks,

Fabio





_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Outreachy kernel] Re: [PATCH] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all with drm_modeset_lock

[Fabio M. De Francesco]

On Monday, April 26, 2021 6:11:11 PM CEST Daniel Vetter wrote:
> On Thu, Apr 22, 2021 at 05:50:34PM +0100, Matthew Wilcox wrote:
> > On Thu, Apr 22, 2021 at 05:59:51PM +0200, Fabio M. De Francesco wrote:
> > > -		drm_modeset_lock_all(drm_dev);
> > > -
> > > 
> > >  		drm_for_each_crtc(crtc, drm_dev) {
> > > 
> > > +			drm_modeset_lock(&crtc->mutex, NULL);
> > > 
> > >  			if (crtc->state->active) {
> > >  			
> > >  				ret = -EBUSY;
> > > 
> > > -				break;
> > > 
> > >  			}
> > > 
> > > +			drm_modeset_unlock(&crtc->mutex);
> > > +			if (ret < 0)
> > > +				break;
> > > 
> > >  		}
> > > 
> > > -		drm_modeset_unlock_all(drm_dev);
> > > -
> > 
> > I might remove the {} around ret = -EBUSY, but this is good.
> > 
> > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> Yup patch looks good, but it's not cc'ed to drm/amdgpu maintainers/m-l, so
> likely won't get picked up. Can you pls check scripts/get_maintainers for
> anything you've missed, add those and resend with Willy's r-b tag
> included?
> 
> Then Alex can pick it up for merging.
> 
> Thanks, Daniel
>
I had already submitted a v2 of this patch with an added 'Review-by' Matthew 
Wilcox under my name. It removed the unnecessary braces that willy pointed 
out. However I see that not all maintainers had been cc'ed, so I'm going to  
resend it as v3.

Thanks,

Fabio