Message-ID: <20160622133216.GL26943@ulmo.ba.sec>
Date:	Wed, 22 Jun 2016 15:32:16 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc:	Daniel Vetter <daniel.vetter@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	Emil Velikov <emil.velikov@...labora.com>
Subject: Re: [PATCH v1 2/3] drm: Add API for capturing frame CRCs

On Wed, Jun 22, 2016 at 10:26:36AM +0200, Tomeu Vizoso wrote:
> On 21 June 2016 at 17:07, Thierry Reding <thierry.reding@...il.com> 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.

This is pretty much the last chance to make changes to this interface,
so I think it'd be good to spend at least some time thinking about it
and if there's anything that could be improved.

> >> +             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.

You do clear the ring buffer when the configured source is changed,
right? If so, then userspace would automatically get the new CRC upon
the next read. How is that different than switching via the "none"
source?

> >> +
> >> +     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.

It could be a follow up patch, I don't mind much either way.

> >> +/*
> >> + * 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?

Right, I see now.

> >> + *   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.

In my opinion that makes things needlessly complicated for userspace. If
you want to query the state of a specific CRTC, you have to read out the
entire file and parse each line to find the correct CRTC. On the other
hand, chances are that you already need to know the path to the CRTC
because you want to read the CRC out of the per-CRTC CRC file. In that
case it would be much easier to simply concatenate the CRTC path and the
CRC (or control) filename and read a single line (actually a single
word) out of it to get at the same information.

Furthermore if you have everything per-CRTC you no longer have to worry
about pipe vs. index (that's always confusing because in the DRM core
they're actually synonymous) because the CRTC path is canonical and will
have the correct context.

Per-CRTC directory with a single duplex file, or separate control and
CRC files, is much simpler than the mix proposed here. No tokenization
required when parsing in userspace, and no tokenization required to
parse in the kernel either.

> >> +                                    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?

I worry slightly about the flexibility of this. On one hand it's
redundant to have to read 5 values if you only get one that's valid.
It's also ambiguous if you receive one value and 4 zeroes, because
zero could technically be a valid CRC.

Furthermore what if some hardware needed to provide more than five
values? One simple solution would be to make the number of values per
entry variable (and perhaps even include the count as first value in
the debugfs file).

On that note: what if there's hardware that doesn't fit into the above
format at all? I can't immediately think of anything, but that's not a
guarantee that it won't happen. I suppose we have to settle on something
for the sake of genericity, so one option would be for eccentric drivers
to encode whatever format they have into uint32_t:s. Also, anything that
doesn't fit into a couple of uint32_t:s could well be considered not a
CRC at all.

> >> +
> >> +             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.

I have difficulty coming up with a concrete use-case, but given that we
have most of the infrastructure to support it already, it might be nice
to have. Could of course be added later on if there's a need.

> >> +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?

I was referring to -1 not being a proper error code. Although I also
questioned whether the check is even necessary. Under what circumstances
will minor->debugfs_root be NULL at this point?

> >> +
> >> +     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.

Any client code dealing with display CRCs is going to be fairly DRM/KMS
specific, no? As such the use of the drm_ prefix is pretty redundant no
matter what.

> >> +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.

When you mention multiple sources, are the additional values meant to be
CRCs for additional sources? Or is it that some hardware generates more
than 32 bits for the CRC and drivers are supposed to split them up into
several values.

> 
> >> 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.

I recall seeing calls to this function from within the i915 driver,
wouldn't that cause the build to fail if i915 was built as a loadable
module?

> >> +      *
> >> +      * 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?

I don't know. It's probably okay to not have the aliases, and it should
be trivial to add them later on if we see that people get annoyed by the
fact that they have to write "auto" instead of "on" or "enable".

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists