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

[Daniel Vetter]

On Thu, Jun 23, 2016 at 10:43 AM, Thierry Reding
 wrote:
> On Thu, Jun 23, 2016 at 10:24:46AM +0200, Tomeu Vizoso wrote:
>> On 23 June 2016 at 10:21, Jani Nikula  wrote:
>> > On Wed, 22 Jun 2016, Daniel Vetter  wrote:
>> >> On Wed, Jun 22, 2016 at 4:31 PM, Thierry Reding
>> >>  wrote:
>> >>> Perhaps another way to avoid that would be to put the two files into a
>> >>> separate directory, as in:
>> >>>
>> >>> /sys/kernel/debug/dri//crtc-/crc/
>> >>> +-- control
>> >>> +-- data
>> >>>
>> >>> That's slightly on the deeply nested side, but on the other hand it
>> >>> nicely uses the filesystem for namespacing, which is what filesystems
>> >>> are really good at.
>> >>
>> >> crtc-/crc/(control|data) sounds great.
>> >
>> > Side note, we should eventually do the same for sink CRCs, but I guess
>> > under the connectors. i915 currently has a special cased version for eDP
>> > (named "i915_sink_crc_eDP1"...), reading the data from DPCD.
>>
>> Was hoping we could just add one more source to this interface to expose 
>> those.
>
> I don't think that would be easy to achieve. You'd have to determine
> that a sink source is connected to the CRTC and then ajdust the list
> of possible sources dynamically.
>
> For connectors we already have separate directories in debugfs and a
> sink can easily be found from the connector it is attached to.
>
> It's also possible, though I don't know of any that do so currently,
> that eventually sinks will support several types (i.e. "sources") of
> CRC, which will further complicate to represent this in the CRTC's
> list of CRC sources.
>
> And it may also be useful to have both CRCs at the same time. The eDP
> specification for example defines a very specific polynomial that is
> used to compute the CRC, whereas not all display hardware uses a CRC
> algorithm that's documented.
>
> Finally, the data that will be checksummed by the CRTC isn't necessarily
> the same as that arriving at the sink.

We already do all that for i915. On some platforms the normal
end-of-pipe CRC doesn't work together with DP, instead you need to use
a special port (= connector/encoder) based CRC. The "auto" one
automatically walks the modeset config on those platforms to figure
out which one to pick. It would be kinda hard for userspace to
automatically figure out when the auto CRC doesn't work on the CRTC
and it would instead need to use the connector one. I think it'd
better to hide that in the kernel driver, where it's known.

Wrt specific polynomials: We could just spec that the "eDP-sink"
source is the one with CRC computed per the eDP spec. Similar for
anything else standardize. Heck you could even do the same with vendor
specific CRCs on IP blocks by using special names.

Wrt CRC measuring different data: That's also already the case on
i915. We have CRC for pre-gamma, post-gamma, post panel-fitter, on the
port, with or without audio stream included,  And this all even
varies between platforms. The only thing that's specifified is that
"auto" should be some CRC after all the blending/color-correction has
been applied, but before any port specific scrambling happens which
might make the CRC change with each frame. E.g. on platforms where
auto aliases the DP ports we need to set a special bit to reset the
scrambler state on each vblank to ensure stable CRCs.

Given all that I think putting sink crc capture into the same
framework makes sense.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Jani Nikula]

On Wed, 22 Jun 2016, Daniel Vetter  wrote:
> On Wed, Jun 22, 2016 at 4:31 PM, Thierry Reding
>  wrote:
>> Perhaps another way to avoid that would be to put the two files into a
>> separate directory, as in:
>>
>> /sys/kernel/debug/dri//crtc-/crc/
>> +-- control
>> +-- data
>>
>> That's slightly on the deeply nested side, but on the other hand it
>> nicely uses the filesystem for namespacing, which is what filesystems
>> are really good at.
>
> crtc-/crc/(control|data) sounds great.

