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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jul 07, 2016 at 06:41:26PM +0300, Ville Syrjälä wrote:
> On Thu, Jul 07, 2016 at 05:06:08PM +0200, Tomeu Vizoso wrote:
> > +static int crtc_crc_release(struct inode *inode, struct file *filep)
> > +{
> > +	struct drm_crtc *crtc = filep->f_inode->i_private;
> > +	struct drm_crtc_crc *crc = &crtc->crc;
> > +
> > +	spin_lock_irq(&crc->lock);
> > +	crc->opened = false;
> > +	spin_unlock_irq(&crc->lock);
> > +
> > +	return 0;
> > +}
> 
> One thing I hate about the i915 code is the fact that it leaves crc
> capture on even if you close the file. igt has an exit handler to
> try to deal with that problem, but if the kernel just did the right
> thing from the start it wouln't be needed. Sadly I failed to convince
> Daniel that the kernel shouldn't suck. I still think it's a bad design.

Ack on fixing this. Not sure what exactly the interface should look like,
since right now igt does open/close the crc capture file a lot of times.
We'd need to change the igt backend quite a bit.

> > +		bytes_read += snprintf(buf, CRC_BUFFER_LEN,
> > +				       "%8u %8x %8x %8x %8x %8x\n",
> 
> You copied the bug from i915. Either make the frame counter hex, or give
> it more room to accomodate the full 32 bit number. Long time ago I posted
> a patch to use hex in i915, but it got stuck in some review limbo and
> as I had more important things to fix, I never revisited the issue.

Ack here too, totally forgotten about this one.

> In i915 we currently stuff the raw hardware frame counter in there, which
> isn't all that great since it's only 24 bits on some machines, and doesn't
> even exist on others. So userspace can't really deal with wraparound in
> any sane way. I guess just switching over to the sw counter universally
> could be the most sane way to go. Or maybe we should have room for both?

Either we go with the cooked vblank counter or both. Maybe both while at
it, it might come handy ... One problem is with CRC capturing in the sink,
where it's ill-defined how a sink frame corresponds to the drm vblank
counter. In that case we might need to return 0x######## or something else for
"invalid".

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch