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

On 3 August 2016 at 09:06, Ville Syrjälä ville.syrjala@linux.intel.com wrote:

On Fri, Jul 22, 2016 at 04:10:44PM +0200, Tomeu Vizoso wrote:

...

Adds files and directories to debugfs for controlling and reading frame CRCs, per CRTC:

dri/0/crtc-0/crc dri/0/crtc-0/crc/control dri/0/crtc-0/crc/data

Drivers can implement the set_crc_source callback() in drm_crtc_funcs to start and stop generating frame CRCs and can add entries to the output by calling drm_crtc_add_crc_entry.

v2: - Lots of good fixes suggested by Thierry. - Added documentation. - Changed the debugfs layout. - Moved to allocate the entries circular queue once when frame generation gets enabled for the first time. v3: - Use the control file just to select the source, and start and stop capture when the data file is opened and closed, respectively. - Make variable the number of CRC values per entry, per source. - Allocate entries queue each time we start capturing as now there isn't a fixed number of CRC values per entry. - Store the frame counter in the data file as a 8-digit hex number. - For sources that cannot provide useful frame numbers, place XXXXXXXX in the frame field.

Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com

...

...

+static ssize_t crc_control_write(struct file *file, const char __user *ubuf,

•                          size_t len, loff_t *offp)
  

+{

• struct seq_file *m = file->private_data;
  

• struct drm_crtc *crtc = m->private;
  

• struct drm_crtc_crc *crc = &crtc->crc;
  

• char *source;
  

• if (len == 0)
  

•         return 0;
  

• if (len > PAGE_SIZE - 1) {
  

•         DRM_DEBUG_KMS("Expected < %lu bytes into crtc crc control\n",
  

•                       PAGE_SIZE);
  

•         return -E2BIG;
  

• }
  

• source = kmalloc(len + 1, GFP_KERNEL);
  

• if (!source)
  

•         return -ENOMEM;
  

• if (copy_from_user(source, ubuf, len)) {
  

•         kfree(source);
  

•         return -EFAULT;
  

• }
  

memdup_user_nul() ?

Good call.

...

• if (source[len - 1] == '\n')
  

•         source[len - 1] = '\0';
  

• else
  

•         source[len] = '\0';
  

• spin_lock_irq(&crc->lock);
  

• if (crc->opened) {
  

•         kfree(source);
  

•         return -EBUSY;
  

• }
  

Why not just start the thing here?

For the sake of symmetry, as we are stopping when the data file is closed.

...

+static struct drm_crtc_crc_entry *crtc_get_crc_entry(struct drm_crtc_crc *crc,

•                                              int index)
  

+{

• void *p = crc->entries;
  

• size_t entry_size = (sizeof(*crc->entries) +
  

•                      sizeof(*crc->entries[0].crcs) * crc->values_cnt);
  

This computation is duplicated also in crtc_crc_open(). could use a common helper to do it.

Shame the language doesn't have a way to deal with arrays of variable sized arrays in a nice way.

Ok.

...

• return p + entry_size * index;
  

+}

+#define MAX_LINE_LEN (8 + 9 * DRM_MAX_CRC_NR + 1 + 1)

+static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,

•                      size_t count, loff_t *pos)
  

+{

• struct drm_crtc *crtc = filep->f_inode->i_private;
  

• struct drm_crtc_crc *crc = &crtc->crc;
  

• struct drm_crtc_crc_entry *entry;
  

• char buf[MAX_LINE_LEN];
  

• int ret, i;
  

• spin_lock_irq(&crc->lock);
  

• if (!crc->source) {
  

•         spin_unlock_irq(&crc->lock);
  

•         return 0;
  

• }
  

• /* Nothing to read? */
  

• while (crtc_crc_data_count(crc) == 0) {
  

•         if (filep->f_flags & O_NONBLOCK) {
  

•                 spin_unlock_irq(&crc->lock);
  

•                 return -EAGAIN;
  

•         }
  

•         ret = wait_event_interruptible_lock_irq(crc->wq,
  

•                                                 crtc_crc_data_count(crc),
  

•                                                 crc->lock);
  

•         if (ret) {
  

•                 spin_unlock_irq(&crc->lock);
  

•                 return ret;
  

•         }
  

• }
  

• /* We know we have an entry to be read */
  

• entry = crtc_get_crc_entry(crc, crc->tail);
  

• /*
  

•  * 1 frame field of 8 chars plus a number of CRC fields of 8
  

•  * chars each, space separated and with a newline at the end.
  

•  */
  

• if (count < 8 + 9 * crc->values_cnt + 1 + 1) {
  

Just < MAX_LINE_LEN perhaps? Or could make a macro/function that takes crc->values_cnt or DRM_MAX_CRC_NR as an argument.

Sounds good, went with a macro.

...

•         spin_unlock_irq(&crc->lock);
  

•         return -EINVAL;
  

• }
  

• BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRC_ENTRIES_NR);
  

• crc->tail = (crc->tail + 1) & (DRM_CRC_ENTRIES_NR - 1);
  

• spin_unlock_irq(&crc->lock);
  

• if (entry->has_frame_counter)
  

•         snprintf(buf, 9, "%08x", entry->frame);
  

• else
  

•         snprintf(buf, 9, "XXXXXXXX");
  

Should we add "0x" prefix to all these numbers to make it clear that they're in fact hex?

Sounds like a good idea to me.

...

• for (i = 0; i < crc->values_cnt; i++)
  

•         snprintf(buf + strlen(buf), 10, " %08x", entry->crcs[i]);
  

The 'n' in snprintf() here seems pointless. As does the strlen().

Good.

...

• snprintf(buf + strlen(buf), 2, "\n");
  

• if (copy_to_user(user_buf, buf, strlen(buf) + 1))
  

•         return -EFAULT;
  

• return strlen(buf) + 1;
  

More strlen()s that shouldn't be needed.

Ok.

Thanks!

Tomeu