Re: [PATCH v4 2/4] drm: Add API for capturing frame CRCs

[Tomeu Vizoso]

On 2 September 2016 at 16:50, Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote:
> Hi Tomeu,
>
> On 5 August 2016 at 11:45, Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx> wrote:
>
>> +#ifdef CONFIG_DEBUG_FS
>> +       spin_lock_init(&crtc->crc.lock);
>> +       init_waitqueue_head(&crtc->crc.wq);
>> +       crtc->crc.source = kstrdup("auto", GFP_KERNEL);
> Pedantic: kstrdup() can never fail ?

Dealt with.

>> +#endif
>> +
>>         if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>>                 drm_object_attach_property(&crtc->base, config->prop_active, 0);
>>                 drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
>> @@ -764,6 +771,11 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
>>          * the indices on the drm_crtc after us in the crtc_list.
>>          */
>>
>> +#ifdef CONFIG_DEBUG_FS
>> +       drm_debugfs_crtc_remove(crtc);
>> +       kfree(crtc->crc.source);
>> +#endif
>> +
> Worth adding these two as static inlines in the header ?

Have moved the ifdefs to static functions in the same file.

> Things are a bit asymetrical here. drm_debugfs_crtc_add() is in
> drm_minor_register(), so why isn't drm_debugfs_crtc_remove() in
> drm_minor_free() ?

Good call. Have moved drm_debugfs_crtc_remove to drm_minor_unregister.

>> -#endif /* CONFIG_DEBUG_FS */
>> +int drm_debugfs_crtc_add(struct drm_crtc *crtc)
>> +{
>> +       struct drm_minor *minor = crtc->dev->primary;
>> +       struct dentry *root;
>> +       char *name;
>> +
>> +       name = kasprintf(GFP_KERNEL, "crtc-%d", crtc->index);
> Mildly related: some userspace projects define convenience helpers
> which you use in order to allocate the correct amount of space on
> stack.
> Is there such a set for the kernel ?
>
> With those the above will become something like:
> char name[5+DECIMAL_STR_MAX] = ksprintf("crtc-%d", crtc->index);

Don't know of those but I think that in this specific case an
allocation is not that much of a problem.

>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>
>> + *
>> + * Authors:
>> + *    Eric Anholt <eric@xxxxxxxxxx>
>> + *    Keith Packard <keithp@xxxxxxxxxx>
>> + *
> Wondering about these authorship lines - is the code copied/derived
> from somewhere ? If so please mention where from.

Done, and also fixed the actual people that should be in there.

>> +/*
>> + * 1 frame field of 10 chars plus a number of CRC fields of 10 chars each, space
>> + * separated and with a newline at the end.
>> + */
>> +#define LINE_LEN(VALUES_CNT)   (10 + 11 * VALUES_CNT + 1 + 1)
> Hmm I seem to suck at math. The macro seems larger than expected (comment and
> code wise) by 1, right ?

The comment indeed should mention that it's NULL-terminated, but the
code is correct, isn't it?

>> +#define MAX_LINE_LEN           (LINE_LEN(DRM_MAX_CRC_NR))
>> +
> Worth using lowercase VALUES_CNT and less likely to clash macro names ?

Done.

>
>
>> +       BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRC_ENTRIES_NR);
>> +       crc->tail = (crc->tail + 1) & (DRM_CRC_ENTRIES_NR - 1);
> It's strange that the kernel does not have a macro for this. But for the sake
> of me I cannot find one.

Yeah.

>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -192,6 +192,7 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type)
>>  static int drm_minor_register(struct drm_device *dev, unsigned int type)
>>  {
>>         struct drm_minor *minor;
>> +       struct drm_crtc *crtc;
>>         unsigned long flags;
>>         int ret;
>>
>> @@ -207,6 +208,14 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
>>                 return ret;
>>         }
>>
>> +       if (type == DRM_MINOR_LEGACY) {
> Sidenote: If we did not use DRM_MINOR_LEGACY for render-only GPUs we would
> save a couple of cycles here :-)
>
>> +               drm_for_each_crtc(crtc, dev) {
>> +                       ret = drm_debugfs_crtc_add(crtc);
>> +                       if (ret)
>> +                               goto err_debugfs;
> IMHO we don't want to bring down the whole setup if this fails, unlike
> where we cannot create debug/dri all together.
>
> Regardless: we seems to be missing the cleanup in the error path ?

Have changed it to only log an error and continue, so no cleanup is necessary.

>> +#if defined(CONFIG_DEBUG_FS)
>> +extern const struct file_operations drm_crc_control_fops;
>> +extern const struct file_operations drm_crtc_crc_fops;
> These seems to be named differently in the code. Namely:
>
> drm_crtc_crc_control_fops
> drm_crtc_crc_data_fops
>
> Then again, you don't seem to use the outside of the file, so I'd make them
> static and drop this hunk.

Oops.

>
>
>>          * before data structures are torndown.
>>          */
>>         void (*early_unregister)(struct drm_crtc *crtc);
>> +
>> +       /**
>> +        * @set_crc_source:
>> +        *
>> +        * Changes the source of CRC checksums of frames at the request of
>> +        * userspace, typically for testing purposes. The sources available are
>> +        * specific of each driver and a %NULL value indicates that CRC
>> +        * generation is to be switched off.
>> +        *
>> +        * When CRC generation is enabled, the driver should call
>> +        * drm_crtc_add_crc_entry() at each frame, providing any information
>> +        * that characterizes the frame contents in the crcN arguments, as
>> +        * provided from the configured source. Drivers should accept a "auto"
> Very nicely written documentation. Just to make it more obvious - s/should/must/

Ok.

>> +/**
>> + * struct drm_crtc_crc_entry - entry describing a frame's content
>> + * @frame: number of the frame this CRC is about
>> + * @crc: array of values that characterize the frame
>> + */
>> +struct drm_crtc_crc_entry {
>> +       bool has_frame_counter;
> Missing documentation. What is/should frame (below) going to contain if false ?

Ok.

>> +       uint32_t frame;
>> +       uint32_t crcs[0];
> Likely the only one here: using a zero sized array is a GNU extension which we
> can avoid. Esp since we cap the size to 10 (DRM_MAX_CRC_NR).

Agreed, I also think that the added code complexity isn't worth the
reduced memory usage.

>> +};
>> +
>> +#define DRM_MAX_CRC_NR         10
>> +#define DRM_CRC_ENTRIES_NR     128
>> +/**
>> + * struct drm_crtc_crc - data supporting CRC capture on a given CRTC
>> + * @lock: protects the fields in this struct
>> + * @source: name of the currently configured source of CRCs
>> + * @opened: whether userspace has opened the data file for reading
>> + * @entries: array of entries, with size of %DRM_CRTC_CRC_ENTRIES_NR
>> + * @head: head of circular queue
>> + * @tail: tail of circular queue
>> + * @wq: workqueue used to synchronize reading and writing
>> + */
>> +struct drm_crtc_crc {
>> +       spinlock_t lock;
>> +       const char *source;
>> +       bool opened;
>> +       struct drm_crtc_crc_entry *entries;
>> +       int head, tail;
>> +       size_t values_cnt;
> Missing documentation. Please explicitly state its relation to DRM_MAX_CRC_NR,
> as you did with entries and DRM_CRTC_CRC_ENTRIES_NR. Worth moving where it's
> applicable/used - i.e. in drm_crtc_crc_entry ?

Done.

Lots of good stuff, thanks a lot!

Tomeu