[PATCH v3 3/5] drm/dp: add helpers for capture of frame CRCs
On 6 January 2017 at 16:56, Sean Paul wrote:
> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso
> 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
>> ---
>>
>> 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, );
>> + if (ret < 0)
>> + return ret;
>> +
>> + WARN_ON(!(buf & DP_TEST_SINK_START));
>> +
>> + ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK_MISC, );
>> + 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(>hw_mutex);
[PATCH v3 3/5] drm/dp: add helpers for capture of frame CRCs
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
---
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, );
+ if (ret < 0)
+ return ret;
+
+ WARN_ON(!(buf & DP_TEST_SINK_START));
+
+ ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK_MISC, );
+ 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).
+*/
+ 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;
+ }
+
+ 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(>hw_mutex);
+ INIT_WORK(>crc_work, drm_dp_aux_crc_work);
aux->ddc.algo = _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, );
+ 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(>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, );
+ if (ret < 0)
+ return ret;
+
+ ret = drm_dp_dpcd_writeb(aux, DP_TEST_SINK, buf & ~DP_TEST_SINK_START);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_dp_stop_crc);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 4fa77b434594..276e1ecd947b 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -723,6 +723,8 @@ struct drm_dp_aux_msg {
* @dev: pointer to struct device that is the parent for this
[PATCH v3 3/5] drm/dp: add helpers for capture of frame CRCs
On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso
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
> ---
>
> 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, );
> + if (ret < 0)
> + return ret;
> +
> + WARN_ON(!(buf & DP_TEST_SINK_START));
> +
> + ret = drm_dp_dpcd_readb(aux, DP_TEST_SINK_MISC, );
> + 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 (and differentiate between EAGAIN and
other failures):
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(>hw_mutex);
> + INIT_WORK(>crc_work, drm_dp_aux_crc_work);
>
> aux->ddc.algo = _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
