[Intel-gfx] [PATCH] drm: Add frame CRC debugfs files only for drivers that have CRTC

[Daniel Vetter]

On Thu, Sep 29, 2016 at 10:25:09AM +0200, Tomeu Vizoso wrote:
> On 29 September 2016 at 05:42, Dhinakaran Pandiyan
> <dhinakaran.pandiyan at intel.com> wrote:
> > vgem does not do modeset, looping through non-existent CRTC's while
> > registering drm_minor in
> >
> >         'commit 48c787899882 ("drm: Add API for capturing frame CRCs")'
> >
> > caused kernel oops. So, let's add CRC debugfs files
> > only for those drivers that do modeset.
> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > Cc: Tomeu Vizoso <tomeu.vizoso at collabora.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Emil Velikov <emil.velikov at collabora.com>
> 
> Reviewed-by: Tomeu Vizoso <tomeu.vizoso at collabora.com>

Applied to drm-misc, thanks.

> But I would prefer if drm_for_each_crtc was safe to call in any device
> regardless of the features that its driver supports.

Imo explicit checks are much better, especially in userspace-facing code
like this (well it's debugfs and not an ioctl, but still). Unrelated rant:
I think we should throw away the concept of having per-minor debugfs (with
a symlink for backwards compat). Then we could have put the crc code into
drm_modeset_register_all, and there would not have been any bug.
-Daniel

> 
> Regards,
> 
> Tomeu
> 
> > ---
> >  drivers/gpu/drm/drm_drv.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 70d2543..294404f 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -208,6 +208,7 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
> >         struct drm_crtc *crtc;
> >         unsigned long flags;
> >         int ret;
> > +       bool is_modeset;
> >
> >         DRM_DEBUG("\n");
> >
> > @@ -221,7 +222,8 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
> >                 return ret;
> >         }
> >
> > -       if (type == DRM_MINOR_PRIMARY) {
> > +       is_modeset = drm_core_check_feature(dev, DRIVER_MODESET);
> > +       if (type == DRM_MINOR_PRIMARY && is_modeset) {
> >                 drm_for_each_crtc(crtc, dev) {
> >                         ret = drm_debugfs_crtc_add(crtc);
> >                         if (ret)
> > @@ -255,12 +257,14 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
> >         struct drm_minor *minor;
> >         struct drm_crtc *crtc;
> >         unsigned long flags;
> > +       bool is_modeset;
> >
> >         minor = *drm_minor_get_slot(dev, type);
> >         if (!minor || !device_is_registered(minor->kdev))
> >                 return;
> >
> > -       if (type == DRM_MINOR_PRIMARY) {
> > +       is_modeset = drm_core_check_feature(dev, DRIVER_MODESET);
> > +       if (type == DRM_MINOR_PRIMARY && is_modeset) {
> >                 drm_for_each_crtc(crtc, dev)
> >                         drm_debugfs_crtc_remove(crtc);
> >         }
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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