Side note, we should eventually do the same for sink CRCs, but I guess
under the connectors. i915 currently has a special cased version for eDP
(named "i915_sink_crc_eDP1"...), reading the data from DPCD.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Thierry Reding]

On Thu, Jun 23, 2016 at 10:24:46AM +0200, Tomeu Vizoso wrote:
> On 23 June 2016 at 10:21, Jani Nikula  wrote:
> > On Wed, 22 Jun 2016, Daniel Vetter  wrote:
> >> On Wed, Jun 22, 2016 at 4:31 PM, Thierry Reding
> >>  wrote:
> >>> Perhaps another way to avoid that would be to put the two files into a
> >>> separate directory, as in:
> >>>
> >>> /sys/kernel/debug/dri//crtc-/crc/
> >>> +-- control
> >>> +-- data
> >>>
> >>> That's slightly on the deeply nested side, but on the other hand it
> >>> nicely uses the filesystem for namespacing, which is what filesystems
> >>> are really good at.
> >>
> >> crtc-/crc/(control|data) sounds great.
> >
> > Side note, we should eventually do the same for sink CRCs, but I guess
> > under the connectors. i915 currently has a special cased version for eDP
> > (named "i915_sink_crc_eDP1"...), reading the data from DPCD.
> 
> Was hoping we could just add one more source to this interface to expose 
> those.

I don't think that would be easy to achieve. You'd have to determine
that a sink source is connected to the CRTC and then ajdust the list
of possible sources dynamically.

For connectors we already have separate directories in debugfs and a
sink can easily be found from the connector it is attached to.

It's also possible, though I don't know of any that do so currently,
that eventually sinks will support several types (i.e. "sources") of
CRC, which will further complicate to represent this in the CRTC's
list of CRC sources.

And it may also be useful to have both CRCs at the same time. The eDP
specification for example defines a very specific polynomial that is
used to compute the CRC, whereas not all display hardware uses a CRC
algorithm that's documented.

Finally, the data that will be checksummed by the CRTC isn't necessarily
the same as that arriving at the sink.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

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

[Tomeu Vizoso]

On 23 June 2016 at 10:21, Jani Nikula  wrote:
> On Wed, 22 Jun 2016, Daniel Vetter  wrote:
>> On Wed, Jun 22, 2016 at 4:31 PM, Thierry Reding
>>  wrote:
>>> Perhaps another way to avoid that would be to put the two files into a
>>> separate directory, as in:
>>>
>>> /sys/kernel/debug/dri//crtc-/crc/
>>> +-- control
>>> +-- data
>>>
>>> That's slightly on the deeply nested side, but on the other hand it
>>> nicely uses the filesystem for namespacing, which is what filesystems
>>> are really good at.
>>
>> crtc-/crc/(control|data) sounds great.
>
> Side note, we should eventually do the same for sink CRCs, but I guess
> under the connectors. i915 currently has a special cased version for eDP
> (named "i915_sink_crc_eDP1"...), reading the data from DPCD.

Was hoping we could just add one more source to this interface to expose those.

Regards,

Tomeu

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

[Daniel Vetter]

On Wed, Jun 22, 2016 at 4:31 PM, Thierry Reding
 wrote:
> On Wed, Jun 22, 2016 at 04:08:52PM +0200, Daniel Vetter wrote:
>> On Wed, Jun 22, 2016 at 3:32 PM, Thierry Reding
>>  wrote:
>> >> >> + *   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.
>>
>> Just jumping on this one here. I agree that if we remodel the
>> interface making the control file per-crtc would make sense. I think
>> separate control and read files makes sense, that's much less magic.
>
> Agreed, separate files would be a little simpler. I must admit that my
> proposal is partially motivated by a desire to avoid cumbersome naming
> of files. If we have separate files, what do you name them? crc for
> reading, crc_control for writing? crc_values for reading and crc for
> writing?
>
> Perhaps another way to avoid that would be to put the two files into a
> separate directory, as in:
>
> /sys/kernel/debug/dri//crtc-/crc/
> +-- control
> +-- data
>
> That's slightly on the deeply nested side, but on the other hand it
> nicely uses the filesystem for namespacing, which is what filesystems
> are really good at.

