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 ?
> +#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 ?
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() ?
> -#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);
> --- /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.
> +/*
> + * 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 ?
> +#define MAX_LINE_LEN (LINE_LEN(DRM_MAX_CRC_NR))
> +
Worth using lowercase VALUES_CNT and less likely to clash macro names ?
> + 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.
> --- 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 ?
> +#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.
> * 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/
> +/**
> + * 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 ?
> + 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).
> +};
> +
> +#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 ?
Regards,
Emil
--
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
