linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/5] drm/dp: Implement CRC debugfs API
@ 2017-01-09 13:32 Tomeu Vizoso
  2017-01-09 13:32 ` [PATCH v4 1/5] drm/dp: add connector backpointer to drm_dp_aux Tomeu Vizoso
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Tomeu Vizoso @ 2017-01-09 13:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ville Syrjälä,
	Sean Paul, Thierry Reding, intel-gfx-trybot, Emil Velikov,
	Daniel Vetter, Benjamin Gaignard, Tomeu Vizoso, Mark Yao,
	Heiko Stuebner, Jani Nikula, dri-devel, linux-rockchip,
	David Airlie, Archit Taneja, linux-arm-kernel

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.

In this v4, "drm/dp: add helpers for capture of frame CRCs" has been
modified to address some of the comments from Sean Paul.

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                    | 129 +++++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  42 +++++++
 include/drm/bridge/analogix_dp.h                   |   3 +
 include/drm/drm_dp_helper.h                        |   9 ++
 5 files changed, 209 insertions(+), 8 deletions(-)

-- 
2.9.3

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v4 1/5] drm/dp: add connector backpointer to drm_dp_aux
  2017-01-09 13:32 [PATCH v4 0/5] drm/dp: Implement CRC debugfs API Tomeu Vizoso
@ 2017-01-09 13:32 ` Tomeu Vizoso
  2017-01-09 13:32 ` [PATCH v4 2/5] drm/bridge: analogix_dp: set connector " Tomeu Vizoso
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Tomeu Vizoso @ 2017-01-09 13:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ville Syrjälä,
	Sean Paul, Thierry Reding, intel-gfx-trybot, Emil Velikov,
	Daniel Vetter, Benjamin Gaignard, Tomeu Vizoso, Jani Nikula,
	dri-devel, David Airlie

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v4 2/5] drm/bridge: analogix_dp: set connector to drm_dp_aux
  2017-01-09 13:32 [PATCH v4 0/5] drm/dp: Implement CRC debugfs API Tomeu Vizoso
  2017-01-09 13:32 ` [PATCH v4 1/5] drm/dp: add connector backpointer to drm_dp_aux Tomeu Vizoso
@ 2017-01-09 13:32 ` Tomeu Vizoso
  2017-01-09 13:32 ` [PATCH v4 3/5] drm/dp: add helpers for capture of frame CRCs Tomeu Vizoso
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Tomeu Vizoso @ 2017-01-09 13:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ville Syrjälä,
	Sean Paul, Thierry Reding, intel-gfx-trybot, Emil Velikov,
	Daniel Vetter, Benjamin Gaignard, Tomeu Vizoso, Archit Taneja,
	David Airlie, dri-devel

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v4 3/5] drm/dp: add helpers for capture of frame CRCs
  2017-01-09 13:32 [PATCH v4 0/5] drm/dp: Implement CRC debugfs API Tomeu Vizoso
  2017-01-09 13:32 ` [PATCH v4 1/5] drm/dp: add connector backpointer to drm_dp_aux Tomeu Vizoso
  2017-01-09 13:32 ` [PATCH v4 2/5] drm/bridge: analogix_dp: set connector " Tomeu Vizoso
@ 2017-01-09 13:32 ` Tomeu Vizoso
  2017-01-09 16:45   ` Sean Paul
  2017-01-09 13:32 ` [PATCH v4 4/5] drm/bridge: analogix_dp: " Tomeu Vizoso
  2017-01-09 13:32 ` [PATCH v4 5/5] drm/rockchip: Implement CRC debugfs API Tomeu Vizoso
  4 siblings, 1 reply; 7+ messages in thread
From: Tomeu Vizoso @ 2017-01-09 13:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ville Syrjälä,
	Sean Paul, Thierry Reding, intel-gfx-trybot, Emil Velikov,
	Daniel Vetter, Benjamin Gaignard, Tomeu Vizoso, Jani Nikula,
	dri-devel, David Airlie

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.

v4: Style fixes (Sean Paul)
    Reworked retry of CRC reads (Sean Paul)
    Flush worker after stopping CRC generationa (Sean Paul)

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

 drivers/gpu/drm/drm_dp_helper.c | 129 ++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     |   7 +++
 2 files changed, 136 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 3e6fe82c6d64..f9018faf7e07 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -981,6 +981,83 @@ 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];