crtc-/crc/(control|data) sounds great.

>> And by reading the control file you can check what's the currently
>> selected source easily.
>
> Is that really a useful feature? If you're going to capture CRCs, you
> likely just want to set whatever you expect to receive irrespective of
> the current setting.

I think it's useful for hacking together your driver support, going
through tests it one more indirection. And I have a really bad
attention span ;-)

>> I'm not sure on the canonical CRTC path - right now we don't have that
>> in debugfs. I think just using index numbers is ok, we use those all
>> over the place already. Or maybe we could indeed add a new per-crtc
>> subdir in debugfs for this. Either way is fine with me.
>
> I can imagine that we'd like to expose a number of other per-CRTC
> properties (name, parts of the state, object ID, one day perhaps VBLANK
> counts, ...) this way, so a per-CRTC directory makes a lot of sense in
> my opinion.

Yeah, we might have room for more ... otoh for read-only debug
information I prefer big files that dump everything. Easier to extend.
But for tests/automation the one-value-per-file idea from sysfs, or at
least going much closer to that idea is a good idea.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Thierry Reding]

On Wed, Jun 22, 2016 at 04:08:52PM +0200, Daniel Vetter wrote:
> On Wed, Jun 22, 2016 at 3:32 PM, Thierry Reding
>  wrote:
> >> >> + *   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.
> 
> Just jumping on this one here. I agree that if we remodel the
> interface making the control file per-crtc would make sense. I think
> separate control and read files makes sense, that's much less magic.

Agreed, separate files would be a little simpler. I must admit that my
proposal is partially motivated by a desire to avoid cumbersome naming
of files. If we have separate files, what do you name them? crc for
reading, crc_control for writing? crc_values for reading and crc for
writing?

Perhaps another way to avoid that would be to put the two files into a
separate directory, as in:

/sys/kernel/debug/dri//crtc-/crc/
+-- control
+-- data

That's slightly on the deeply nested side, but on the other hand it
nicely uses the filesystem for namespacing, which is what filesystems
are really good at.

> And by reading the control file you can check what's the currently
> selected source easily.

Is that really a useful feature? If you're going to capture CRCs, you
likely just want to set whatever you expect to receive irrespective of
the current setting.

> I'm not sure on the canonical CRTC path - right now we don't have that
> in debugfs. I think just using index numbers is ok, we use those all
> over the place already. Or maybe we could indeed add a new per-crtc
> subdir in debugfs for this. Either way is fine with me.

I can imagine that we'd like to expose a number of other per-CRTC
properties (name, parts of the state, object ID, one day perhaps VBLANK
counts, ...) this way, so a per-CRTC directory makes a lot of sense in
my opinion.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

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

[Daniel Vetter]

On Tue, Jun 21, 2016 at 01:06:41PM +0200, Tomeu Vizoso wrote:
> Adds a per-device debugfile "drm_crc_control" that allows selecting a
> source for frame checksums in each CRTC that supports them.
> 
> The checksums for each subsequent frame can be read from the per-CRTC
> file "drm_crtc_N_crc".
> 
> The code is taken from the i915 driver and other drivers can now provide
> frame CRCs by implementing the set_crc_source callback in
> drm_crtc_funcs.
> 
> Signed-off-by: Tomeu Vizoso 
> ---
> 
>  drivers/gpu/drm/drm_crtc.c |  28 ++-
>  drivers/gpu/drm/drm_debugfs.c  | 506 
> -
>  drivers/gpu/drm/drm_internal.h |  10 +
>  include/drm/drmP.h |   5 +
>  include/drm/drm_crtc.h |  72 ++
>  5 files changed, 611 insertions(+), 10 deletions(-)

I think we should finalize the internal and external api first, but this
needs a bit better documentation. For that I think it'd be good to extract
this to a new file like drm_debugfs_crc.[hc] and pull that into a new
section (maybe under the testing and validation section, next to the igt
howto) in drm-uapi.rst.

