Re: [PATCH 4/7] drm: Move the legacy kms disable_all helper to crtc helpers

From: Sean Paul <sean@poorly.run>
To: Alex Deucher <alexdeucher@gmail.com>
Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	Chunming Zhou <David1.Zhou@amd.com>,
	sbobroff@linux.ibm.com, Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	amd-gfx list <amd-gfx@lists.freedesktop.org>,
	Rex Zhu <Rex.Zhu@amd.com>, Dave Airlie <airlied@linux.ie>,
	Huang Rui <ray.huang@amd.com>,
	Maling list - DRI developers <dri-devel@lists.freedesktop.org>,
	nouveau <nouveau@lists.freedesktop.org>,
	"Deucher, Alexander" <alexander.deucher@amd.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Shaoyun Liu <Shaoyun.Liu@amd.com>,
	Christian Koenig <christian.koenig@amd.com>,
	"monk.liu" <Monk.Liu@amd.com>, Ben Skeggs <bskeggs@redhat.com>
Subject: Re: [PATCH 4/7] drm: Move the legacy kms disable_all helper to crtc helpers
Date: Tue, 11 Dec 2018 10:53:30 -0500	[thread overview]
Message-ID: <20181211155330.GD154160@art_vandelay> (raw)
In-Reply-To: <CADnq5_ODEwoeY0uomUYcokYyqP0+e34-hN6i2RJNjPEV1Fs=2w@mail.gmail.com>

On Mon, Dec 10, 2018 at 10:58:20AM -0500, Alex Deucher wrote:
> On Mon, Dec 10, 2018 at 5:04 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > It's not a core function, and the matching atomic functions are also
> > not in the core. Plus the suspend/resume helper is also already there.
> >
> > Needs a tiny bit of open-coding, but less midlayer beats that I think.
> >
> > Cc: Sam Bobroff <sbobroff@linux.ibm.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Ben Skeggs <bskeggs@redhat.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
> > Cc: Rex Zhu <Rex.Zhu@amd.com>
> > Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > Cc: Huang Rui <ray.huang@amd.com>
> > Cc: Shaoyun Liu <Shaoyun.Liu@amd.com>
> > Cc: Monk Liu <Monk.Liu@amd.com>
> > Cc: nouveau@lists.freedesktop.org
> > Cc: amd-gfx@lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  2 +-
> >  drivers/gpu/drm/drm_crtc.c                 | 31 -------------------
> >  drivers/gpu/drm/drm_crtc_helper.c          | 35 ++++++++++++++++++++++
> >  drivers/gpu/drm/nouveau/nouveau_display.c  |  2 +-
> >  drivers/gpu/drm/radeon/radeon_display.c    |  2 +-
> >  include/drm/drm_crtc.h                     |  2 --
> >  include/drm/drm_crtc_helper.h              |  1 +
> >  7 files changed, 39 insertions(+), 36 deletions(-)
> >

/snip

> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> > index a3c81850e755..23159eb494f1 100644
> > --- a/drivers/gpu/drm/drm_crtc_helper.c
> > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> > @@ -984,3 +984,38 @@ void drm_helper_resume_force_mode(struct drm_device *dev)
> >         drm_modeset_unlock_all(dev);
> >  }
> >  EXPORT_SYMBOL(drm_helper_resume_force_mode);
> > +
> > +/**
> > + * drm_helper_force_disable_all - Forcibly turn off all enabled CRTCs
> > + * @dev: DRM device whose CRTCs to turn off
> > + *
> > + * Drivers may want to call this on unload to ensure that all displays are
> > + * unlit and the GPU is in a consistent, low power state. Takes modeset locks.
> > + *
> > + * Note: This should only be used by non-atomic legacy drivers. For an atomic
> > + * version look at drm_atomic_helper_shutdown().
> > + *
> > + * Returns:
> > + * Zero on success, error code on failure.
> > + */
> > +int drm_helper_force_disable_all(struct drm_device *dev)
> 
> Maybe put crtc somewhere in the function name so it's clear what we
> are disabling.

FWIW, I think it's more clear this way. set_config_internal will turn off
everything attached to the crtc, so _everything_ will be disabled in this case.

Either way,

Reviewed-by: Sean Paul <sean@poorly.run>

Sean

> With that fixed:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> 
> > +{
> > +       struct drm_crtc *crtc;
> > +       int ret = 0;
> > +
> > +       drm_modeset_lock_all(dev);
> > +       drm_for_each_crtc(crtc, dev)
> > +               if (crtc->enabled) {
> > +                       struct drm_mode_set set = {
> > +                               .crtc = crtc,
> > +                       };
> > +
> > +                       ret = drm_mode_set_config_internal(&set);
> > +                       if (ret)
> > +                               goto out;
> > +               }
> > +out:
> > +       drm_modeset_unlock_all(dev);
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL(drm_helper_force_disable_all);
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> > index f326ffd86766..5d273a655479 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> > @@ -453,7 +453,7 @@ nouveau_display_fini(struct drm_device *dev, bool suspend, bool runtime)
> >                 if (drm_drv_uses_atomic_modeset(dev))
> >                         drm_atomic_helper_shutdown(dev);
> >                 else
> > -                       drm_crtc_force_disable_all(dev);
> > +                       drm_helper_force_disable_all(dev);
> >         }
> >
> >         /* disable flip completion events */
> > diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> > index e6912eb99b42..92332226e5cf 100644
> > --- a/drivers/gpu/drm/radeon/radeon_display.c
> > +++ b/drivers/gpu/drm/radeon/radeon_display.c
> > @@ -1643,7 +1643,7 @@ void radeon_modeset_fini(struct radeon_device *rdev)
> >         if (rdev->mode_info.mode_config_initialized) {
> >                 drm_kms_helper_poll_fini(rdev->ddev);
> >                 radeon_hpd_fini(rdev);
> > -               drm_crtc_force_disable_all(rdev->ddev);
> > +               drm_helper_force_disable_all(rdev->ddev);
> >                 radeon_fbdev_fini(rdev);
> >                 radeon_afmt_fini(rdev);
> >                 drm_mode_config_cleanup(rdev->ddev);
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index b45bec0b7a9c..85abd3fe9e83 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1149,8 +1149,6 @@ static inline uint32_t drm_crtc_mask(const struct drm_crtc *crtc)
> >         return 1 << drm_crtc_index(crtc);
> >  }
> >
> > -int drm_crtc_force_disable_all(struct drm_device *dev);
> > -
> >  int drm_mode_set_config_internal(struct drm_mode_set *set);
> >  struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx);
> >
> > diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
> > index d65f034843ce..0ee9a96b70da 100644
> > --- a/include/drm/drm_crtc_helper.h
> > +++ b/include/drm/drm_crtc_helper.h
> > @@ -56,6 +56,7 @@ bool drm_helper_encoder_in_use(struct drm_encoder *encoder);
> >  int drm_helper_connector_dpms(struct drm_connector *connector, int mode);
> >
> >  void drm_helper_resume_force_mode(struct drm_device *dev);
> > +int drm_helper_force_disable_all(struct drm_device *dev);
> >
> >  /* drm_probe_helper.c */
> >  int drm_helper_probe_single_connector_modes(struct drm_connector
> > --
> > 2.20.0.rc1
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx