Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs

[Tomeu Vizoso]

On 21 June 2016 at 17:07, Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> On Tue, Jun 21, 2016 at 01:06:41PM +0200, Tomeu Vizoso wrote:
> [...]
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> [...]
>
>> +
>> +static int crc_control_show(struct seq_file *m, void *data)
>> +{
>> +     struct drm_device *dev = m->private;
>> +     struct drm_crtc *crtc;
>> +
>> +     drm_for_each_crtc(crtc, dev)
>> +             seq_printf(m, "crtc %d %s\n", crtc->index,
>> +                        crtc->crc.source ? crtc->crc.source : "none");
>> +
>> +     return 0;
>> +}
>
> Why are these control files not per-CRTC? I'd imagine you could do
> something like control the CRC generation on writes and provide the
> sampled CRCs on reads.

We just thought there wasn't a strong point for breaking the existing
API in a fundamental way. The current proposal allows us to reuse more
code between the new and legacy CRC implementations in i915 and in
IGT.

>> +             source = NULL;
>> +
>> +     if (!crc->source && !source)
>> +             return 0;
>> +
>> +     if (crc->source && source && !strcmp(crc->source, source))
>> +             return 0;
>> +
>> +     /* Forbid changing the source without going back to "none". */
>> +     if (crc->source && source)
>> +             return -EINVAL;
>
> Why? It seems to me that if a driver doesn't support switching from one
> source to another directly, then it should internally handle that. After
> all the source parameter is already driver-specific, so it seems odd to
> impose this kind of policy on it at this level.

Hmm, I don't see when that would make sense for userspace. If
userspace has a source configured and changes directly to another, how
does it know what's the last CRC for the old source? I think that if
userspace does that it's shooting in its foot and it's good to give an
error.

>> +
>> +     drm_for_each_crtc(crtc, dev)
>> +             if (i++ == index)
>> +                     return crtc;
>> +
>> +     return NULL;
>> +}
>
> This looks like a candidate for the core. I know that at least Tegra
> implements a variant of this, and I think i915 does, too.

And a few others. I would go this way but when I pinged danvet on irc
he didn't reply so I just went with the safe option.

>> +/*
>> + * Parse CRC command strings:
>> + *   command: wsp* object wsp+ (crtc | pipe) wsp+ source wsp*
>
> Should the "(crtc | pipe)" in the above be "object"?

In one case they are literals and in the other symbols.

>> + *   object: ('crtc' | 'pipe')
>
> Because you define that here?
>
>> + *   crtc: (0 | 1 | 2 | ...)
>> + *   pipe: (A | B | C)
>> + *   source: (none | plane1 | plane2 | ...)
>
> I wouldn't provide "plane1 | plane2 |" here, since the parameter is
> passed as-is to drivers, which may or may not support plane1 or plane2.

Agreed, feels more confusing than clarifying.

>> + *   wsp: (#0x20 | #0x9 | #0xA)+
>> + *
>> + * eg.:
>> + *  "crtc 0 plane1"  ->  Start CRC computations on plane1 of first CRTC
>> + *  "crtc 0 none"    ->  Stop CRC
>
> I've said this above, but again, it seems odd to me that you'd have to
> configure the CRC per-CRTC in one per-device file and read out the CRC
> from per-CRTC files.

Not sure, I like that the per-crtc files just provide CRC data, and
that there's a separate control file that can be queried for the
current state.

>
>> +                                    entry->frame, entry->crc[0],
>> +                                    entry->crc[1], entry->crc[2],
>> +                                    entry->crc[3], entry->crc[4]);
>
> What about drivers that only support one uint32_t for the CRC? Do they
> also need to output all unused uint32_t:s?

Yeah, do you think that could be a problem?

>> +
>> +             ret = copy_to_user(user_buf, buf, CRC_LINE_LEN);
>> +             if (ret == CRC_LINE_LEN)
>> +                     return -EFAULT;
>>
>> +             user_buf += CRC_LINE_LEN;
>> +             n_entries--;
>> +
>> +             spin_lock_irq(&crc->lock);
>> +     }
>> +
>> +     spin_unlock_irq(&crc->lock);
>> +
>> +     return bytes_read;
>> +}
>> +
>> +const struct file_operations drm_crtc_crc_fops = {
>> +     .owner = THIS_MODULE,
>> +     .open = crtc_crc_open,
>> +     .read = crtc_crc_read,
>> +     .release = crtc_crc_release,
>> +};
>
> Do we want to support poll?

Don't see the point of it, TBH.

>> +
>> +static int drm_debugfs_crtc_add_for_minor(struct drm_crtc *crtc,
>> +                                       struct drm_minor *minor)
>> +{
>> +     struct dentry *ent;
>> +     char *name;
>> +
>> +     if (!minor->debugfs_root)
>> +             return -1;
>
> Can this ever happen? Perhaps turn this into a symbolic name if you
> really need it.

Sorry, can you explain what you mean by that?

>> +
>> +     name = kasprintf(GFP_KERNEL, "drm_crtc_%d_crc", crtc->index);
>> +     if (!name)
>> +             return -ENOMEM;
>
> I think it might be preferable to move this all into per-CRTC debugfs
> directories, perhaps even collapse the "crc" and "control" files. But in
> any case I think the drm_ prefix is redundant here and should be
> dropped.

Yeah, I kind of like the redundancy as in the client code you will
only sometimes find the file name next to the directory name, but I
don't particularly care myself.

>> +int drm_debugfs_crtc_add(struct drm_crtc *crtc)
>> +{
>> +     int ret;
>> +
>> +     ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->control);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->primary);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = drm_debugfs_crtc_add_for_minor(crtc, crtc->dev->render);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return 0;
>> +}
>
> Do we really want these for all minors? Is ->primary not enough? It
> certainly seems completely misplaced in ->render, and I don't think
> anything really uses ->control anymore.

Agreed.

>> +void drm_crtc_add_crc_entry(struct drm_crtc *crtc, uint32_t frame,
>> +                         uint32_t crc0, uint32_t crc1, uint32_t crc2,
>> +                         uint32_t crc3, uint32_t crc4)
>
> Perhaps allow passing the CRC as an array with a count parameter? I can
> imagine that a lot of hardware will only give you a single uint32_t for
> the CRC, in which case you could do:
>
>         drm_crtc_add_crc_entry(crtc, frame, &crc, 1);
>
> instead of:
>
>         drm_crtc_add_crc_entry(crtc, frame, crc, 0, 0, 0, 0);
>
> It would probably save poor users of the interface, such as myself, a
> lot of headaches because they can't remember how many uint32_t:s the
> function needs.

Sounds good to me, I don't really know how common multiple sources of
complex CRC data will be in the future.

>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index 38401d406532..e5b124d937f5 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -100,6 +100,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>>  int drm_debugfs_cleanup(struct drm_minor *minor);
>>  int drm_debugfs_connector_add(struct drm_connector *connector);
>>  void drm_debugfs_connector_remove(struct drm_connector *connector);
>> +int drm_debugfs_crtc_add(struct drm_crtc *crtc);
>> +void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
>
> Oh... this isn't something that drivers are supposed to call?

Right, it's analogous to drm_debugfs_connector_add.

>> +      *
>> +      * 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"
>> +      * source name that will select a default source for this CRTC.
>
> Would it be useful to provide some more aliases? "enable" and "on" for
> "auto", "disable" and "off" for "none"?

Not sure, TBH, i like to keep my interfaces simple. Do you think this
could be helpful?

Thanks for the great review!

Tomeu