More doc bikeshedding below.

> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e7c862bd2f19..4dae42b122d9 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -657,8 +657,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, 
> struct drm_crtc *crtc,
>  drm_num_crtcs(dev));
>   }
>   if (!crtc->name) {
> - drm_mode_object_unregister(dev, >base);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto err_unregister;
>   }
>  
>   crtc->base.properties = >properties;
> @@ -673,12 +673,30 @@ int drm_crtc_init_with_planes(struct drm_device *dev, 
> struct drm_crtc *crtc,
>   if (cursor)
>   cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
>  
> +#ifdef CONFIG_DEBUG_FS
> + spin_lock_init(>crc.lock);
> + init_waitqueue_head(>crc.wq);
> + crtc->crc.debugfs_entries = kmalloc_array(DRM_MINOR_CNT,
> +   sizeof(*crtc->crc.debugfs_entries),
> +   GFP_KERNEL);
> +
> + ret = drm_debugfs_crtc_add(crtc);
> + if (ret)
> + goto err_free_name;
> +#endif
> +
>   if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>   drm_object_attach_property(>base, config->prop_active, 0);
>   drm_object_attach_property(>base, config->prop_mode_id, 
> 0);
>   }
>  
>   return 0;
> +
> +err_free_name:
> + kfree(crtc->name);
> +err_unregister:
> + drm_mode_object_unregister(dev, >base);
> + return ret;
>  }
>  EXPORT_SYMBOL(drm_crtc_init_with_planes);
>  
> @@ -699,6 +717,12 @@ 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.debugfs_entries);
> + kfree(crtc->crc.source);
> +#endif
> +
>   kfree(crtc->gamma_store);
>   crtc->gamma_store = NULL;
>  
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index fa10cef2ba37..cdc8836bc22a 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -30,6 +30,8 @@
>   * OTHER DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -127,6 +129,259 @@ fail:
>  }
>  EXPORT_SYMBOL(drm_debugfs_create_files);
>  
> +static int
> +drm_add_fake_info_node(struct drm_minor *minor,
> +struct dentry *ent,
> +const void *key)
> +{
> + struct drm_info_node *node;
> +
> + node = kmalloc(sizeof(*node), GFP_KERNEL);
> + if (node == NULL) {
> + debugfs_remove(ent);
> + return -ENOMEM;
> + }
> +
> + node->minor = minor;
> + node->dent = ent;
> + node->info_ent = (void *) key;
> +
> + mutex_lock(>debugfs_lock);
> + list_add(>list, >debugfs_list);
> + mutex_unlock(>debugfs_lock);
> +
> + return 0;
> +}
> +
> +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;
> +}
> +
> +static int crc_control_open(struct inode *inode, struct file *file)
> +{
> + struct drm_device *dev = inode->i_private;
> +
> + return single_open(file, crc_control_show, dev);
> +}
> +
> +static int crc_control_update_crtc(struct drm_crtc *crtc, const char *source)
> +{
> + struct drm_crtc_crc *crc = >crc;
> + struct drm_crtc_crc_entry *entries;
> + int ret;
> +
> + if (!strcmp(source, "none"))
> + source = NULL;
> +
> + 

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

[Daniel Vetter]

On Wed, Jun 22, 2016 at 3:32 PM, Thierry Reding
 wrote:
>> >> +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.

O_NONBLOCK is definitely needed to be able to write testcases, so that
you can schedule flips and collect CRCs for them in parallel (e.g. to
validate atomic flips by making sure there's never an intermediate CRC
with garbage). I haven't seen a need yet for poll, and like you say
it's easy to add if we ever need it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Wed, Jun 22, 2016 at 3:32 PM, Thierry Reding
 wrote:
>> >> + *   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.

Just jumping on this one here. I agree that if we remodel the
interface making the control file per-crtc would make sense. I think
separate control and read files makes sense, that's much less magic.
And by reading the control file you can check what's the currently
selected source easily.

I'm not sure on the canonical CRTC path - right now we don't have that
in debugfs. I think just using index numbers is ok, we use those all
over the place already. Or maybe we could indeed add a new per-crtc
subdir in debugfs for this. Either way is fine with me.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Thierry Reding]

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

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

