[PATCH v3 0/5] drm/dp: Implement CRC debugfs API

[PATCH v3 0/5] drm/dp: Implement CRC debugfs API

[Tomeu Vizoso]

Hi,

this series builds up on the API for exposing captured CRCs through
debugfs.

It adds new DP helpers for starting and stopping CRC capture and gets
the Rockchip driver to use it.

Also had to add a connector backpointer to the drm_dp_aux struct so we could
wait for the right vblank and store the CRCs afterwards, I will be glad
to hear about better alternatives.

With these patches, tests in IGT such as kms_pipe_crc_basic and
kms_plane do pass on RK3288.

Thanks,

Tomeu


Tomeu Vizoso (5):
  drm/dp: add connector backpointer to drm_dp_aux
  drm/bridge: analogix_dp: set connector to drm_dp_aux
  drm/dp: add helpers for capture of frame CRCs
  drm/bridge: analogix_dp: add helpers for capture of frame CRCs
  drm/rockchip: Implement CRC debugfs API

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  34 ++++--
 drivers/gpu/drm/drm_dp_helper.c                    | 118 +++++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  48 +++++++++
 include/drm/bridge/analogix_dp.h                   |   3 +
 include/drm/drm_dp_helper.h                        |   9 ++
 5 files changed, 204 insertions(+), 8 deletions(-)

-- 
2.9.3

[PATCH v3 1/5] drm/dp: add connector backpointer to drm_dp_aux

[Tomeu Vizoso]

This backpointer allows DP helpers to access the connector it's being
used for.

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

 include/drm/drm_dp_helper.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 55bbeb0ff594..4fa77b434594 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -721,6 +721,7 @@ struct drm_dp_aux_msg {
  * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
  * @ddc: I2C adapter that can be used for I2C-over-AUX communication
  * @dev: pointer to struct device that is the parent for this AUX channel
+ * @connector: backpointer to connector that uses this AUX channel
  * @hw_mutex: internal mutex used for locking transfers
  * @transfer: transfers a message representing a single AUX transaction
  *
@@ -757,6 +758,7 @@ struct drm_dp_aux {
 	const char *name;
 	struct i2c_adapter ddc;
 	struct device *dev;
+	struct drm_connector *connector;
 	struct mutex hw_mutex;
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);
-- 
2.9.3

[PATCH v3 2/5] drm/bridge: analogix_dp: set connector to drm_dp_aux

[Tomeu Vizoso]

Set the backpointer so that the DP helpers are able to access the
connector that the drm_dp_aux is associated with.

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

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 02b97bf64ee4..7d45d3e4600a 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1402,23 +1402,25 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 	dp->drm_dev = drm_dev;
 	dp->encoder = dp->plat_data->encoder;
 
+	ret = analogix_dp_create_bridge(drm_dev, dp);
+	if (ret) {
+		DRM_ERROR("failed to create bridge (%d)\n", ret);
+		goto err_encoder_cleanup;
+	}
+
 	dp->aux.name = "DP-AUX";
 	dp->aux.transfer = analogix_dpaux_transfer;
 	dp->aux.dev = &pdev->dev;
+	dp->aux.connector = &dp->connector;
 
 	ret = drm_dp_aux_register(&dp->aux);
 	if (ret)
-		goto err_disable_pm_runtime;
-
-	ret = analogix_dp_create_bridge(drm_dev, dp);
-	if (ret) {
-		DRM_ERROR("failed to create bridge (%d)\n", ret);
-		drm_encoder_cleanup(dp->encoder);
-		goto err_disable_pm_runtime;
-	}
+		goto err_encoder_cleanup;
 
 	return 0;
 
+err_encoder_cleanup:
+	drm_encoder_cleanup(dp->encoder);
 err_disable_pm_runtime:
 	pm_runtime_disable(dev);
 
-- 
2.9.3

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

[Tomeu Vizoso]

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@collabora.com>
---

 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).
