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

[Ville Syrjälä]

On Fri, Aug 05, 2016 at 12:46:29PM +0200, Tomeu Vizoso wrote:
> 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.

Yes, but if the data file is already open, we should start as soon as
the source is configured. Or are you redusing to open the data file w/o
a source selected?

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

-- 
Ville Syrjälä
Intel OTC