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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