+	 */
+	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(&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;
+
+	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 AUX channel
  * @connector: backpointer to connector that uses this AUX channel
  * @hw_mutex: internal mutex used for locking transfers
+ * @crc_work: worker that captures CRCs for each frame
+ * @crc_count: counter of captured frame CRCs
  * @transfer: transfers a message representing a single AUX transaction
  *
  * The .dev field should be set to a pointer to the device that implements
@@ -760,6 +762,8 @@ struct drm_dp_aux {
 	struct device *dev;
 	struct drm_connector *connector;
 	struct mutex hw_mutex;
+	struct work_struct crc_work;
+	u8 crc_count;
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);
 	/**
@@ -838,4 +842,7 @@ void drm_dp_aux_init(struct drm_dp_aux *aux);
 int drm_dp_aux_register(struct drm_dp_aux *aux);
 void drm_dp_aux_unregister(struct drm_dp_aux *aux);
 
+int drm_dp_start_crc(struct drm_dp_aux *aux);
+int drm_dp_stop_crc(struct drm_dp_aux *aux);
+
 #endif /* _DRM_DP_HELPER_H_ */
-- 
2.9.3

[PATCH v3 4/5] drm/bridge: analogix_dp: add helpers for capture of frame CRCs

[Tomeu Vizoso]

Add two simple functions that just take the drm_dp_aux from our struct
and calls the corresponding DP helpers with it.

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

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 16 ++++++++++++++++
 include/drm/bridge/analogix_dp.h                   |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 7d45d3e4600a..02f63eb1b887 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1483,6 +1483,22 @@ int analogix_dp_resume(struct device *dev)
 EXPORT_SYMBOL_GPL(analogix_dp_resume);
 #endif
 
+int analogix_dp_start_crc(struct drm_connector *connector)
+{
+	struct analogix_dp_device *dp = to_dp(connector);
+
+	return drm_dp_start_crc(&dp->aux);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_start_crc);
+
+int analogix_dp_stop_crc(struct drm_connector *connector)
+{
+	struct analogix_dp_device *dp = to_dp(connector);
+
+	return drm_dp_stop_crc(&dp->aux);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_stop_crc);
+
 MODULE_AUTHOR("Jingoo Han <jg1.han@samsung.com>");
 MODULE_DESCRIPTION("Analogix DP Core Driver");
 MODULE_LICENSE("GPL v2");
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index f6f0c062205c..c99d6eaef1ac 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -49,4 +49,7 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 		     struct analogix_dp_plat_data *plat_data);
 void analogix_dp_unbind(struct device *dev, struct device *master, void *data);
 
+int analogix_dp_start_crc(struct drm_connector *connector);
+int analogix_dp_stop_crc(struct drm_connector *connector);
+
 #endif /* _ANALOGIX_DP_H_ */
-- 
2.9.3

[PATCH v3 5/5] drm/rockchip: Implement CRC debugfs API

[Tomeu Vizoso]

Implement the .set_crc_source() callback and call the DP helpers
accordingly to start and stop CRC capture.

This is only done if this CRTC is currently using the eDP connector.

v3: Remove superfluous check on rockchip_crtc_state->output_type

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

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 48 +++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fb5f001f51c3..5e19bef6d5b4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -19,6 +19,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_flip_work.h>
 #include <drm/drm_plane_helper.h>
+#include <drm/bridge/analogix_dp.h>
 
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -1105,6 +1106,52 @@ static void vop_crtc_destroy_state(struct drm_crtc *crtc,
 	kfree(s);
 }
 
+static struct drm_connector *vop_get_edp_connector(struct vop *vop)
+{
+	struct drm_crtc *crtc = &vop->crtc;
+	struct drm_connector *connector;
+
+	mutex_lock(&crtc->dev->mode_config.mutex);
+	drm_for_each_connector(connector, crtc->dev)
+		if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
+			mutex_unlock(&crtc->dev->mode_config.mutex);
+			return connector;
+		}
+	mutex_unlock(&crtc->dev->mode_config.mutex);
+
+	return NULL;
+}
+
+static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
+				   const char *source_name, size_t *values_cnt)
+{
+	struct vop *vop = to_vop(crtc);
+	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc->state);
+	struct drm_connector *connector;
+	int ret;
+
+	connector = vop_get_edp_connector(vop);
+	if (!connector)
+		return -EINVAL;
+
+	*values_cnt = 3;
+
+	if (source_name &&
+	    strcmp(source_name, "auto") == 0) {
+
+		if (s->output_type != DRM_MODE_CONNECTOR_eDP)
+			return -EINVAL;
+
+		ret = analogix_dp_start_crc(connector);
+	} else if (!source_name)
+
+		ret = analogix_dp_stop_crc(connector);
+	else
+		ret = -EINVAL;
+
+	return ret;
+}
+
 static const struct drm_crtc_funcs vop_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
@@ -1112,6 +1159,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
 	.reset = vop_crtc_reset,
 	.atomic_duplicate_state = vop_crtc_duplicate_state,
 	.atomic_destroy_state = vop_crtc_destroy_state,
