| Message ID | 1475120520-20156-1-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 29 Sep 2016, 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> Fixes: 48c787899882 ("drm: Add API for capturing frame CRCs") Dhinakaran, for future reference, please add the kernel oops to the commit messages. It'll help reviewing the patch and matching other bug reports to the fix. Tomeu, Emil, please review ASAP. This started oopsing all over the place in our CI... we'll need to start running pre-merge testing on patches on dri-devel too, like we do on intel-gfx. (Hint, for now, Cc'ing intel-gfx on drm patches will run our CI on it.) Thanks, Jani. > --- > 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); > }
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(-)