Hi Kieran,
On Saturday, 28 April 2018 22:16:48 EEST Kieran Bingham wrote:
On 22/04/18 23:34, Laurent Pinchart wrote:
Implement CRC computation configuration and reporting through the DRM debugfs-based CRC API. The CRC source can be configured to any input plane or the pipeline output.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
I don't think I have any actual blocking questions here, so feel free to add a
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
I'll not be in distress if the CRC structures remain duplicated (although I see from your other mail you've considered defining the structure non-anonymously
Changes since v1:
- Format the source names using plane IDs instead of plane indices
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 156 ++++++++++++++++++++++++++-- drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 19 ++++ drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 7 ++ 3 files changed, 176 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index c4420538ec85..d71d709fe3d9 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
[snip]
@@ -710,15 +756,111 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc *crtc) rcrtc->vblank_enable = false; }
-static const struct drm_crtc_funcs crtc_funcs = {
- .reset = drm_atomic_helper_crtc_reset,
+static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
const char *source_name,size_t *values_cnt)+{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- struct drm_modeset_acquire_ctx ctx;
- struct drm_crtc_state *crtc_state;
- struct drm_atomic_state *state;
- enum vsp1_du_crc_source source;
- unsigned int index = 0;
- unsigned int i;
- int ret;
- /*
* Parse the source name. Supported values are "plane%u" to compute the* CRC on an input plane (%u is the plane ID), and "auto" to computethe
* CRC on the composer (VSP) output.*/- if (!source_name) {
source = VSP1_DU_CRC_NONE;- } else if (!strcmp(source_name, "auto")) {
source = VSP1_DU_CRC_OUTPUT;- } else if (strstarts(source_name, "plane")) {
source = VSP1_DU_CRC_PLANE;ret = kstrtouint(source_name + strlen("plane"), 10, &index);if (ret < 0)return ret;for (i = 0; i < rcrtc->vsp->num_planes; ++i) {if (index == rcrtc->vsp->planes[i].plane.base.id) {index = i;break;}}if (i >= rcrtc->vsp->num_planes)return -EINVAL;- } else {
return -EINVAL;- }
- *values_cnt = 1;
- /* Perform an atomic commit to set the CRC source. */
- drm_modeset_acquire_init(&ctx, 0);
- state = drm_atomic_state_alloc(crtc->dev);
- if (!state) {
ret = -ENOMEM;goto unlock;- }
- state->acquire_ctx = &ctx;
+retry:
- crtc_state = drm_atomic_get_crtc_state(state, crtc);
- if (!IS_ERR(crtc_state)) {
struct rcar_du_crtc_state *rcrtc_state;rcrtc_state = to_rcar_crtc_state(crtc_state);rcrtc_state->crc.source = source;rcrtc_state->crc.index = index;ret = drm_atomic_commit(state);Does this 'cost' a vblank ? (as in - does this action being performed from userspace have the capability to cause flicker, or loss of frame?)
It shouldn't cause flicker, but it could delay atomic commits queued by userspace by one frame. It's not ideal, but given that changing the CRC source requires a pipeline update on the VSP side, creating an atomic commit internally in the driver was the easiest solution.
Now that the BRU/BRS series has created infrastructure on the VSP side to perform pipeline reconfiguration we could also take advantage of that, but it would still incur a delay of one frame, so I don't think that would really help.
- } else {
ret = PTR_ERR(crtc_state);- }
- if (ret == -EDEADLK) {
drm_atomic_state_clear(state);drm_modeset_backoff(&ctx);goto retry;Not knowing what the -EDEADLK represents yet, this isn't an infinite loop opportunity is it ? (I assume the state_clear(),backoff() clean up and prevent that.)
-EDEADLK comes from the drm_atomic_get_crtc_state() or drm_atomic_commit() calls trying to lock a ww-mutex in a way that would cause a deadlock. In that case a backoff sequence is needed to release all locks that have been successfully taken, followed by a retry. The ww-mutex API should prevent infinite loops by ensuring fairness between the contenders.
- }
- drm_atomic_state_put(state);
+unlock:
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
- return 0;
+}
[snip]
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index fdc2bf99bda1..518ee2c60eb8 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h @@ -21,6 +21,8 @@ #include <drm/drmP.h> #include <drm/drm_crtc.h>
+#include <media/vsp1.h>
struct rcar_du_group; struct rcar_du_vsp;
@@ -69,6 +71,23 @@ struct rcar_du_crtc {
#define to_rcar_crtc(c) container_of(c, struct rcar_du_crtc, crtc)
+/**
- struct rcar_du_crtc_state - Driver-specific CRTC state
- @state: base DRM CRTC state
- @crc.source: source for CRC calculation
- @crc.index: index of the CRC source plane (when crc.source is set to
plane)
- */
+struct rcar_du_crtc_state {
- struct drm_crtc_state state;
- struct {
enum vsp1_du_crc_source source;unsigned int index;- } crc;
Another definition of this structure ... (is this the third?) do we need to replicate it each time ? (I know it's small ... but I love to keep things DRY)
I'll fix that as I've introduced a vsp1_du_crc_config structure in response to your review of patch 7/8.
+};
+#define to_rcar_crtc_state(s) container_of(s, struct rcar_du_crtc_state, state)
enum rcar_du_output { RCAR_DU_OUTPUT_DPAD0, RCAR_DU_OUTPUT_DPAD1,