+	.set_crc_source = vop_crtc_set_crc_source,
 };
 
 static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
-- 
2.9.3

Re: [PATCH v3 1/5] drm/dp: add connector backpointer to drm_dp_aux

[Sean Paul]

On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> This backpointer allows DP helpers to access the connector it's being
> used for.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
>  include/drm/drm_dp_helper.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 55bbeb0ff594..4fa77b434594 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -721,6 +721,7 @@ struct drm_dp_aux_msg {
>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
>   * @ddc: I2C adapter that can be used for I2C-over-AUX communication
>   * @dev: pointer to struct device that is the parent for this AUX channel
> + * @connector: backpointer to connector that uses this AUX channel
>   * @hw_mutex: internal mutex used for locking transfers
>   * @transfer: transfers a message representing a single AUX transaction
>   *
> @@ -757,6 +758,7 @@ struct drm_dp_aux {
>         const char *name;
>         struct i2c_adapter ddc;
>         struct device *dev;
> +       struct drm_connector *connector;

Perhaps I'm misreading the series, but it seems like you could instead
add int crc_pipe along with the other crc fields. Then when you start
the crc, set the pipe number. If you have the pipe in the crc worker,
you can wait vblank on that pipe (no pointers needed).

Reasonable?

Sean

>         struct mutex hw_mutex;
>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>                             struct drm_dp_aux_msg *msg);
> --
> 2.9.3
>



-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[Sean Paul]

On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> 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@collabora.com>
> ---
>
>  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 (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(&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?


> +       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 AUX channel
>   * @connector: backpointer to connector that uses this AUX channel
>   * @hw_mutex: internal mutex used for locking transfers
> + * @crc_work: worker that captures CRCs for each frame
> + * @crc_count: counter of captured frame CRCs
>   * @transfer: transfers a message representing a single AUX transaction
>   *
>   * The .dev field should be set to a pointer to the device that implements
> @@ -760,6 +762,8 @@ struct drm_dp_aux {
>         struct device *dev;
>         struct drm_connector *connector;
>         struct mutex hw_mutex;
> +       struct work_struct crc_work;
> +       u8 crc_count;
>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>                             struct drm_dp_aux_msg *msg);
>         /**
> @@ -838,4 +842,7 @@ void drm_dp_aux_init(struct drm_dp_aux *aux);
>  int drm_dp_aux_register(struct drm_dp_aux *aux);
>  void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>
> +int drm_dp_start_crc(struct drm_dp_aux *aux);
> +int drm_dp_stop_crc(struct drm_dp_aux *aux);
> +
>  #endif /* _DRM_DP_HELPER_H_ */
> --
> 2.9.3
>



-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 5/5] drm/rockchip: Implement CRC debugfs API

[Sean Paul]

On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> Implement the .set_crc_source() callback and call the DP helpers
> accordingly to start and stop CRC capture.
>
> This is only done if this CRTC is currently using the eDP connector.
>
> v3: Remove superfluous check on rockchip_crtc_state->output_type
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 48 +++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index fb5f001f51c3..5e19bef6d5b4 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -19,6 +19,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_flip_work.h>
>  #include <drm/drm_plane_helper.h>
> +#include <drm/bridge/analogix_dp.h>
>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -1105,6 +1106,52 @@ static void vop_crtc_destroy_state(struct drm_crtc *crtc,
>         kfree(s);
>  }
>
> +static struct drm_connector *vop_get_edp_connector(struct vop *vop)
> +{
> +       struct drm_crtc *crtc = &vop->crtc;
> +       struct drm_connector *connector;
> +
> +       mutex_lock(&crtc->dev->mode_config.mutex);
> +       drm_for_each_connector(connector, crtc->dev)
> +               if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> +                       mutex_unlock(&crtc->dev->mode_config.mutex);
> +                       return connector;
> +               }
> +       mutex_unlock(&crtc->dev->mode_config.mutex);
> +
> +       return NULL;
> +}
> +
> +static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
> +                                  const char *source_name, size_t *values_cnt)
> +{
> +       struct vop *vop = to_vop(crtc);
> +       struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc->state);
> +       struct drm_connector *connector;
> +       int ret;
> +
> +       connector = vop_get_edp_connector(vop);
> +       if (!connector)
> +               return -EINVAL;
> +
> +       *values_cnt = 3;
> +
> +       if (source_name &&
> +           strcmp(source_name, "auto") == 0) {
> +
> +               if (s->output_type != DRM_MODE_CONNECTOR_eDP)
> +                       return -EINVAL;
> +
> +               ret = analogix_dp_start_crc(connector);

To pick up my thoughts from 1/5, you could do the following instead:

analogix_dp_start_crc(drm_crtc_index(crtc));


> +       } else if (!source_name)
> +
> +               ret = analogix_dp_stop_crc(connector);
> +       else
> +               ret = -EINVAL;
> +
> +       return ret;
> +}
> +
>  static const struct drm_crtc_funcs vop_crtc_funcs = {
>         .set_config = drm_atomic_helper_set_config,
>         .page_flip = drm_atomic_helper_page_flip,
> @@ -1112,6 +1159,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
>         .reset = vop_crtc_reset,
>         .atomic_duplicate_state = vop_crtc_duplicate_state,
>         .atomic_destroy_state = vop_crtc_destroy_state,
> +       .set_crc_source = vop_crtc_set_crc_source,
>  };
>
>  static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
> --
> 2.9.3
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 1/5] drm/dp: add connector backpointer to drm_dp_aux

