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

[Tomeu Vizoso]

On 3 August 2016 at 09:06, Ville SyrjÃlà <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
...
>> +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