[Outreachy kernel] [PATCH 1/2] drm/amd/amdgpu/amdgpu_device.c: Replace drm_modeset_*_all with DRM_MODESET_LOCK_ALL_*

[Julia Lawall]

On Mon, 19 Apr 2021, Fabio M. De Francesco wrote:

> On Monday, April 19, 2021 3:08:51 PM CEST Julia Lawall wrote:
> > On Mon, 19 Apr 2021, Fabio M. De Francesco wrote:
> > > Replace the deprecated API with new helpers, according to the TODO list
> > > of the DRM subsystem.
> >
> > The commit message will perhaps not be very meaningful one year from now.
> > You could say for example DRM_MODESET_LOCK_ALL_BEGIN was introduced in
> > commit XXX (there is a proper format for referring to other commits) for
> > what purpose.  And then say that you are making the replacement.
> >
> > Actually, I'm a little surprised to see something that looks like a
> > function call be replaced by something that looks like a macro.  Maybe it
> > was a macro all along, and this is just making that more clear.  In any
> > case, if I were to look at this commit, I would appreciate a little more
> > context information.
> >
> > julia
> >
> I have made that message in line with an old commit (9bcaa3fe58ab) that had
> been taken by D. Vetter (the DRM maintainer). That message didn't explain more
> than referring to the TODO list. I try to stick with each subsystem's
> conventions.
>
> However, I agree with you: referring to a TODO list, which some day  will
> surely change, is not the best means to provide context information.
>
> From that macro documentation (https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#c.DRM_MODESET_LOCK_ALL_BEGIN):
>
> "DRM_MODESET_LOCK_ALL_BEGIN  simplifies grabbing all modeset locks using a
> local context. This has the advantage of reducing boilerplate, [...]".
> (Please, note that I haven't copied the last part of the paragraph because it
> talks about checking the return value but I think it's useless because the
> only possible return value is 0).
>
> If more context information is needed, I would add the above-mentioned note to
> my commit message and submit a v2 patch.
>
> Is it the right way to solve the issue that you pointed out?

It seems find to include the information you propose.

thanks,
julia

>
> Thanks in advance,
>
> Fabio
> >
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco at gmail.com>
> > > ---
> > >
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index
> > > 6447cd6ca5a8..e1a71579f8e6 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > @@ -32,6 +32,7 @@
> > >
> > >  #include <linux/slab.h>
> > >
> > >  #include <drm/drm_atomic_helper.h>
> > >
> > > +#include <drm/drm_drv.h>
> > >
> > >  #include <drm/drm_probe_helper.h>
> > >  #include <drm/amdgpu_drm.h>
> > >  #include <linux/vgaarb.h>
> > >
> > > @@ -3694,14 +3695,17 @@ int amdgpu_device_suspend(struct drm_device *dev,
> > > bool fbcon)>
> > >  	if (!amdgpu_device_has_dc_support(adev)) {
> > >
> > >  		/* turn off display hw */
> > >
> > > -		drm_modeset_lock_all(dev);
> > > +		struct drm_modeset_acquire_ctx ctx;
> > > +		int ret;
> > > +
> > > +		DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> > >
> > >  		drm_connector_list_iter_begin(dev, &iter);
> > >  		drm_for_each_connector_iter(connector, &iter)
> > >
> > >  			drm_helper_connector_dpms(connector,
> > >
> > >
> DRM_MODE_DPMS_OFF);
> > >
> > >  		drm_connector_list_iter_end(&iter);
> > >
> > > -		drm_modeset_unlock_all(dev);
> > > -			/* unpin the front buffers and cursors */
> > > +		DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> > > +		/* unpin the front buffers and cursors */
> > >
> > >  		list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> head) {
> > >
> > >  			struct amdgpu_crtc *amdgpu_crtc =
> to_amdgpu_crtc(crtc);
> > >  			struct drm_framebuffer *fb = crtc->primary-
> >fb;
> > >
> > > @@ -3830,19 +3834,21 @@ int amdgpu_device_resume(struct drm_device *dev,
> > > bool fbcon)>
> > >  	/* blat the mode back in */
> > >  	if (fbcon) {
> > >
> > >  		if (!amdgpu_device_has_dc_support(adev)) {
> > >
> > > +			struct drm_modeset_acquire_ctx ctx;
> > > +			int ret;
> > > +
> > >
> > >  			/* pre DCE11 */
> > >  			drm_helper_resume_force_mode(dev);
> > >
> > >  			/* turn on display hw */
> > >
> > > -			drm_modeset_lock_all(dev);
> > > +			DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0,
> ret);
> > >
> > >  			drm_connector_list_iter_begin(dev, &iter);
> > >  			drm_for_each_connector_iter(connector,
> &iter)
> > >
> > >
> drm_helper_connector_dpms(connector,
> > >
> > >
> DRM_MODE_DPMS_ON);
> > >
> > >  			drm_connector_list_iter_end(&iter);
> > >
> > > -
> > > -			drm_modeset_unlock_all(dev);
> > > +			DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> > >
> > >  		}
> > >  		amdgpu_fbdev_set_suspend(adev, 0);
> > >
> > >  	}
> > >
> > > --
> > > 2.31.1
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "outreachy-kernel" group. To unsubscribe from this group and stop
> receiving
> > > emails from it, send an email to
> > > outreachy-kernel+unsubscribe at googlegroups.com. To view this discussion on
> > > the web visit
> > > https://groups.google.com/d/msgid/outreachy-kernel/20210419122059.738-2-fmd
> > > efrancesco%40gmail.com.
>
>
>
>
>