+	bool retry = false;
+	int ret;
+
+	if (WARN_ON(!aux->connector))
+		return;
+
+	crtc = aux->connector->state->crtc;
+	while (crtc->crc.opened) {
+		if (!retry) {
+			drm_crtc_wait_one_vblank(crtc);
+			if (!crtc->crc.opened)
+				break;
+		}
+
+		ret = drm_dp_aux_get_crc(aux, crc_bytes);
+		if (ret == -EAGAIN) {
+			if (retry)
+				DRM_DEBUG_KMS("Failed to get a CRC even 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);
+		}
+	}
+}
+
 /**
  * drm_dp_aux_init() - minimally initialise an aux channel
  * @aux: DisplayPort AUX channel
@@ -993,6 +1070,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 +1159,54 @@ 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;
+
+	flush_work(&aux->crc_work);
+
+	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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v4 4/5] drm/bridge: analogix_dp: add helpers for capture of frame CRCs
  2017-01-09 13:32 [PATCH v4 0/5] drm/dp: Implement CRC debugfs API Tomeu Vizoso
                   ` (2 preceding siblings ...)
  2017-01-09 13:32 ` [PATCH v4 3/5] drm/dp: add helpers for capture of frame CRCs Tomeu Vizoso
@ 2017-01-09 13:32 ` Tomeu Vizoso
  2017-01-09 13:32 ` [PATCH v4 5/5] drm/rockchip: Implement CRC debugfs API Tomeu Vizoso
  4 siblings, 0 replies; 7+ messages in thread
From: Tomeu Vizoso @ 2017-01-09 13:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ville Syrjälä,
	Sean Paul, Thierry Reding, intel-gfx-trybot, Emil Velikov,
	Daniel Vetter, Benjamin Gaignard, Tomeu Vizoso, Archit Taneja,
	David Airlie, dri-devel

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v4 5/5] drm/rockchip: Implement CRC debugfs API
  2017-01-09 13:32 [PATCH v4 0/5] drm/dp: Implement CRC debugfs API Tomeu Vizoso
                   ` (3 preceding siblings ...)
  2017-01-09 13:32 ` [PATCH v4 4/5] drm/bridge: analogix_dp: " Tomeu Vizoso
@ 2017-01-09 13:32 ` Tomeu Vizoso
  4 siblings, 0 replies; 7+ messages in thread
From: Tomeu Vizoso @ 2017-01-09 13:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ville Syrjälä,
	Sean Paul, Thierry Reding, intel-gfx-trybot, Emil Velikov,
	Daniel Vetter, Benjamin Gaignard, Tomeu Vizoso, Mark Yao,
	Heiko Stuebner, dri-devel, linux-rockchip, David Airlie,
	linux-arm-kernel

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 | 42 +++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fb5f001f51c3..6e5eb1aa182a 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,46 @@ 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)
+		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 +1153,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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 3/5] drm/dp: add helpers for capture of frame CRCs
  2017-01-09 13:32 ` [PATCH v4 3/5] drm/dp: add helpers for capture of frame CRCs Tomeu Vizoso
@ 2017-01-09 16:45   ` Sean Paul
  0 siblings, 0 replies; 7+ messages in thread
From: Sean Paul @ 2017-01-09 16:45 UTC (permalink / raw)
  To: Tomeu Vizoso
  Cc: Linux Kernel Mailing List, Ville Syrjälä,
	Thierry Reding, intel-gfx-trybot, Emil Velikov, Daniel Vetter,
	Benjamin Gaignard, Jani Nikula, dri-devel, David Airlie

On Mon, Jan 9, 2017 at 8:32 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.
>
> v4: Style fixes (Sean Paul)
>     Reworked retry of CRC reads (Sean Paul)
>     Flush worker after stopping CRC generationa (Sean Paul)
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
>  drivers/gpu/drm/drm_dp_helper.c | 129 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     |   7 +++
>  2 files changed, 136 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 3e6fe82c6d64..f9018faf7e07 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -981,6 +981,83 @@ 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];
> +       bool retry = false;
> +       int ret;
> +
> +       if (WARN_ON(!aux->connector))
> +               return;
> +
> +       crtc = aux->connector->state->crtc;
> +       while (crtc->crc.opened) {
> +               if (!retry) {
> +                       drm_crtc_wait_one_vblank(crtc);
> +                       if (!crtc->crc.opened)
> +                               break;
> +               }
> +
> +               ret = drm_dp_aux_get_crc(aux, crc_bytes);
> +               if (ret == -EAGAIN) {
> +                       if (retry)
> +                               DRM_DEBUG_KMS("Failed to get a CRC even after retrying: %d\n",
> +                                             ret);
> +                       retry = !retry;
> +                       usleep_range(1000, 2000);
> +                       continue;
> +               }
> +
> +               retry = false;

Yeah, the retry handling gets ugly fast, doesn't it? I think I'm
coming around to your original way of handing things (with the
addition of handling EAGAIN separately from other errors)

Sean

> +               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);
> +               }
> +       }
> +}
> +
>  /**
>   * drm_dp_aux_init() - minimally initialise an aux channel
>   * @aux: DisplayPort AUX channel
> @@ -993,6 +1070,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 +1159,54 @@ 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;
> +
> +       flush_work(&aux->crc_work);
> +
> +       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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-01-09 16:46 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-09 13:32 [PATCH v4 0/5] drm/dp: Implement CRC debugfs API Tomeu Vizoso
2017-01-09 13:32 ` [PATCH v4 1/5] drm/dp: add connector backpointer to drm_dp_aux Tomeu Vizoso
2017-01-09 13:32 ` [PATCH v4 2/5] drm/bridge: analogix_dp: set connector " Tomeu Vizoso
2017-01-09 13:32 ` [PATCH v4 3/5] drm/dp: add helpers for capture of frame CRCs Tomeu Vizoso
2017-01-09 16:45   ` Sean Paul
2017-01-09 13:32 ` [PATCH v4 4/5] drm/bridge: analogix_dp: " Tomeu Vizoso
2017-01-09 13:32 ` [PATCH v4 5/5] drm/rockchip: Implement CRC debugfs API Tomeu Vizoso

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).