[Tomeu Vizoso]

On 21 June 2016 at 17:07, Thierry Reding  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(>lock);
>> + }
>> +
>> + spin_unlock_irq(>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 

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

[Thierry Reding]

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
> +drm_add_fake_info_node(struct drm_minor *minor,
> +struct dentry *ent,
> +const void *key)

Nit: this fits on two lines instead of four.

> +{
> + struct drm_info_node *node;
> +
> + node = kmalloc(sizeof(*node), GFP_KERNEL);
> + if (node == NULL) {
> + debugfs_remove(ent);

You already remove this in the caller upon returning an error, no need
to do it twice. The caller is where it should be removed.

> + return -ENOMEM;
> + }
> +
> + node->minor = minor;
> + node->dent = ent;
> + node->info_ent = (void *) key;
> +
> + mutex_lock(>debugfs_lock);
> + list_add(>list, >debugfs_list);
> + mutex_unlock(>debugfs_lock);
> +
> + return 0;
> +}

Is there a specific reason why you use the drm_info_node infrastructure
here? Seems like it'd be simpler just doing plain debugfs.

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

> +static int crc_control_open(struct inode *inode, struct file *file)
> +{
> + struct drm_device *dev = inode->i_private;
> +
> + return single_open(file, crc_control_show, dev);
> +}
> +
> +static int crc_control_update_crtc(struct drm_crtc *crtc, const char *source)
> +{
> + struct drm_crtc_crc *crc = >crc;
> + struct drm_crtc_crc_entry *entries;
> + int ret;
> +
> + if (!strcmp(source, "none"))

Nit: I think it's more idiomatic to write the == 0 explicitly for
strcmp() for readability. My brain always interprets !strcmp() as
"strings are not equal", whereas == 0 is more explicit as in "the
difference between strings is 0". But perhaps that's just personal
taste.

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

> + if (!crtc->funcs->set_crc_source)
> + return -ENOTSUPP;
> +
> + if (source) {
> + entries = kcalloc(DRM_CRTC_CRC_ENTRIES_NR,
> +   sizeof(crc->entries[0]),
> +   GFP_KERNEL);
> + if (!entries)
> + return -ENOMEM;
> +
> + spin_lock_irq(>lock);
> + kfree(crc->entries);
> + crc->entries = entries;
> + crc->head = 0;
> + crc->tail = 0;
> + spin_unlock_irq(>lock);
> + }

Why are we reallocating? Couldn't we simply allocate once and keep
reusing the existing array?

> +
> + ret = crtc->funcs->set_crc_source(crtc, source);
> + if (ret)
> + return ret;
> +
> + kfree(crc->source);
> + crc->source = source ? kstrdup(source, GFP_KERNEL) : NULL;
> +
> + if (!source) {
> + spin_lock_irq(>lock);
> + entries = crc->entries;
> + crc->entries = NULL;
> + crc->head = 0;
> + crc->tail = 0;
> + spin_unlock_irq(>lock);
> +
> + kfree(entries);
> + }

This frees the entries array after resetting source to "none", but what
if we never do that? Aren't we going to leak that data at CRTC removal?

> +static struct drm_crtc *crtc_from_index(struct drm_device *dev, int index)

unsigned int index?

> +{
> + struct drm_crtc *crtc;
> + int i = 0;

unsigned int?

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

> +/*
> + * Parse CRC command strings:
> + *   command: wsp* object wsp+ (crtc | pipe) wsp+ source wsp*

Should the "(crtc | pipe)" in the above be "object"?

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

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

[Tomeu Vizoso]

Adds a per-device debugfile "drm_crc_control" that allows selecting a
source for frame checksums in each CRTC that supports them.

The checksums for each subsequent frame can be read from the per-CRTC
file "drm_crtc_N_crc".

The code is taken from the i915 driver and other drivers can now provide
frame CRCs by implementing the set_crc_source callback in
drm_crtc_funcs.

Signed-off-by: Tomeu Vizoso 
---

 drivers/gpu/drm/drm_crtc.c |  28 ++-
 drivers/gpu/drm/drm_debugfs.c  | 506 -
 drivers/gpu/drm/drm_internal.h |  10 +
 include/drm/drmP.h |   5 +
 include/drm/drm_crtc.h |  72 ++
 5 files changed, 611 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e7c862bd2f19..4dae42b122d9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -657,8 +657,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, 
struct drm_crtc *crtc,
   drm_num_crtcs(dev));
}
if (!crtc->name) {
-   drm_mode_object_unregister(dev, >base);
-   return -ENOMEM;
+   ret = -ENOMEM;
+   goto err_unregister;
}

crtc->base.properties = >properties;
@@ -673,12 +673,30 @@ int drm_crtc_init_with_planes(struct drm_device *dev, 
struct drm_crtc *crtc,
if (cursor)
cursor->possible_crtcs = 1 << drm_crtc_index(crtc);

+#ifdef CONFIG_DEBUG_FS
+   spin_lock_init(>crc.lock);
+   init_waitqueue_head(>crc.wq);
+   crtc->crc.debugfs_entries = kmalloc_array(DRM_MINOR_CNT,
+ sizeof(*crtc->crc.debugfs_entries),
+ GFP_KERNEL);
+
+   ret = drm_debugfs_crtc_add(crtc);
+   if (ret)
+   goto err_free_name;
+#endif
+
if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
drm_object_attach_property(>base, config->prop_active, 0);
drm_object_attach_property(>base, config->prop_mode_id, 
0);
}

return 0;
+
+err_free_name:
+   kfree(crtc->name);
+err_unregister:
+   drm_mode_object_unregister(dev, >base);
+   return ret;
 }
 EXPORT_SYMBOL(drm_crtc_init_with_planes);

@@ -699,6 +717,12 @@ 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.debugfs_entries);
+   kfree(crtc->crc.source);
+#endif
+
kfree(crtc->gamma_store);
crtc->gamma_store = NULL;

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index fa10cef2ba37..cdc8836bc22a 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -30,6 +30,8 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */

+#include 
+#include 
 #include 
 #include 
 #include 
@@ -127,6 +129,259 @@ fail:
 }
 EXPORT_SYMBOL(drm_debugfs_create_files);

+static int
+drm_add_fake_info_node(struct drm_minor *minor,
+  struct dentry *ent,
+  const void *key)
+{
+   struct drm_info_node *node;
+
+   node = kmalloc(sizeof(*node), GFP_KERNEL);
+   if (node == NULL) {
+   debugfs_remove(ent);
+   return -ENOMEM;
+   }
+
+   node->minor = minor;
+   node->dent = ent;
+   node->info_ent = (void *) key;
+
+   mutex_lock(>debugfs_lock);
+   list_add(>list, >debugfs_list);
+   mutex_unlock(>debugfs_lock);
+
+   return 0;
+}
+
+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;
+}
+
+static int crc_control_open(struct inode *inode, struct file *file)
+{
+   struct drm_device *dev = inode->i_private;
+
+   return single_open(file, crc_control_show, dev);
+}
+
+static int crc_control_update_crtc(struct drm_crtc *crtc, const char *source)
+{
+   struct drm_crtc_crc *crc = >crc;
+   struct drm_crtc_crc_entry *entries;
+   int ret;
+
+   if (!strcmp(source, "none"))
+   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;
+
+   if (!crtc->funcs->set_crc_source)
+   return -ENOTSUPP;
+
+   if (source) {
+   entries = kcalloc(DRM_CRTC_CRC_ENTRIES_NR,
+ sizeof(crc->entries[0]),
+ GFP_KERNEL);
+   if (!entries)
+