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@xxxxxxxxx> > > > --- > > > > > > 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@xxxxxxxxxxxxxxxx. To view this discussion on > > > the web visit > > > https://groups.google.com/d/msgid/outreachy-kernel/20210419122059.738-2-fmd > > > efrancesco%40gmail.com. > > > > > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
