| Message ID | 1475120520-20156-1-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29 September 2016 at 05:42, Dhinakaran Pandiyan <dhinakaran.pandiyan@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@intel.com> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Emil Velikov <emil.velikov@collabora.com> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> But I would prefer if drm_for_each_crtc was safe to call in any device regardless of the features that its driver supports. 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Sep 29, 2016 at 10:25:09AM +0200, Tomeu Vizoso wrote: > On 29 September 2016 at 05:42, Dhinakaran Pandiyan > <dhinakaran.pandiyan@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@intel.com> > > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Emil Velikov <emil.velikov@collabora.com> > > Reviewed-by: Tomeu Vizoso <tomeu.vizoso@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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 29 September 2016 at 09:59, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Sep 29, 2016 at 10:25:09AM +0200, Tomeu Vizoso wrote: >> On 29 September 2016 at 05:42, Dhinakaran Pandiyan >> <dhinakaran.pandiyan@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@intel.com> >> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> >> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> > Cc: Emil Velikov <emil.velikov@collabora.com> >> >> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@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. Fwiw I agree that explicit checks are better. I keep forgetting that we create a card# node even if the device does not feature DRIVER_MODESET. IMHO if we drop that assumption, we can simplify core DRM and/or some drivers. If userspace is not new enough to know about render nodes it's simply not capable of using new drivers/devices properly and there will be no regressions afaict. Not to mention it the dubious auth requirement even if the device is render only. Alternatively if we can have add a comment why the card# is required that'll be greatly appreciated. That said, the patch is a good fix and is Reviewed-by: Emil Velikov <emil.velikov@collabora.com> Regards, Emil P.S. Some of the links on your blog have gone crazy [2] [1] https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#render-nodes [2] http://blog.ffwll.ch/2016/01/better-markup-for-kernel-gpu-docbook.html
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); }
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@intel.com> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Emil Velikov <emil.velikov@collabora.com> --- drivers/gpu/drm/drm_drv.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)