On 29 September 2016 at 09:59, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
>> > Cc: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
>> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> > Cc: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
>>
>> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
>
> 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.
[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
> On Thu, Sep 29, 2016 at 10:25:09AM +0200, Tomeu Vizoso wrote:
>> On 29 September 2016 at 05:42, Dhinakaran Pandiyan
>> <dhinakaran.pandiyan@xxxxxxxxx
>> > 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@xxxxxxxxx
>> > Cc: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
>> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> > Cc: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
>>
>> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
>
> 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
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
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