[Tomeu Vizoso]

On 6 January 2017 at 16:56, Sean Paul <seanpaul@chromium.org> wrote:
> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> This backpointer allows DP helpers to access the connector it's being
>> used for.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>>  include/drm/drm_dp_helper.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index 55bbeb0ff594..4fa77b434594 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -721,6 +721,7 @@ struct drm_dp_aux_msg {
>>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
>>   * @ddc: I2C adapter that can be used for I2C-over-AUX communication
>>   * @dev: pointer to struct device that is the parent for this AUX channel
>> + * @connector: backpointer to connector that uses this AUX channel
>>   * @hw_mutex: internal mutex used for locking transfers
>>   * @transfer: transfers a message representing a single AUX transaction
>>   *
>> @@ -757,6 +758,7 @@ struct drm_dp_aux {
>>         const char *name;
>>         struct i2c_adapter ddc;
>>         struct device *dev;
>> +       struct drm_connector *connector;
>
> Perhaps I'm misreading the series, but it seems like you could instead
> add int crc_pipe along with the other crc fields. Then when you start
> the crc, set the pipe number. If you have the pipe in the crc worker,
> you can wait vblank on that pipe (no pointers needed).
>
> Reasonable?

I think that would work fine, but is it any better? I like that the
connector isn't going to change during the lifetime of the drm_dp_aux,
but crc_pipe could be changing between sampling sessions and any bugs
in keeping that field updated would be quite disconcerting and
cumbersome to debug.

crc_pipe's advantage is that we wouldn't need to update all callers of
drm_dp_aux_register, but I think it's better to have a connector field
that is mistakenly NULL, than a crc_pipe field with the wrong value.

Regards,

Tomeu

> Sean
>
>>         struct mutex hw_mutex;
>>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>>                             struct drm_dp_aux_msg *msg);
>> --
>> 2.9.3
>>
>
>
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS

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@chromium.org> wrote:
> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> 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@collabora.com>
>> ---
>>
>>  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

Re: [PATCH v3 5/5] drm/rockchip: Implement CRC debugfs API

[Tomeu Vizoso]

On 6 January 2017 at 16:56, Sean Paul <seanpaul@chromium.org> wrote:
> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> Implement the .set_crc_source() callback and call the DP helpers
>> accordingly to start and stop CRC capture.
>>
>> This is only done if this CRTC is currently using the eDP connector.
>>
>> v3: Remove superfluous check on rockchip_crtc_state->output_type
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 48 +++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index fb5f001f51c3..5e19bef6d5b4 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -19,6 +19,7 @@
>>  #include <drm/drm_crtc_helper.h>
>>  #include <drm/drm_flip_work.h>
>>  #include <drm/drm_plane_helper.h>
>> +#include <drm/bridge/analogix_dp.h>
>>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>> @@ -1105,6 +1106,52 @@ static void vop_crtc_destroy_state(struct drm_crtc *crtc,
>>         kfree(s);
>>  }
>>
>> +static struct drm_connector *vop_get_edp_connector(struct vop *vop)
>> +{
>> +       struct drm_crtc *crtc = &vop->crtc;
>> +       struct drm_connector *connector;
>> +
>> +       mutex_lock(&crtc->dev->mode_config.mutex);
>> +       drm_for_each_connector(connector, crtc->dev)
>> +               if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>> +                       mutex_unlock(&crtc->dev->mode_config.mutex);
>> +                       return connector;
>> +               }
>> +       mutex_unlock(&crtc->dev->mode_config.mutex);
>> +
>> +       return NULL;
>> +}
>> +
>> +static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
>> +                                  const char *source_name, size_t *values_cnt)
>> +{
>> +       struct vop *vop = to_vop(crtc);
>> +       struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc->state);
>> +       struct drm_connector *connector;
>> +       int ret;
>> +
>> +       connector = vop_get_edp_connector(vop);
>> +       if (!connector)
>> +               return -EINVAL;
>> +
>> +       *values_cnt = 3;
>> +
>> +       if (source_name &&
>> +           strcmp(source_name, "auto") == 0) {
>> +
>> +               if (s->output_type != DRM_MODE_CONNECTOR_eDP)
>> +                       return -EINVAL;
>> +
>> +               ret = analogix_dp_start_crc(connector);
>
> To pick up my thoughts from 1/5, you could do the following instead:
>
> analogix_dp_start_crc(drm_crtc_index(crtc));

Well, I still need the dpaux to access the dpcd, so that would have to be:

analogix_dp_start_crc(connector, drm_crtc_index(crtc));

Regards,

Tomeu

>> +       } else if (!source_name)
>> +
>> +               ret = analogix_dp_stop_crc(connector);
>> +       else
>> +               ret = -EINVAL;
>> +
>> +       return ret;
>> +}
>> +
>>  static const struct drm_crtc_funcs vop_crtc_funcs = {
>>         .set_config = drm_atomic_helper_set_config,
>>         .page_flip = drm_atomic_helper_page_flip,
>> @@ -1112,6 +1159,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
>>         .reset = vop_crtc_reset,
>>         .atomic_duplicate_state = vop_crtc_duplicate_state,
>>         .atomic_destroy_state = vop_crtc_destroy_state,
>> +       .set_crc_source = vop_crtc_set_crc_source,
>>  };
>>
>>  static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
>> --
>> 2.9.3
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH v3 1/5] drm/dp: add connector backpointer to drm_dp_aux

