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

[Emil Velikov]

On 29 September 2016 at 09:59, Daniel Vetter <daniel at 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 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.
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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20160929/de967404/attachment.html>