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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