[Sean Paul]

On Mon, Jan 9, 2017 at 4:03 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> On 6 January 2017 at 16:56, Sean Paul <seanpaul@chromium.org> wrote:
>> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>> This backpointer allows DP helpers to access the connector it's being
>>> used for.
>>>
>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>> ---
>>>
>>>  include/drm/drm_dp_helper.h | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>>> index 55bbeb0ff594..4fa77b434594 100644
>>> --- a/include/drm/drm_dp_helper.h
>>> +++ b/include/drm/drm_dp_helper.h
>>> @@ -721,6 +721,7 @@ struct drm_dp_aux_msg {
>>>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
>>>   * @ddc: I2C adapter that can be used for I2C-over-AUX communication
>>>   * @dev: pointer to struct device that is the parent for this AUX channel
>>> + * @connector: backpointer to connector that uses this AUX channel
>>>   * @hw_mutex: internal mutex used for locking transfers
>>>   * @transfer: transfers a message representing a single AUX transaction
>>>   *
>>> @@ -757,6 +758,7 @@ struct drm_dp_aux {
>>>         const char *name;
>>>         struct i2c_adapter ddc;
>>>         struct device *dev;
>>> +       struct drm_connector *connector;
>>
>> Perhaps I'm misreading the series, but it seems like you could instead
>> add int crc_pipe along with the other crc fields. Then when you start
>> the crc, set the pipe number. If you have the pipe in the crc worker,
>> you can wait vblank on that pipe (no pointers needed).
>>
>> Reasonable?
>
> I think that would work fine, but is it any better? I like that the
> connector isn't going to change during the lifetime of the drm_dp_aux,

Is this actually guaranteed, though? I am having a hard time thinking
about a practical example where it's not implemented this way, but I
don't see anything actually enforcing the lifetime relationship.

> but crc_pipe could be changing between sampling sessions and any bugs
> in keeping that field updated would be quite disconcerting and
> cumbersome to debug.

Couldn't the same could be said for connector->state->crtc?

Sean

>
> crc_pipe's advantage is that we wouldn't need to update all callers of
> drm_dp_aux_register, but I think it's better to have a connector field
> that is mistakenly NULL, than a crc_pipe field with the wrong value.
>
> Regards,
>
> Tomeu
>
>> Sean
>>
>>>         struct mutex hw_mutex;
>>>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>>>                             struct drm_dp_aux_msg *msg);
>>> --
>>> 2.9.3
>>>
>>
>>
>>
>> --
>> Sean Paul, Software Engineer, Google / Chromium OS



-- 
Sean Paul, Software Engineer, Google / Chromium OS