* [PATCH] drm: Add frame CRC debugfs files only for drivers that have CRTC
@ 2016-09-29 3:42 Dhinakaran Pandiyan
2016-09-29 3:50 ` ✓ Fi.CI.BAT: success for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Dhinakaran Pandiyan @ 2016-09-29 3:42 UTC (permalink / raw)
To: dri-devel
Cc: Daniel Vetter, intel-gfx, Dhinakaran Pandiyan, Tomeu Vizoso,
Emil Velikov
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* ✓ Fi.CI.BAT: success for drm: Add frame CRC debugfs files only for drivers that have CRTC
2016-09-29 3:42 [PATCH] drm: Add frame CRC debugfs files only for drivers that have CRTC Dhinakaran Pandiyan
@ 2016-09-29 3:50 ` Patchwork
2016-09-29 7:54 ` [PATCH] " Jani Nikula
2016-09-29 8:25 ` Tomeu Vizoso
2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2016-09-29 3:50 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx
== Series Details ==
Series: drm: Add frame CRC debugfs files only for drivers that have CRTC
URL : https://patchwork.freedesktop.org/series/13053/
State : success
== Summary ==
Series 13053v1 drm: Add frame CRC debugfs files only for drivers that have CRTC
https://patchwork.freedesktop.org/api/1.0/series/13053/revisions/1/mbox/
Test drv_getparams_basic:
Subgroup basic-subslice-total:
incomplete -> PASS (fi-byt-j1900)
incomplete -> PASS (fi-skl-6700k)
incomplete -> PASS (fi-byt-n2820)
incomplete -> PASS (fi-snb-2520m)
incomplete -> PASS (fi-ivb-3520m)
incomplete -> PASS (fi-skl-6260u)
incomplete -> PASS (fi-bsw-n3050)
incomplete -> PASS (fi-snb-2600)
incomplete -> PASS (fi-skl-6770hq)
incomplete -> PASS (fi-ilk-650)
incomplete -> PASS (fi-bxt-t5700)
incomplete -> PASS (fi-skl-6700hq)
incomplete -> PASS (fi-hsw-4770)
incomplete -> PASS (fi-hsw-4770r)
incomplete -> PASS (fi-bdw-5557u)
Test vgem_reload_basic:
dmesg-fail -> PASS (fi-byt-j1900)
dmesg-fail -> PASS (fi-skl-6700k)
dmesg-fail -> PASS (fi-byt-n2820)
dmesg-fail -> PASS (fi-snb-2520m)
dmesg-fail -> PASS (fi-ivb-3520m)
dmesg-fail -> PASS (fi-skl-6260u)
dmesg-fail -> PASS (fi-bsw-n3050)
dmesg-fail -> PASS (fi-snb-2600)
dmesg-fail -> PASS (fi-skl-6770hq)
dmesg-fail -> PASS (fi-ilk-650)
dmesg-fail -> PASS (fi-bxt-t5700)
dmesg-fail -> PASS (fi-skl-6700hq)
dmesg-fail -> PASS (fi-hsw-4770)
dmesg-fail -> PASS (fi-hsw-4770r)
dmesg-fail -> PASS (fi-bdw-5557u)
fi-bdw-5557u total:244 pass:229 dwarn:0 dfail:0 fail:0 skip:15
fi-bsw-n3050 total:244 pass:202 dwarn:0 dfail:0 fail:0 skip:42
fi-bxt-t5700 total:244 pass:214 dwarn:0 dfail:0 fail:0 skip:30
fi-byt-j1900 total:244 pass:212 dwarn:0 dfail:0 fail:1 skip:31
fi-byt-n2820 total:244 pass:208 dwarn:0 dfail:0 fail:1 skip:35
fi-hsw-4770 total:244 pass:222 dwarn:0 dfail:0 fail:0 skip:22
fi-hsw-4770r total:244 pass:221 dwarn:0 dfail:0 fail:1 skip:22
fi-ilk-650 total:244 pass:182 dwarn:0 dfail:0 fail:2 skip:60
fi-ivb-3520m total:244 pass:219 dwarn:0 dfail:0 fail:0 skip:25
fi-ivb-3770 total:244 pass:207 dwarn:0 dfail:0 fail:0 skip:37
fi-skl-6260u total:244 pass:230 dwarn:0 dfail:0 fail:0 skip:14
fi-skl-6700hq total:244 pass:221 dwarn:1 dfail:0 fail:0 skip:22
fi-skl-6700k total:244 pass:219 dwarn:1 dfail:0 fail:0 skip:24
fi-skl-6770hq total:244 pass:228 dwarn:1 dfail:0 fail:1 skip:14
fi-snb-2520m total:244 pass:208 dwarn:0 dfail:0 fail:0 skip:36
fi-snb-2600 total:244 pass:207 dwarn:0 dfail:0 fail:0 skip:37
Results at /archive/results/CI_IGT_test/Patchwork_2590/
edf68a3aacccc2ad1ff8915fcc8011ad43d53be9 drm-intel-nightly: 2016y-09m-28d-15h-13m-34s UTC integration manifest
7b1057e drm: Add frame CRC debugfs files only for drivers that have CRTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: Add frame CRC debugfs files only for drivers that have CRTC
2016-09-29 3:42 [PATCH] drm: Add frame CRC debugfs files only for drivers that have CRTC Dhinakaran Pandiyan
2016-09-29 3:50 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2016-09-29 7:54 ` Jani Nikula
2016-09-29 8:25 ` Tomeu Vizoso
2 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2016-09-29 7:54 UTC (permalink / raw)
To: dri-devel
Cc: Daniel Vetter, intel-gfx, Dhinakaran Pandiyan, Tomeu Vizoso,
Emil Velikov
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);
> }
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: Add frame CRC debugfs files only for drivers that have CRTC
2016-09-29 3:42 [PATCH] drm: Add frame CRC debugfs files only for drivers that have CRTC Dhinakaran Pandiyan
2016-09-29 3:50 ` ✓ Fi.CI.BAT: success for " Patchwork
2016-09-29 7:54 ` [PATCH] " Jani Nikula
@ 2016-09-29 8:25 ` Tomeu Vizoso
2016-09-29 8:59 ` Daniel Vetter
2 siblings, 1 reply; 6+ messages in thread
From: Tomeu Vizoso @ 2016-09-29 8:25 UTC (permalink / raw)
To: Dhinakaran Pandiyan
Cc: Daniel Vetter, Intel Graphics Development, dri-devel, Emil Velikov
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: Add frame CRC debugfs files only for drivers that have CRTC
2016-09-29 8:25 ` Tomeu Vizoso
@ 2016-09-29 8:59 ` Daniel Vetter
2016-09-29 10:27 ` [Intel-gfx] " Emil Velikov
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2016-09-29 8:59 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: Daniel Vetter, Intel Graphics Development, Dhinakaran Pandiyan,
dri-devel, Emil Velikov
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-gfx] [PATCH] drm: Add frame CRC debugfs files only for drivers that have CRTC
2016-09-29 8:59 ` Daniel Vetter
@ 2016-09-29 10:27 ` Emil Velikov
0 siblings, 0 replies; 6+ messages in thread
From: Emil Velikov @ 2016-09-29 10:27 UTC (permalink / raw)
To: Daniel Vetter
Cc: Tomeu Vizoso, Daniel Vetter, Intel Graphics Development,
dri-devel, Dhinakaran Pandiyan, Emil Velikov
[-- Attachment #1.1: Type: text/plain, Size: 2235 bytes --]
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
[-- Attachment #1.2: Type: text/html, Size: 3110 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-29 10:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-29 3:42 [PATCH] drm: Add frame CRC debugfs files only for drivers that have CRTC Dhinakaran Pandiyan
2016-09-29 3:50 ` ✓ Fi.CI.BAT: success for " Patchwork
2016-09-29 7:54 ` [PATCH] " Jani Nikula
2016-09-29 8:25 ` Tomeu Vizoso
2016-09-29 8:59 ` Daniel Vetter
2016-09-29 10:27 ` [Intel-gfx] " Emil Velikov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.