Message-ID: <CACvgo51JtxLcGAbBxn08=bQCGokeBobR+Z73ELs7U6q4QLa1-g@mail.gmail.com>
Date:   Fri, 2 Sep 2016 15:50:54 +0100
From:   Emil Velikov <emil.l.velikov@...il.com>
To:     Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc:     "Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
        linux-doc@...r.kernel.org, Jonathan Corbet <corbet@....net>,
        ML dri-devel <dri-devel@...ts.freedesktop.org>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Emil Velikov <emil.velikov@...labora.com>
Subject: Re: [PATCH v4 2/4] drm: Add API for capturing frame CRCs

Hi Tomeu,

On 5 August 2016 at 11:45, Tomeu Vizoso <tomeu.vizoso@...labora.com> 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@...olt.net>
> + *    Keith Packard <keithp@...thp.com>
> + *
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

Powered by blists - more mailing lists