Re: [PATCH v3 3/5] drm/dp: add helpers for capture of frame CRCs

[Tomeu Vizoso]

On 6 January 2017 at 16:56, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx> wrote:
>> Adds helpers for starting and stopping capture of frame CRCs through the
>> DPCD. When capture is on, a worker waits for vblanks and retrieves the
>> frame CRC to put it in the queue on the CRTC that is using the
>> eDP connector, so it's passed to userspace.
>>
>> v2: Reuse drm_crtc_wait_one_vblank
>>     Update locking, as drm_crtc_add_crc_entry now takes the lock
>>
>> v3: Don't call wake_up_interruptible directly, that's now done in
>>     drm_crtc_add_crc_entry.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
>> ---
>>
>>  drivers/gpu/drm/drm_dp_helper.c | 118 ++++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_dp_helper.h     |   7 +++
>>  2 files changed, 125 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index 3e6fe82c6d64..e531f1317268 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -981,6 +981,74 @@ static const struct i2c_lock_operations drm_dp_i2c_lock_ops = {
>>         .unlock_bus = unlock_bus,
>>  };
>>
>> +static int drm_dp_aux_get_crc(struct drm_dp_aux *aux, u8 *crc)
>> +{
>> +       u8 buf, count;
>> +       int ret;
>> +
>> +       ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       WARN_ON(!(buf & DP_TEST_SINK_START));
>> +
>> +       ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK_MISC, &buf);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       count = buf & DP_TEST_COUNT_MASK;
>> +       if (count == aux->crc_count)
>> +               return -EAGAIN; /* No CRC yet */
>> +
>> +       aux->crc_count = count;
>> +
>> +       /* At DP_TEST_CRC_R_CR, there's 6 bytes containing CRC data, 2 bytes
>> +        * per component (RGB or CrYCb).
>
> nit: doesn't conform to CodingStyle
>
>> +        */
>> +       ret = drm_dp_dpcd_read(aux, DP_TEST_CRC_R_CR, crc, 6);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       return 0;
>> +}
>> +
>> +static void drm_dp_aux_crc_work(struct work_struct *work)
>> +{
>> +       struct drm_dp_aux *aux = container_of(work, struct drm_dp_aux,
>> +                                             crc_work);
>> +       struct drm_crtc *crtc;
>> +       u8 crc_bytes[6];
>> +       uint32_t crcs[3];
>> +       int ret;
>> +
>> +       if (WARN_ON(!aux->connector))
>> +               return;
>> +
>> +       crtc = aux->connector->state->crtc;
>> +       while (crtc->crc.opened) {
>> +               drm_crtc_wait_one_vblank(crtc);
>> +               if (!crtc->crc.opened)
>> +                       break;
>> +
>> +               ret = drm_dp_aux_get_crc(aux, crc_bytes);
>> +               if (ret == -EAGAIN) {
>> +                       usleep_range(1000, 2000);
>> +                       ret = drm_dp_aux_get_crc(aux, crc_bytes);
>> +               }
>> +
>> +               if (ret) {
>> +                       DRM_DEBUG_KMS("Failed to get a CRC even after retrying: %d\n",
>> +                                     ret);
>> +                       continue;
>> +               }
>
> I think it'd be better to restructure this to only call
> drm_dp_aux_get_crc in one place

I'm not completely sure, TBH, as I liked that it was very clear that
we only retry once. I'm about to send v4 with this change and you can
judge by yourself.

> (and differentiate between EAGAIN and
> other failures):

Good point.

> bool retry = false;
> while (...) {
>         ...
>
>         ret = drm_dp_aux_get_crc(aux, crc_bytes);
>         if (ret == -EAGAIN) {
>                 if (retry)
>                         DRM_DEBUG_KMS("Get CRC failed after retrying: %d\n",
>                                       ret);
>                 retry = !retry;
>                 usleep_range(1000, 2000);
>                 continue;
>         }
>
>         retry = false;
>         if (!ret) {
>                 crcs[0] = crc_bytes[0] | crc_bytes[1] << 8;
>                 crcs[1] = crc_bytes[2] | crc_bytes[3] << 8;
>                 crcs[2] = crc_bytes[4] | crc_bytes[5] << 8;
>                 ret = drm_crtc_add_crc_entry(crtc, false, 0, crcs);
>                 if (ret)
>                         DRM_DEBUG_KMS("Failed to add crc entry %d\n", ret);
>         } else {
>                 DRM_DEBUG_KMS("Get CRC failed: %d\n", ret);
>         }
> }
>
>> +
>> +               crcs[0] = crc_bytes[0] | crc_bytes[1] << 8;
>> +               crcs[1] = crc_bytes[2] | crc_bytes[3] << 8;
>> +               crcs[2] = crc_bytes[4] | crc_bytes[5] << 8;
>> +               ret = drm_crtc_add_crc_entry(crtc, false, 0, crcs);
>> +       }
>> +}
>> +
>>  /**
>>   * drm_dp_aux_init() - minimally initialise an aux channel
>>   * @aux: DisplayPort AUX channel
>> @@ -993,6 +1061,7 @@ static const struct i2c_lock_operations drm_dp_i2c_lock_ops = {
>>  void drm_dp_aux_init(struct drm_dp_aux *aux)
>>  {
>>         mutex_init(&aux->hw_mutex);
>> +       INIT_WORK(&aux->crc_work, drm_dp_aux_crc_work);
>>
>>         aux->ddc.algo = &drm_dp_i2c_algo;
>>         aux->ddc.algo_data = aux;
>> @@ -1081,3 +1150,52 @@ int drm_dp_psr_setup_time(const u8 psr_cap[EDP_PSR_RECEIVER_CAP_SIZE])
>>  EXPORT_SYMBOL(drm_dp_psr_setup_time);
>>
>>  #undef PSR_SETUP_TIME
>> +
>> +/**
>> + * drm_dp_start_crc() - start capture of frame CRCs
>> + * @aux: DisplayPort AUX channel
>> + *
>> + * Returns 0 on success or a negative error code on failure.
>> + */
>> +int drm_dp_start_crc(struct drm_dp_aux *aux)
>> +{
>> +       u8 buf;
>> +       int ret;
>> +
>> +       ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ret = drm_dp_dpcd_writeb(aux, DP_TEST_SINK, buf | DP_TEST_SINK_START);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       aux->crc_count = 0;
>> +       schedule_work(&aux->crc_work);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(drm_dp_start_crc);
>> +
>> +/**
>> + * drm_dp_stop_crc() - stop capture of frame CRCs
>> + * @aux: DisplayPort AUX channel
>> + *
>> + * Returns 0 on success or a negative error code on failure.
>> + */
>> +int drm_dp_stop_crc(struct drm_dp_aux *aux)
>> +{
>> +       u8 buf;
>> +       int ret;
>> +
>> +       ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK, &buf);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ret = drm_dp_dpcd_writeb(aux, DP_TEST_SINK, buf & ~DP_TEST_SINK_START);
>> +       if (ret < 0)
>> +               return ret;
>> +
>
> Can we flush the worker here to head off any races?

Good point.

Thanks,

Tomeu