[V2][PATCH 0/2] drm: xlnx: add some functions

[V2][PATCH 0/2] drm: xlnx: add some functions

[quanyang.wang]

From: Quanyang Wang <quanyang.wang@windriver.com>

Hi all,

The patch "drm: xlnx: add is_layer_vid() to simplify the code" is to
simplify the code which judge the layer type.

The patch "drm: xlnx: consolidate the functions which programming AUDIO_VIDEO_SELECT register"
is to consolidate the code that can configure vid/gfx/audio to output
different mode (live/mem/disable/tpg) in one function "zynqmp_disp_avbuf_output_select".

Changelogs:

 V1 ---> V2:
 - As per Paul's comments, add "const" to the argument "layer" of the
 function is_layer_vid, and just return the result of "==" operator, and
 add Acked-by from Paul. 
 - As per Paul's comments, fix some pattern errors and use FIELD_PREP()
 macro instead of *_SHIFT and use GENMASK/BIT to create *_MASK macros.

Thanks,
Quanyang


Quanyang Wang (2):
  drm: xlnx: add is_layer_vid() to simplify the code
  drm: xlnx: consolidate the functions which programming
    AUDIO_VIDEO_SELECT register

 drivers/gpu/drm/xlnx/zynqmp_disp.c      | 193 ++++++++++++++----------
 drivers/gpu/drm/xlnx/zynqmp_disp_regs.h |  23 +--
 2 files changed, 121 insertions(+), 95 deletions(-)

-- 
2.25.1

[V2][PATCH 1/2] drm: xlnx: add is_layer_vid() to simplify the code

[quanyang.wang]

From: Quanyang Wang <quanyang.wang@windriver.com>

Add a new function is_layer_vid() to simplify the code that
judges if a layer is the video layer.

Acked-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
---
 drivers/gpu/drm/xlnx/zynqmp_disp.c | 39 +++++++++++++++++-------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 109d627968ac..eefb278e24c6 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -434,30 +434,35 @@ static void zynqmp_disp_avbuf_write(struct zynqmp_disp_avbuf *avbuf,
 	writel(val, avbuf->base + reg);
 }
 
+static bool is_layer_vid(const struct zynqmp_disp_layer *layer)
+{
+	return layer->id == ZYNQMP_DISP_LAYER_VID;
+}
+
 /**
  * zynqmp_disp_avbuf_set_format - Set the input format for a layer
  * @avbuf: Audio/video buffer manager
- * @layer: The layer ID
+ * @layer: The layer
  * @fmt: The format information
  *
  * Set the video buffer manager format for @layer to @fmt.
  */
 static void zynqmp_disp_avbuf_set_format(struct zynqmp_disp_avbuf *avbuf,
-					 enum zynqmp_disp_layer_id layer,
+					 struct zynqmp_disp_layer *layer,
 					 const struct zynqmp_disp_format *fmt)
 {
 	unsigned int i;
 	u32 val;
 
 	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_FMT);
-	val &= layer == ZYNQMP_DISP_LAYER_VID
+	val &= is_layer_vid(layer)
 	    ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
 	    : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
 	val |= fmt->buf_fmt;
 	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_FMT, val);
 
 	for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; i++) {
-		unsigned int reg = layer == ZYNQMP_DISP_LAYER_VID
+		unsigned int reg = is_layer_vid(layer)
 				 ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
 				 : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
 
@@ -573,19 +578,19 @@ static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf *avbuf)
 /**
  * zynqmp_disp_avbuf_enable_video - Enable a video layer
  * @avbuf: Audio/video buffer manager
- * @layer: The layer ID
+ * @layer: The layer
  * @mode: Operating mode of layer
  *
  * Enable the video/graphics buffer for @layer.
  */
 static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf *avbuf,
-					   enum zynqmp_disp_layer_id layer,
+					   struct zynqmp_disp_layer *layer,
 					   enum zynqmp_disp_layer_mode mode)
 {
 	u32 val;
 
 	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
-	if (layer == ZYNQMP_DISP_LAYER_VID) {
+	if (is_layer_vid(layer)) {
 		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
 		if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
 			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM;
@@ -605,17 +610,17 @@ static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf *avbuf,
 /**
  * zynqmp_disp_avbuf_disable_video - Disable a video layer
  * @avbuf: Audio/video buffer manager
- * @layer: The layer ID
+ * @layer: The layer
  *
  * Disable the video/graphics buffer for @layer.
  */
 static void zynqmp_disp_avbuf_disable_video(struct zynqmp_disp_avbuf *avbuf,
-					    enum zynqmp_disp_layer_id layer)
+					    struct zynqmp_disp_layer *layer)
 {
 	u32 val;
 
 	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
-	if (layer == ZYNQMP_DISP_LAYER_VID) {
+	if (is_layer_vid(layer)) {
 		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
 		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE;
 	} else {
@@ -807,7 +812,7 @@ static void zynqmp_disp_blend_layer_set_csc(struct zynqmp_disp_blend *blend,
 		}
 	}
 
-	if (layer->id == ZYNQMP_DISP_LAYER_VID)
+	if (is_layer_vid(layer))
 		reg = ZYNQMP_DISP_V_BLEND_IN1CSC_COEFF(0);
 	else
 		reg = ZYNQMP_DISP_V_BLEND_IN2CSC_COEFF(0);
@@ -818,7 +823,7 @@ static void zynqmp_disp_blend_layer_set_csc(struct zynqmp_disp_blend *blend,
 		zynqmp_disp_blend_write(blend, reg + 8, coeffs[i + swap[2]]);
 	}
 
-	if (layer->id == ZYNQMP_DISP_LAYER_VID)
+	if (is_layer_vid(layer))
 		reg = ZYNQMP_DISP_V_BLEND_IN1CSC_OFFSET(0);
 	else
 		reg = ZYNQMP_DISP_V_BLEND_IN2CSC_OFFSET(0);
@@ -1025,7 +1030,7 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer *layer,
  */
 static void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
 {
-	zynqmp_disp_avbuf_enable_video(&layer->disp->avbuf, layer->id,
+	zynqmp_disp_avbuf_enable_video(&layer->disp->avbuf, layer,
 				       ZYNQMP_DISP_LAYER_NONLIVE);
 	zynqmp_disp_blend_layer_enable(&layer->disp->blend, layer);
 
@@ -1046,7 +1051,7 @@ static void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer)
 	for (i = 0; i < layer->drm_fmt->num_planes; i++)
 		dmaengine_terminate_sync(layer->dmas[i].chan);
 
-	zynqmp_disp_avbuf_disable_video(&layer->disp->avbuf, layer->id);
+	zynqmp_disp_avbuf_disable_video(&layer->disp->avbuf, layer);
 	zynqmp_disp_blend_layer_disable(&layer->disp->blend, layer);
 }
 
@@ -1067,7 +1072,7 @@ static void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
 	layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format);
 	layer->drm_fmt = info;
 
-	zynqmp_disp_avbuf_set_format(&layer->disp->avbuf, layer->id,
+	zynqmp_disp_avbuf_set_format(&layer->disp->avbuf, layer,
 				     layer->disp_fmt);
 
 	/*
@@ -1244,8 +1249,8 @@ static int zynqmp_disp_create_planes(struct zynqmp_disp *disp)
 			drm_formats[j] = layer->info->formats[j].drm_fmt;
 
 		/* Graphics layer is primary, and video layer is overlay. */
-		type = i == ZYNQMP_DISP_LAYER_GFX
-		     ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
+		type = is_layer_vid(layer)
+		     ? DRM_PLANE_TYPE_OVERLAY : DRM_PLANE_TYPE_PRIMARY;
 		ret = drm_universal_plane_init(disp->drm, &layer->plane, 0,
 					       &zynqmp_disp_plane_funcs,
 					       drm_formats,
-- 
2.25.1

[V2][PATCH 2/2] drm: xlnx: consolidate the functions which programming AUDIO_VIDEO_SELECT register

[quanyang.wang]

From: Quanyang Wang <quanyang.wang@windriver.com>

For now, the functions zynqmp_disp_avbuf_enable/disable_audio and
zynqmp_disp_avbuf_enable/disable_video are all programming the register
AV_BUF_OUTPUT_AUDIO_VIDEO_SELECT to select the output for audio or video.
And in the future, many drm properties (like video_tpg, audio_tpg,
audio_pl, etc) also need to access it. So let's introduce some variables
of enum type and consolidate the code to unify handling this.

Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
---
 drivers/gpu/drm/xlnx/zynqmp_disp.c      | 168 ++++++++++++++----------
 drivers/gpu/drm/xlnx/zynqmp_disp_regs.h |  23 +---
 2 files changed, 106 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index eefb278e24c6..3672d2f5665b 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -102,12 +102,39 @@ enum zynqmp_disp_layer_id {
 
 /**
  * enum zynqmp_disp_layer_mode - Layer mode
- * @ZYNQMP_DISP_LAYER_NONLIVE: non-live (memory) mode
+ * @ZYNQMP_DISP_LAYER_MEM: memory mode
  * @ZYNQMP_DISP_LAYER_LIVE: live (stream) mode
+ * @ZYNQMP_DISP_LAYER_TPG: tpg mode (only for video layer)
+ * @ZYNQMP_DISP_LAYER_DISABLE: disable mode
  */
 enum zynqmp_disp_layer_mode {
-	ZYNQMP_DISP_LAYER_NONLIVE,
-	ZYNQMP_DISP_LAYER_LIVE
+	ZYNQMP_DISP_LAYER_MEM,
+	ZYNQMP_DISP_LAYER_LIVE,
+	ZYNQMP_DISP_LAYER_TPG,
+	ZYNQMP_DISP_LAYER_DISABLE
+};
+
+enum avbuf_vid_mode {
+	VID_MODE_LIVE,
+	VID_MODE_MEM,
+	VID_MODE_TPG,
+	VID_MODE_NONE
+};
+
+enum avbuf_gfx_mode {
+	GFX_MODE_DISABLE,
+	GFX_MODE_MEM,
+	GFX_MODE_LIVE,
+	GFX_MODE_NONE
+};
+
+enum avbuf_aud_mode {
+	AUD1_MODE_LIVE,
+	AUD1_MODE_MEM,
+	AUD1_MODE_TPG,
+	AUD1_MODE_DISABLE,
+	AUD2_MODE_DISABLE,
+	AUD2_MODE_ENABLE
 };
 
 /**
@@ -542,92 +569,102 @@ static void zynqmp_disp_avbuf_disable_channels(struct zynqmp_disp_avbuf *avbuf)
 }
 
 /**
- * zynqmp_disp_avbuf_enable_audio - Enable audio
+ * zynqmp_disp_avbuf_output_select - Select the buffer manager outputs
  * @avbuf: Audio/video buffer manager
+ * @layer: The layer
+ * @mode: The mode for this layer
  *
- * Enable all audio buffers with a non-live (memory) source.
+ * Select the buffer manager outputs for @layer.
  */
-static void zynqmp_disp_avbuf_enable_audio(struct zynqmp_disp_avbuf *avbuf)
+static void zynqmp_disp_avbuf_output_select(struct zynqmp_disp_avbuf *avbuf,
+					   struct zynqmp_disp_layer *layer,
+					   u32 mode)
 {
-	u32 val;
+	u32 reg;
 
-	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
-	val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
-	val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MEM;
-	val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN;
-	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
+	reg = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
+
+	/* Select audio mode when the layer is NULL */
+	if (layer == NULL) {
+		if (mode >= AUD2_MODE_DISABLE) {
+			reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK;
+			reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK,
+					(mode - AUD2_MODE_DISABLE));
+		} else {
+			reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
+			reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK, mode);
+		}
+	} else if (is_layer_vid(layer)) {
+		reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
+		reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK, mode);
+	} else {
+		reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
+		reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK, mode);
+	}
+
+	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, reg);
 }
 
 /**
- * zynqmp_disp_avbuf_disable_audio - Disable audio
+ * zynqmp_disp_avbuf_enable_audio - Enable audio
  * @avbuf: Audio/video buffer manager
  *
- * Disable all audio buffers.
+ * Enable all audio buffers.
  */
-static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf *avbuf)
+static void zynqmp_disp_avbuf_enable_audio(struct zynqmp_disp_avbuf *avbuf)
 {
-	u32 val;
-
-	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
-	val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
-	val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_DISABLE;
-	val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN;
-	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
+	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD1_MODE_MEM);
+	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD2_MODE_ENABLE);
 }
 
 /**
- * zynqmp_disp_avbuf_enable_video - Enable a video layer
+ * zynqmp_disp_avbuf_disable_audio - Disable audio
  * @avbuf: Audio/video buffer manager
- * @layer: The layer
- * @mode: Operating mode of layer
  *
- * Enable the video/graphics buffer for @layer.
+ * Disable all audio buffers.
  */
-static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf *avbuf,
-					   struct zynqmp_disp_layer *layer,
-					   enum zynqmp_disp_layer_mode mode)
+static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf *avbuf)
 {
-	u32 val;
-
-	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
-	if (is_layer_vid(layer)) {
-		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
-		if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
-			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM;
-		else
-			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_LIVE;
-	} else {
-		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
-		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM;
-		if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
-			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM;
-		else
-			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_LIVE;
-	}
-	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
+	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD1_MODE_DISABLE);
+	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD2_MODE_DISABLE);
 }
 
 /**
- * zynqmp_disp_avbuf_disable_video - Disable a video layer
- * @avbuf: Audio/video buffer manager
+ * zynqmp_disp_avbuf_set_layer_output - Set layer output
  * @layer: The layer
+ * @mode: The layer mode
  *
- * Disable the video/graphics buffer for @layer.
+ * Set output for @layer
  */
-static void zynqmp_disp_avbuf_disable_video(struct zynqmp_disp_avbuf *avbuf,
-					    struct zynqmp_disp_layer *layer)
+static void zynqmp_disp_avbuf_set_layer_output(struct zynqmp_disp_layer *layer,
+					   enum zynqmp_disp_layer_mode mode)
 {
-	u32 val;
-
-	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
-	if (is_layer_vid(layer)) {
-		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
-		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE;
-	} else {
-		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
-		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_DISABLE;
+	struct zynqmp_disp *disp = layer->disp;
+	int val;
+
+	switch (mode) {
+	case ZYNQMP_DISP_LAYER_LIVE:
+		val = is_layer_vid(layer) ? VID_MODE_LIVE : GFX_MODE_LIVE;
+		break;
+	case ZYNQMP_DISP_LAYER_MEM:
+		val = is_layer_vid(layer) ? VID_MODE_MEM : GFX_MODE_MEM;
+		break;
+	case ZYNQMP_DISP_LAYER_TPG:
+		if (!is_layer_vid(layer)) {
+			dev_err(disp->dev, "gfx layer has no tpg mode\n");
+			return;
+		}
+		val = VID_MODE_TPG;
+		break;
+	case ZYNQMP_DISP_LAYER_DISABLE:
+		val = is_layer_vid(layer) ? VID_MODE_NONE : GFX_MODE_DISABLE;
+		break;
+	default:
+		dev_err(disp->dev, "invalid layer mode\n");
+		return;
 	}
-	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
+
+	zynqmp_disp_avbuf_output_select(&disp->avbuf, layer, val);
 }
 
 /**
@@ -1030,11 +1067,10 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer *layer,
  */
 static void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
 {
-	zynqmp_disp_avbuf_enable_video(&layer->disp->avbuf, layer,
-				       ZYNQMP_DISP_LAYER_NONLIVE);
+	zynqmp_disp_avbuf_set_layer_output(layer, ZYNQMP_DISP_LAYER_MEM);
 	zynqmp_disp_blend_layer_enable(&layer->disp->blend, layer);
 
-	layer->mode = ZYNQMP_DISP_LAYER_NONLIVE;
+	layer->mode = ZYNQMP_DISP_LAYER_MEM;
 }
 
 /**
@@ -1051,7 +1087,7 @@ static void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer)
 	for (i = 0; i < layer->drm_fmt->num_planes; i++)
 		dmaengine_terminate_sync(layer->dmas[i].chan);
 
-	zynqmp_disp_avbuf_disable_video(&layer->disp->avbuf, layer);
+	zynqmp_disp_avbuf_set_layer_output(layer, ZYNQMP_DISP_LAYER_DISABLE);
 	zynqmp_disp_blend_layer_disable(&layer->disp->blend, layer);
 }
 
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
index f92a006d5070..4316e324102d 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
@@ -118,25 +118,10 @@
 #define ZYNQMP_DISP_AV_BUF_STC_SNAPSHOT0		0x60
 #define ZYNQMP_DISP_AV_BUF_STC_SNAPSHOT1		0x64
 #define ZYNQMP_DISP_AV_BUF_OUTPUT			0x70
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_SHIFT		0
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK		(0x3 << 0)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_LIVE		(0 << 0)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM		(1 << 0)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_PATTERN		(2 << 0)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE		(3 << 0)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_SHIFT		2
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK		(0x3 << 2)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_DISABLE		(0 << 2)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM		(1 << 2)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_LIVE		(2 << 2)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_NONE		(3 << 2)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_SHIFT		4
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK		(0x3 << 4)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_PL		(0 << 4)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MEM		(1 << 4)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_PATTERN		(2 << 4)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_DISABLE		(3 << 4)
-#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN		BIT(6)
+#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK		GENMASK(1, 0)
+#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK		GENMASK(3, 2)
+#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK		GENMASK(5, 4)
+#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK		BIT(6)
 #define ZYNQMP_DISP_AV_BUF_HCOUNT_VCOUNT_INT0		0x74
 #define ZYNQMP_DISP_AV_BUF_HCOUNT_VCOUNT_INT1		0x78
 #define ZYNQMP_DISP_AV_BUF_PATTERN_GEN_SELECT		0x100
-- 
2.25.1

Re: [V2][PATCH 2/2] drm: xlnx: consolidate the functions which programming AUDIO_VIDEO_SELECT register

[Paul Cercueil]

Hi,

Le mar., mai 18 2021 at 17:50:19 +0800, quanyang.wang@windriver.com a 
écrit :
> From: Quanyang Wang <quanyang.wang@windriver.com>
> 
> For now, the functions zynqmp_disp_avbuf_enable/disable_audio and
> zynqmp_disp_avbuf_enable/disable_video are all programming the 
> register
> AV_BUF_OUTPUT_AUDIO_VIDEO_SELECT to select the output for audio or 
> video.
> And in the future, many drm properties (like video_tpg, audio_tpg,
> audio_pl, etc) also need to access it. So let's introduce some 
> variables
> of enum type and consolidate the code to unify handling this.
> 
> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>

Acked-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul


> ---
>  drivers/gpu/drm/xlnx/zynqmp_disp.c      | 168 
> ++++++++++++++----------
>  drivers/gpu/drm/xlnx/zynqmp_disp_regs.h |  23 +---
>  2 files changed, 106 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c 
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index eefb278e24c6..3672d2f5665b 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -102,12 +102,39 @@ enum zynqmp_disp_layer_id {
> 
>  /**
>   * enum zynqmp_disp_layer_mode - Layer mode
> - * @ZYNQMP_DISP_LAYER_NONLIVE: non-live (memory) mode
> + * @ZYNQMP_DISP_LAYER_MEM: memory mode
>   * @ZYNQMP_DISP_LAYER_LIVE: live (stream) mode
> + * @ZYNQMP_DISP_LAYER_TPG: tpg mode (only for video layer)
> + * @ZYNQMP_DISP_LAYER_DISABLE: disable mode
>   */
>  enum zynqmp_disp_layer_mode {
> -	ZYNQMP_DISP_LAYER_NONLIVE,
> -	ZYNQMP_DISP_LAYER_LIVE
> +	ZYNQMP_DISP_LAYER_MEM,
> +	ZYNQMP_DISP_LAYER_LIVE,
> +	ZYNQMP_DISP_LAYER_TPG,
> +	ZYNQMP_DISP_LAYER_DISABLE
> +};
> +
> +enum avbuf_vid_mode {
> +	VID_MODE_LIVE,
> +	VID_MODE_MEM,
> +	VID_MODE_TPG,
> +	VID_MODE_NONE
> +};
> +
> +enum avbuf_gfx_mode {
> +	GFX_MODE_DISABLE,
> +	GFX_MODE_MEM,
> +	GFX_MODE_LIVE,
> +	GFX_MODE_NONE
> +};
> +
> +enum avbuf_aud_mode {
> +	AUD1_MODE_LIVE,
> +	AUD1_MODE_MEM,
> +	AUD1_MODE_TPG,
> +	AUD1_MODE_DISABLE,
> +	AUD2_MODE_DISABLE,
> +	AUD2_MODE_ENABLE
>  };
> 
>  /**
> @@ -542,92 +569,102 @@ static void 
> zynqmp_disp_avbuf_disable_channels(struct zynqmp_disp_avbuf *avbuf)
>  }
> 
>  /**
> - * zynqmp_disp_avbuf_enable_audio - Enable audio
> + * zynqmp_disp_avbuf_output_select - Select the buffer manager 
> outputs
>   * @avbuf: Audio/video buffer manager
> + * @layer: The layer
> + * @mode: The mode for this layer
>   *
> - * Enable all audio buffers with a non-live (memory) source.
> + * Select the buffer manager outputs for @layer.
>   */
> -static void zynqmp_disp_avbuf_enable_audio(struct zynqmp_disp_avbuf 
> *avbuf)
> +static void zynqmp_disp_avbuf_output_select(struct zynqmp_disp_avbuf 
> *avbuf,
> +					   struct zynqmp_disp_layer *layer,
> +					   u32 mode)
>  {
> -	u32 val;
> +	u32 reg;
> 
> -	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> -	val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
> -	val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MEM;
> -	val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN;
> -	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
> +	reg = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> +
> +	/* Select audio mode when the layer is NULL */
> +	if (layer == NULL) {
> +		if (mode >= AUD2_MODE_DISABLE) {
> +			reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK;
> +			reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK,
> +					(mode - AUD2_MODE_DISABLE));
> +		} else {
> +			reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
> +			reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK, mode);
> +		}
> +	} else if (is_layer_vid(layer)) {
> +		reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
> +		reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK, mode);
> +	} else {
> +		reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
> +		reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK, mode);
> +	}
> +
> +	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, reg);
>  }
> 
>  /**
> - * zynqmp_disp_avbuf_disable_audio - Disable audio
> + * zynqmp_disp_avbuf_enable_audio - Enable audio
>   * @avbuf: Audio/video buffer manager
>   *
> - * Disable all audio buffers.
> + * Enable all audio buffers.
>   */
> -static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf 
> *avbuf)
> +static void zynqmp_disp_avbuf_enable_audio(struct zynqmp_disp_avbuf 
> *avbuf)
>  {
> -	u32 val;
> -
> -	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> -	val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
> -	val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_DISABLE;
> -	val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN;
> -	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
> +	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD1_MODE_MEM);
> +	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD2_MODE_ENABLE);
>  }
> 
>  /**
> - * zynqmp_disp_avbuf_enable_video - Enable a video layer
> + * zynqmp_disp_avbuf_disable_audio - Disable audio
>   * @avbuf: Audio/video buffer manager
> - * @layer: The layer
> - * @mode: Operating mode of layer
>   *
> - * Enable the video/graphics buffer for @layer.
> + * Disable all audio buffers.
>   */
> -static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf 
> *avbuf,
> -					   struct zynqmp_disp_layer *layer,
> -					   enum zynqmp_disp_layer_mode mode)
> +static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf 
> *avbuf)
>  {
> -	u32 val;
> -
> -	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> -	if (is_layer_vid(layer)) {
> -		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
> -		if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
> -			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM;
> -		else
> -			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_LIVE;
> -	} else {
> -		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
> -		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM;
> -		if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
> -			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM;
> -		else
> -			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_LIVE;
> -	}
> -	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
> +	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD1_MODE_DISABLE);
> +	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD2_MODE_DISABLE);
>  }
> 
>  /**
> - * zynqmp_disp_avbuf_disable_video - Disable a video layer
> - * @avbuf: Audio/video buffer manager
> + * zynqmp_disp_avbuf_set_layer_output - Set layer output
>   * @layer: The layer
> + * @mode: The layer mode
>   *
> - * Disable the video/graphics buffer for @layer.
> + * Set output for @layer
>   */
> -static void zynqmp_disp_avbuf_disable_video(struct zynqmp_disp_avbuf 
> *avbuf,
> -					    struct zynqmp_disp_layer *layer)
> +static void zynqmp_disp_avbuf_set_layer_output(struct 
> zynqmp_disp_layer *layer,
> +					   enum zynqmp_disp_layer_mode mode)
>  {
> -	u32 val;
> -
> -	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> -	if (is_layer_vid(layer)) {
> -		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
> -		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE;
> -	} else {
> -		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
> -		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_DISABLE;
> +	struct zynqmp_disp *disp = layer->disp;
> +	int val;
> +
> +	switch (mode) {
> +	case ZYNQMP_DISP_LAYER_LIVE:
> +		val = is_layer_vid(layer) ? VID_MODE_LIVE : GFX_MODE_LIVE;
> +		break;
> +	case ZYNQMP_DISP_LAYER_MEM:
> +		val = is_layer_vid(layer) ? VID_MODE_MEM : GFX_MODE_MEM;
> +		break;
> +	case ZYNQMP_DISP_LAYER_TPG:
> +		if (!is_layer_vid(layer)) {
> +			dev_err(disp->dev, "gfx layer has no tpg mode\n");
> +			return;
> +		}
> +		val = VID_MODE_TPG;
> +		break;
> +	case ZYNQMP_DISP_LAYER_DISABLE:
> +		val = is_layer_vid(layer) ? VID_MODE_NONE : GFX_MODE_DISABLE;
> +		break;
> +	default:
> +		dev_err(disp->dev, "invalid layer mode\n");
> +		return;
>  	}
> -	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
> +
> +	zynqmp_disp_avbuf_output_select(&disp->avbuf, layer, val);
>  }
> 
>  /**
> @@ -1030,11 +1067,10 @@ zynqmp_disp_layer_find_format(struct 
> zynqmp_disp_layer *layer,
>   */
>  static void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
>  {
> -	zynqmp_disp_avbuf_enable_video(&layer->disp->avbuf, layer,
> -				       ZYNQMP_DISP_LAYER_NONLIVE);
> +	zynqmp_disp_avbuf_set_layer_output(layer, ZYNQMP_DISP_LAYER_MEM);
>  	zynqmp_disp_blend_layer_enable(&layer->disp->blend, layer);
> 
> -	layer->mode = ZYNQMP_DISP_LAYER_NONLIVE;
> +	layer->mode = ZYNQMP_DISP_LAYER_MEM;
>  }
> 
>  /**
> @@ -1051,7 +1087,7 @@ static void zynqmp_disp_layer_disable(struct 
> zynqmp_disp_layer *layer)
>  	for (i = 0; i < layer->drm_fmt->num_planes; i++)
>  		dmaengine_terminate_sync(layer->dmas[i].chan);
> 
> -	zynqmp_disp_avbuf_disable_video(&layer->disp->avbuf, layer);
> +	zynqmp_disp_avbuf_set_layer_output(layer, 
> ZYNQMP_DISP_LAYER_DISABLE);
>  	zynqmp_disp_blend_layer_disable(&layer->disp->blend, layer);
>  }
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h 
> b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> index f92a006d5070..4316e324102d 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> @@ -118,25 +118,10 @@
>  #define ZYNQMP_DISP_AV_BUF_STC_SNAPSHOT0		0x60
>  #define ZYNQMP_DISP_AV_BUF_STC_SNAPSHOT1		0x64
>  #define ZYNQMP_DISP_AV_BUF_OUTPUT			0x70
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_SHIFT		0
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK		(0x3 << 0)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_LIVE		(0 << 0)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM		(1 << 0)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_PATTERN		(2 << 0)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE		(3 << 0)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_SHIFT		2
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK		(0x3 << 2)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_DISABLE		(0 << 2)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM		(1 << 2)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_LIVE		(2 << 2)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_NONE		(3 << 2)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_SHIFT		4
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK		(0x3 << 4)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_PL		(0 << 4)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MEM		(1 << 4)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_PATTERN		(2 << 4)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_DISABLE		(3 << 4)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN		BIT(6)
> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK		GENMASK(1, 0)
> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK		GENMASK(3, 2)
> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK		GENMASK(5, 4)
> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK		BIT(6)
>  #define ZYNQMP_DISP_AV_BUF_HCOUNT_VCOUNT_INT0		0x74
>  #define ZYNQMP_DISP_AV_BUF_HCOUNT_VCOUNT_INT1		0x78
>  #define ZYNQMP_DISP_AV_BUF_PATTERN_GEN_SELECT		0x100
> --
> 2.25.1
> 

Re: [V2][PATCH 1/2] drm: xlnx: add is_layer_vid() to simplify the code

[Laurent Pinchart]

Hi Quanyang,

Thank you for the patch.

On Tue, May 18, 2021 at 05:50:18PM +0800, quanyang.wang@windriver.com wrote:
> From: Quanyang Wang <quanyang.wang@windriver.com>
> 
> Add a new function is_layer_vid() to simplify the code that
> judges if a layer is the video layer.

I like this, and especially passing the layer pointer to functions
instead of the layer ID. I'm however wondering is we shouldn't name the
function xlnx_zynqmp_disp_layer_is_video(), for consistency. If that's
fine with you I can make the change when applying the patch to my tree,
there's no need to submit a new version.

> Acked-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/xlnx/zynqmp_disp.c | 39 +++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 109d627968ac..eefb278e24c6 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -434,30 +434,35 @@ static void zynqmp_disp_avbuf_write(struct zynqmp_disp_avbuf *avbuf,
>  	writel(val, avbuf->base + reg);
>  }
>  
> +static bool is_layer_vid(const struct zynqmp_disp_layer *layer)
> +{
> +	return layer->id == ZYNQMP_DISP_LAYER_VID;
> +}
> +
>  /**
>   * zynqmp_disp_avbuf_set_format - Set the input format for a layer
>   * @avbuf: Audio/video buffer manager
> - * @layer: The layer ID
> + * @layer: The layer
>   * @fmt: The format information
>   *
>   * Set the video buffer manager format for @layer to @fmt.
>   */
>  static void zynqmp_disp_avbuf_set_format(struct zynqmp_disp_avbuf *avbuf,
> -					 enum zynqmp_disp_layer_id layer,
> +					 struct zynqmp_disp_layer *layer,
>  					 const struct zynqmp_disp_format *fmt)
>  {
>  	unsigned int i;
>  	u32 val;
>  
>  	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_FMT);
> -	val &= layer == ZYNQMP_DISP_LAYER_VID
> +	val &= is_layer_vid(layer)
>  	    ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
>  	    : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
>  	val |= fmt->buf_fmt;
>  	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_FMT, val);
>  
>  	for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; i++) {
> -		unsigned int reg = layer == ZYNQMP_DISP_LAYER_VID
> +		unsigned int reg = is_layer_vid(layer)
>  				 ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
>  				 : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
>  
> @@ -573,19 +578,19 @@ static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf *avbuf)
>  /**
>   * zynqmp_disp_avbuf_enable_video - Enable a video layer
>   * @avbuf: Audio/video buffer manager
> - * @layer: The layer ID
> + * @layer: The layer
>   * @mode: Operating mode of layer
>   *
>   * Enable the video/graphics buffer for @layer.
>   */
>  static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf *avbuf,
> -					   enum zynqmp_disp_layer_id layer,
> +					   struct zynqmp_disp_layer *layer,
>  					   enum zynqmp_disp_layer_mode mode)
>  {
>  	u32 val;
>  
>  	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> -	if (layer == ZYNQMP_DISP_LAYER_VID) {
> +	if (is_layer_vid(layer)) {
>  		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
>  		if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
>  			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM;
> @@ -605,17 +610,17 @@ static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf *avbuf,
>  /**
>   * zynqmp_disp_avbuf_disable_video - Disable a video layer
>   * @avbuf: Audio/video buffer manager
> - * @layer: The layer ID
> + * @layer: The layer
>   *
>   * Disable the video/graphics buffer for @layer.
>   */
>  static void zynqmp_disp_avbuf_disable_video(struct zynqmp_disp_avbuf *avbuf,
> -					    enum zynqmp_disp_layer_id layer)
> +					    struct zynqmp_disp_layer *layer)
>  {
>  	u32 val;
>  
>  	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> -	if (layer == ZYNQMP_DISP_LAYER_VID) {
> +	if (is_layer_vid(layer)) {
>  		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
>  		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE;
>  	} else {
> @@ -807,7 +812,7 @@ static void zynqmp_disp_blend_layer_set_csc(struct zynqmp_disp_blend *blend,
>  		}
>  	}
>  
> -	if (layer->id == ZYNQMP_DISP_LAYER_VID)
> +	if (is_layer_vid(layer))
>  		reg = ZYNQMP_DISP_V_BLEND_IN1CSC_COEFF(0);
>  	else
>  		reg = ZYNQMP_DISP_V_BLEND_IN2CSC_COEFF(0);
> @@ -818,7 +823,7 @@ static void zynqmp_disp_blend_layer_set_csc(struct zynqmp_disp_blend *blend,
>  		zynqmp_disp_blend_write(blend, reg + 8, coeffs[i + swap[2]]);
>  	}
>  
> -	if (layer->id == ZYNQMP_DISP_LAYER_VID)
> +	if (is_layer_vid(layer))
>  		reg = ZYNQMP_DISP_V_BLEND_IN1CSC_OFFSET(0);
>  	else
>  		reg = ZYNQMP_DISP_V_BLEND_IN2CSC_OFFSET(0);
> @@ -1025,7 +1030,7 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer *layer,
>   */
>  static void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
>  {
> -	zynqmp_disp_avbuf_enable_video(&layer->disp->avbuf, layer->id,
> +	zynqmp_disp_avbuf_enable_video(&layer->disp->avbuf, layer,
>  				       ZYNQMP_DISP_LAYER_NONLIVE);
>  	zynqmp_disp_blend_layer_enable(&layer->disp->blend, layer);
>  
> @@ -1046,7 +1051,7 @@ static void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer)
>  	for (i = 0; i < layer->drm_fmt->num_planes; i++)
>  		dmaengine_terminate_sync(layer->dmas[i].chan);
>  
> -	zynqmp_disp_avbuf_disable_video(&layer->disp->avbuf, layer->id);
> +	zynqmp_disp_avbuf_disable_video(&layer->disp->avbuf, layer);
>  	zynqmp_disp_blend_layer_disable(&layer->disp->blend, layer);
>  }
>  
> @@ -1067,7 +1072,7 @@ static void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
>  	layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format);
>  	layer->drm_fmt = info;
>  
> -	zynqmp_disp_avbuf_set_format(&layer->disp->avbuf, layer->id,
> +	zynqmp_disp_avbuf_set_format(&layer->disp->avbuf, layer,
>  				     layer->disp_fmt);
>  
>  	/*
> @@ -1244,8 +1249,8 @@ static int zynqmp_disp_create_planes(struct zynqmp_disp *disp)
>  			drm_formats[j] = layer->info->formats[j].drm_fmt;
>  
>  		/* Graphics layer is primary, and video layer is overlay. */
> -		type = i == ZYNQMP_DISP_LAYER_GFX
> -		     ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
> +		type = is_layer_vid(layer)
> +		     ? DRM_PLANE_TYPE_OVERLAY : DRM_PLANE_TYPE_PRIMARY;
>  		ret = drm_universal_plane_init(disp->drm, &layer->plane, 0,
>  					       &zynqmp_disp_plane_funcs,
>  					       drm_formats,

-- 
Regards,

Laurent Pinchart

Re: [V2][PATCH 1/2] drm: xlnx: add is_layer_vid() to simplify the code

[quanyang.wang]

Hi Laurent,

Thank you for your review.

On 5/20/21 5:37 PM, Laurent Pinchart wrote:
> Hi Quanyang,
> 
> Thank you for the patch.
> 
> On Tue, May 18, 2021 at 05:50:18PM +0800, quanyang.wang@windriver.com wrote:
>> From: Quanyang Wang <quanyang.wang@windriver.com>
>>
>> Add a new function is_layer_vid() to simplify the code that
>> judges if a layer is the video layer.
> 
> I like this, and especially passing the layer pointer to functions
> instead of the layer ID. I'm however wondering is we shouldn't name the
> function xlnx_zynqmp_disp_layer_is_video(), for consistency. If that's
> fine with you I can make the change when applying the patch to my tree,
> there's no need to submit a new version.
That's fine. Please help change the function name to 
xlnx_zynqmp_disp_layer_is_video().

Thanks,
Quanyang
> 
>> Acked-by: Paul Cercueil <paul@crapouillou.net>
>> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> ---
>>   drivers/gpu/drm/xlnx/zynqmp_disp.c | 39 +++++++++++++++++-------------
>>   1 file changed, 22 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> index 109d627968ac..eefb278e24c6 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> @@ -434,30 +434,35 @@ static void zynqmp_disp_avbuf_write(struct zynqmp_disp_avbuf *avbuf,
>>   	writel(val, avbuf->base + reg);
>>   }
>>   
>> +static bool is_layer_vid(const struct zynqmp_disp_layer *layer)
>> +{
>> +	return layer->id == ZYNQMP_DISP_LAYER_VID;
>> +}
>> +
>>   /**
>>    * zynqmp_disp_avbuf_set_format - Set the input format for a layer
>>    * @avbuf: Audio/video buffer manager
>> - * @layer: The layer ID
>> + * @layer: The layer
>>    * @fmt: The format information
>>    *
>>    * Set the video buffer manager format for @layer to @fmt.
>>    */
>>   static void zynqmp_disp_avbuf_set_format(struct zynqmp_disp_avbuf *avbuf,
>> -					 enum zynqmp_disp_layer_id layer,
>> +					 struct zynqmp_disp_layer *layer,
>>   					 const struct zynqmp_disp_format *fmt)
>>   {
>>   	unsigned int i;
>>   	u32 val;
>>   
>>   	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_FMT);
>> -	val &= layer == ZYNQMP_DISP_LAYER_VID
>> +	val &= is_layer_vid(layer)
>>   	    ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
>>   	    : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
>>   	val |= fmt->buf_fmt;
>>   	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_FMT, val);
>>   
>>   	for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; i++) {
>> -		unsigned int reg = layer == ZYNQMP_DISP_LAYER_VID
>> +		unsigned int reg = is_layer_vid(layer)
>>   				 ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
>>   				 : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
>>   
>> @@ -573,19 +578,19 @@ static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf *avbuf)
>>   /**
>>    * zynqmp_disp_avbuf_enable_video - Enable a video layer
>>    * @avbuf: Audio/video buffer manager
>> - * @layer: The layer ID
>> + * @layer: The layer
>>    * @mode: Operating mode of layer
>>    *
>>    * Enable the video/graphics buffer for @layer.
>>    */
>>   static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf *avbuf,
>> -					   enum zynqmp_disp_layer_id layer,
>> +					   struct zynqmp_disp_layer *layer,
>>   					   enum zynqmp_disp_layer_mode mode)
>>   {
>>   	u32 val;
>>   
>>   	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
>> -	if (layer == ZYNQMP_DISP_LAYER_VID) {
>> +	if (is_layer_vid(layer)) {
>>   		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
>>   		if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
>>   			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM;
>> @@ -605,17 +610,17 @@ static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf *avbuf,
>>   /**
>>    * zynqmp_disp_avbuf_disable_video - Disable a video layer
>>    * @avbuf: Audio/video buffer manager
>> - * @layer: The layer ID
>> + * @layer: The layer
>>    *
>>    * Disable the video/graphics buffer for @layer.
>>    */
>>   static void zynqmp_disp_avbuf_disable_video(struct zynqmp_disp_avbuf *avbuf,
>> -					    enum zynqmp_disp_layer_id layer)
>> +					    struct zynqmp_disp_layer *layer)
>>   {
>>   	u32 val;
>>   
>>   	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
>> -	if (layer == ZYNQMP_DISP_LAYER_VID) {
>> +	if (is_layer_vid(layer)) {
>>   		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
>>   		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE;
>>   	} else {
>> @@ -807,7 +812,7 @@ static void zynqmp_disp_blend_layer_set_csc(struct zynqmp_disp_blend *blend,
>>   		}
>>   	}
>>   
>> -	if (layer->id == ZYNQMP_DISP_LAYER_VID)
>> +	if (is_layer_vid(layer))
>>   		reg = ZYNQMP_DISP_V_BLEND_IN1CSC_COEFF(0);
>>   	else
>>   		reg = ZYNQMP_DISP_V_BLEND_IN2CSC_COEFF(0);
>> @@ -818,7 +823,7 @@ static void zynqmp_disp_blend_layer_set_csc(struct zynqmp_disp_blend *blend,
>>   		zynqmp_disp_blend_write(blend, reg + 8, coeffs[i + swap[2]]);
>>   	}
>>   
>> -	if (layer->id == ZYNQMP_DISP_LAYER_VID)
>> +	if (is_layer_vid(layer))
>>   		reg = ZYNQMP_DISP_V_BLEND_IN1CSC_OFFSET(0);
>>   	else
>>   		reg = ZYNQMP_DISP_V_BLEND_IN2CSC_OFFSET(0);
>> @@ -1025,7 +1030,7 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer *layer,
>>    */
>>   static void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
>>   {
>> -	zynqmp_disp_avbuf_enable_video(&layer->disp->avbuf, layer->id,
>> +	zynqmp_disp_avbuf_enable_video(&layer->disp->avbuf, layer,
>>   				       ZYNQMP_DISP_LAYER_NONLIVE);
>>   	zynqmp_disp_blend_layer_enable(&layer->disp->blend, layer);
>>   
>> @@ -1046,7 +1051,7 @@ static void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer)
>>   	for (i = 0; i < layer->drm_fmt->num_planes; i++)
>>   		dmaengine_terminate_sync(layer->dmas[i].chan);
>>   
>> -	zynqmp_disp_avbuf_disable_video(&layer->disp->avbuf, layer->id);
>> +	zynqmp_disp_avbuf_disable_video(&layer->disp->avbuf, layer);
>>   	zynqmp_disp_blend_layer_disable(&layer->disp->blend, layer);
>>   }
>>   
>> @@ -1067,7 +1072,7 @@ static void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
>>   	layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format);
>>   	layer->drm_fmt = info;
>>   
>> -	zynqmp_disp_avbuf_set_format(&layer->disp->avbuf, layer->id,
>> +	zynqmp_disp_avbuf_set_format(&layer->disp->avbuf, layer,
>>   				     layer->disp_fmt);
>>   
>>   	/*
>> @@ -1244,8 +1249,8 @@ static int zynqmp_disp_create_planes(struct zynqmp_disp *disp)
>>   			drm_formats[j] = layer->info->formats[j].drm_fmt;
>>   
>>   		/* Graphics layer is primary, and video layer is overlay. */
>> -		type = i == ZYNQMP_DISP_LAYER_GFX
>> -		     ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
>> +		type = is_layer_vid(layer)
>> +		     ? DRM_PLANE_TYPE_OVERLAY : DRM_PLANE_TYPE_PRIMARY;
>>   		ret = drm_universal_plane_init(disp->drm, &layer->plane, 0,
>>   					       &zynqmp_disp_plane_funcs,
>>   					       drm_formats,
> 

Re: [V2][PATCH 2/2] drm: xlnx: consolidate the functions which programming AUDIO_VIDEO_SELECT register

[Laurent Pinchart]

Hi Quanyang,

Thank you for the patch.

On Tue, May 18, 2021 at 05:50:19PM +0800, quanyang.wang@windriver.com wrote:
> From: Quanyang Wang <quanyang.wang@windriver.com>
> 
> For now, the functions zynqmp_disp_avbuf_enable/disable_audio and
> zynqmp_disp_avbuf_enable/disable_video are all programming the register
> AV_BUF_OUTPUT_AUDIO_VIDEO_SELECT to select the output for audio or video.
> And in the future, many drm properties (like video_tpg, audio_tpg,
> audio_pl, etc) also need to access it. So let's introduce some variables
> of enum type and consolidate the code to unify handling this.
> 
> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
> ---
>  drivers/gpu/drm/xlnx/zynqmp_disp.c      | 168 ++++++++++++++----------
>  drivers/gpu/drm/xlnx/zynqmp_disp_regs.h |  23 +---
>  2 files changed, 106 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index eefb278e24c6..3672d2f5665b 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -102,12 +102,39 @@ enum zynqmp_disp_layer_id {
>  
>  /**
>   * enum zynqmp_disp_layer_mode - Layer mode
> - * @ZYNQMP_DISP_LAYER_NONLIVE: non-live (memory) mode
> + * @ZYNQMP_DISP_LAYER_MEM: memory mode
>   * @ZYNQMP_DISP_LAYER_LIVE: live (stream) mode
> + * @ZYNQMP_DISP_LAYER_TPG: tpg mode (only for video layer)
> + * @ZYNQMP_DISP_LAYER_DISABLE: disable mode

"Disable" isn't really a mode :-S

>   */
>  enum zynqmp_disp_layer_mode {
> -	ZYNQMP_DISP_LAYER_NONLIVE,
> -	ZYNQMP_DISP_LAYER_LIVE
> +	ZYNQMP_DISP_LAYER_MEM,
> +	ZYNQMP_DISP_LAYER_LIVE,
> +	ZYNQMP_DISP_LAYER_TPG,
> +	ZYNQMP_DISP_LAYER_DISABLE
> +};
> +
> +enum avbuf_vid_mode {
> +	VID_MODE_LIVE,
> +	VID_MODE_MEM,
> +	VID_MODE_TPG,
> +	VID_MODE_NONE
> +};

I don't like this much. The enum here doesn't clearly show that the
values correspond to hardware register values. I'd rather address this
in drivers/gpu/drm/xlnx/zynqmp_disp_regs.h, see below for a proposal.

> +
> +enum avbuf_gfx_mode {
> +	GFX_MODE_DISABLE,
> +	GFX_MODE_MEM,
> +	GFX_MODE_LIVE,
> +	GFX_MODE_NONE
> +};
> +
> +enum avbuf_aud_mode {
> +	AUD1_MODE_LIVE,
> +	AUD1_MODE_MEM,
> +	AUD1_MODE_TPG,
> +	AUD1_MODE_DISABLE,
> +	AUD2_MODE_DISABLE,
> +	AUD2_MODE_ENABLE

Combining AUD1 and AUD2 in the same enum, with the
" - AUD2_MODE_DISABLE" below, is really a hack. Let's keep
hardware-related valeus in drivers/gpu/drm/xlnx/zynqmp_disp_regs.h.

Overall I'm not really fond of this rework I'm afraid, I think the
result is way less readable. Given that this isn't required yet as
support for the TPG or the PL input isn't part of this series, unless it
can be rewritten in a better way already, I'd prefer dropping this patch
for now and including it in a series that enables TPG or PL input.

I also think it could be beneficial to split the patch in two, it seems
to do a bit too much.

>  };
>  
>  /**
> @@ -542,92 +569,102 @@ static void zynqmp_disp_avbuf_disable_channels(struct zynqmp_disp_avbuf *avbuf)
>  }
>  
>  /**
> - * zynqmp_disp_avbuf_enable_audio - Enable audio
> + * zynqmp_disp_avbuf_output_select - Select the buffer manager outputs
>   * @avbuf: Audio/video buffer manager
> + * @layer: The layer
> + * @mode: The mode for this layer
>   *
> - * Enable all audio buffers with a non-live (memory) source.
> + * Select the buffer manager outputs for @layer.
>   */
> -static void zynqmp_disp_avbuf_enable_audio(struct zynqmp_disp_avbuf *avbuf)
> +static void zynqmp_disp_avbuf_output_select(struct zynqmp_disp_avbuf *avbuf,
> +					   struct zynqmp_disp_layer *layer,
> +					   u32 mode)
>  {
> -	u32 val;
> +	u32 reg;
>  
> -	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> -	val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
> -	val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MEM;
> -	val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN;
> -	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
> +	reg = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> +
> +	/* Select audio mode when the layer is NULL */

This is also a hack, I don't really like it. I'd much prefer keeping
audio handling in separate functions.

> +	if (layer == NULL) {
> +		if (mode >= AUD2_MODE_DISABLE) {
> +			reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK;
> +			reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK,
> +					(mode - AUD2_MODE_DISABLE));
> +		} else {
> +			reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
> +			reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK, mode);
> +		}
> +	} else if (is_layer_vid(layer)) {
> +		reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
> +		reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK, mode);
> +	} else {
> +		reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
> +		reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK, mode);
> +	}
> +
> +	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, reg);
>  }
>  
>  /**
> - * zynqmp_disp_avbuf_disable_audio - Disable audio
> + * zynqmp_disp_avbuf_enable_audio - Enable audio
>   * @avbuf: Audio/video buffer manager
>   *
> - * Disable all audio buffers.
> + * Enable all audio buffers.
>   */
> -static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf *avbuf)
> +static void zynqmp_disp_avbuf_enable_audio(struct zynqmp_disp_avbuf *avbuf)
>  {
> -	u32 val;
> -
> -	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> -	val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
> -	val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_DISABLE;
> -	val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN;
> -	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
> +	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD1_MODE_MEM);
> +	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD2_MODE_ENABLE);
>  }
>  
>  /**
> - * zynqmp_disp_avbuf_enable_video - Enable a video layer
> + * zynqmp_disp_avbuf_disable_audio - Disable audio
>   * @avbuf: Audio/video buffer manager
> - * @layer: The layer
> - * @mode: Operating mode of layer
>   *
> - * Enable the video/graphics buffer for @layer.
> + * Disable all audio buffers.
>   */
> -static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf *avbuf,
> -					   struct zynqmp_disp_layer *layer,
> -					   enum zynqmp_disp_layer_mode mode)
> +static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf *avbuf)
>  {
> -	u32 val;
> -
> -	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> -	if (is_layer_vid(layer)) {
> -		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
> -		if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
> -			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM;
> -		else
> -			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_LIVE;
> -	} else {
> -		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
> -		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM;
> -		if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
> -			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM;
> -		else
> -			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_LIVE;
> -	}
> -	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
> +	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD1_MODE_DISABLE);
> +	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD2_MODE_DISABLE);
>  }
>  
>  /**
> - * zynqmp_disp_avbuf_disable_video - Disable a video layer
> - * @avbuf: Audio/video buffer manager
> + * zynqmp_disp_avbuf_set_layer_output - Set layer output
>   * @layer: The layer
> + * @mode: The layer mode
>   *
> - * Disable the video/graphics buffer for @layer.
> + * Set output for @layer
>   */
> -static void zynqmp_disp_avbuf_disable_video(struct zynqmp_disp_avbuf *avbuf,
> -					    struct zynqmp_disp_layer *layer)
> +static void zynqmp_disp_avbuf_set_layer_output(struct zynqmp_disp_layer *layer,
> +					   enum zynqmp_disp_layer_mode mode)
>  {
> -	u32 val;
> -
> -	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> -	if (is_layer_vid(layer)) {
> -		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
> -		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE;
> -	} else {
> -		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
> -		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_DISABLE;
> +	struct zynqmp_disp *disp = layer->disp;
> +	int val;
> +
> +	switch (mode) {
> +	case ZYNQMP_DISP_LAYER_LIVE:
> +		val = is_layer_vid(layer) ? VID_MODE_LIVE : GFX_MODE_LIVE;
> +		break;
> +	case ZYNQMP_DISP_LAYER_MEM:
> +		val = is_layer_vid(layer) ? VID_MODE_MEM : GFX_MODE_MEM;
> +		break;
> +	case ZYNQMP_DISP_LAYER_TPG:
> +		if (!is_layer_vid(layer)) {
> +			dev_err(disp->dev, "gfx layer has no tpg mode\n");
> +			return;
> +		}
> +		val = VID_MODE_TPG;
> +		break;
> +	case ZYNQMP_DISP_LAYER_DISABLE:
> +		val = is_layer_vid(layer) ? VID_MODE_NONE : GFX_MODE_DISABLE;
> +		break;
> +	default:
> +		dev_err(disp->dev, "invalid layer mode\n");
> +		return;
>  	}
> -	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
> +
> +	zynqmp_disp_avbuf_output_select(&disp->avbuf, layer, val);
>  }
>  
>  /**
> @@ -1030,11 +1067,10 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer *layer,
>   */
>  static void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
>  {
> -	zynqmp_disp_avbuf_enable_video(&layer->disp->avbuf, layer,
> -				       ZYNQMP_DISP_LAYER_NONLIVE);
> +	zynqmp_disp_avbuf_set_layer_output(layer, ZYNQMP_DISP_LAYER_MEM);
>  	zynqmp_disp_blend_layer_enable(&layer->disp->blend, layer);
>  
> -	layer->mode = ZYNQMP_DISP_LAYER_NONLIVE;
> +	layer->mode = ZYNQMP_DISP_LAYER_MEM;
>  }
>  
>  /**
> @@ -1051,7 +1087,7 @@ static void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer)
>  	for (i = 0; i < layer->drm_fmt->num_planes; i++)
>  		dmaengine_terminate_sync(layer->dmas[i].chan);
>  
> -	zynqmp_disp_avbuf_disable_video(&layer->disp->avbuf, layer);
> +	zynqmp_disp_avbuf_set_layer_output(layer, ZYNQMP_DISP_LAYER_DISABLE);
>  	zynqmp_disp_blend_layer_disable(&layer->disp->blend, layer);
>  }
>  
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> index f92a006d5070..4316e324102d 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> @@ -118,25 +118,10 @@
>  #define ZYNQMP_DISP_AV_BUF_STC_SNAPSHOT0		0x60
>  #define ZYNQMP_DISP_AV_BUF_STC_SNAPSHOT1		0x64
>  #define ZYNQMP_DISP_AV_BUF_OUTPUT			0x70
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_SHIFT		0
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK		(0x3 << 0)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_LIVE		(0 << 0)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM		(1 << 0)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_PATTERN		(2 << 0)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE		(3 << 0)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_SHIFT		2
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK		(0x3 << 2)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_DISABLE		(0 << 2)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM		(1 << 2)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_LIVE		(2 << 2)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_NONE		(3 << 2)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_SHIFT		4
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK		(0x3 << 4)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_PL		(0 << 4)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MEM		(1 << 4)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_PATTERN		(2 << 4)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_DISABLE		(3 << 4)
> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN		BIT(6)
> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK		GENMASK(1, 0)
> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK		GENMASK(3, 2)
> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK		GENMASK(5, 4)
> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK		BIT(6)
>  #define ZYNQMP_DISP_AV_BUF_HCOUNT_VCOUNT_INT0		0x74
>  #define ZYNQMP_DISP_AV_BUF_HCOUNT_VCOUNT_INT1		0x78
>  #define ZYNQMP_DISP_AV_BUF_PATTERN_GEN_SELECT		0x100

Following my comment above, let's write this

#define ZYNQMP_DISP_AV_BUF_OUTPUT_LIVE			0
#define ZYNQMP_DISP_AV_BUF_OUTPUT_MEM			1
#define ZYNQMP_DISP_AV_BUF_OUTPUT_PATTERN		2
#define ZYNQMP_DISP_AV_BUF_OUTPUT_NONE			3
#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1(v)		((v) << 0)
#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK		GENMASK(1, 0)
#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2(v)		((v) << 2)
#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK		GENMASK(3, 2)
#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID3(v)		((v) << 4)
#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID3_MASK		GENMASK(5, 4)
#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID4(v)		((v) << 6)
#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID4_MASK		GENMASK(7, 6)

Or possibly better,

#define ZYNQMP_DISP_AV_BUF_OUTPUT_LIVE			0
#define ZYNQMP_DISP_AV_BUF_OUTPUT_MEM			1
#define ZYNQMP_DISP_AV_BUF_OUTPUT_PATTERN		2
#define ZYNQMP_DISP_AV_BUF_OUTPUT_NONE			3
#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID(n, v)		((v) << ((n) * 2))
#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID_MASK(n)		GENMASK((n)+1, (n))
-- 
Regards,

Laurent Pinchart

Re: [V2][PATCH 2/2] drm: xlnx: consolidate the functions which programming AUDIO_VIDEO_SELECT register

[quanyang.wang]

Hi Laurent,
Thank you for your review.

On 5/20/21 5:56 PM, Laurent Pinchart wrote:
> Hi Quanyang,
> 
> Thank you for the patch.
> 
> On Tue, May 18, 2021 at 05:50:19PM +0800, quanyang.wang@windriver.com wrote:
>> From: Quanyang Wang <quanyang.wang@windriver.com>
>>
>> For now, the functions zynqmp_disp_avbuf_enable/disable_audio and
>> zynqmp_disp_avbuf_enable/disable_video are all programming the register
>> AV_BUF_OUTPUT_AUDIO_VIDEO_SELECT to select the output for audio or video.
>> And in the future, many drm properties (like video_tpg, audio_tpg,
>> audio_pl, etc) also need to access it. So let's introduce some variables
>> of enum type and consolidate the code to unify handling this.
>>
>> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
>> ---
>>   drivers/gpu/drm/xlnx/zynqmp_disp.c      | 168 ++++++++++++++----------
>>   drivers/gpu/drm/xlnx/zynqmp_disp_regs.h |  23 +---
>>   2 files changed, 106 insertions(+), 85 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> index eefb278e24c6..3672d2f5665b 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> @@ -102,12 +102,39 @@ enum zynqmp_disp_layer_id {
>>   
>>   /**
>>    * enum zynqmp_disp_layer_mode - Layer mode
>> - * @ZYNQMP_DISP_LAYER_NONLIVE: non-live (memory) mode
>> + * @ZYNQMP_DISP_LAYER_MEM: memory mode
>>    * @ZYNQMP_DISP_LAYER_LIVE: live (stream) mode
>> + * @ZYNQMP_DISP_LAYER_TPG: tpg mode (only for video layer)
>> + * @ZYNQMP_DISP_LAYER_DISABLE: disable mode
> 
> "Disable" isn't really a mode :-S
Yes, as per Register Reference for AV_BUF_OUTPUT_AUDIO_VIDEO_SELECT
VID_STREAM2_SEL	3:2	rw	0x2	STREAM2 Enable
				00: Disable graphics from memory/live
				01: Enable graphics from memory
				10: Enable graphics from live
				11: None
VID_STREAM1_SEL	1:0	rw	STREAM1 Select:
				0: Live Video
				1: Video from memory
				2: Pattern Generator
				3: None (black frames)

The NONE mode maybe better.
> 
>>    */
>>   enum zynqmp_disp_layer_mode {
>> -	ZYNQMP_DISP_LAYER_NONLIVE,
>> -	ZYNQMP_DISP_LAYER_LIVE
>> +	ZYNQMP_DISP_LAYER_MEM,
>> +	ZYNQMP_DISP_LAYER_LIVE,
>> +	ZYNQMP_DISP_LAYER_TPG,
>> +	ZYNQMP_DISP_LAYER_DISABLE
>> +};
>> +
>> +enum avbuf_vid_mode {
>> +	VID_MODE_LIVE,
>> +	VID_MODE_MEM,
>> +	VID_MODE_TPG,
>> +	VID_MODE_NONE
>> +};
> 
> I don't like this much. The enum here doesn't clearly show that the
> values correspond to hardware register values. I'd rather address this
> in drivers/gpu/drm/xlnx/zynqmp_disp_regs.h, see below for a proposal.
The value of LIVE/MEM/TPG is different from two layer.
In Video layer, 0 is live, 1 is mem, 2 is tpg and 3 is blackscreen.
But for gfx mode, 0 is disable, 1 is mem, 2 is live and 3 is blackscreen.
If we define ZYNQMP_DISP_AV_BUF_OUTPUT_LIVE is 0, it only works for 
video layer but not graphic layer. So I define one enumeration type to 
abstract the both layer mode and also define two enumeration for each 
layer in order to write enumeration vairables directly to register.
> 
>> +
>> +enum avbuf_gfx_mode {
>> +	GFX_MODE_DISABLE,
>> +	GFX_MODE_MEM,
>> +	GFX_MODE_LIVE,
>> +	GFX_MODE_NONE
>> +};
>> +
>> +enum avbuf_aud_mode {
>> +	AUD1_MODE_LIVE,
>> +	AUD1_MODE_MEM,
>> +	AUD1_MODE_TPG,
>> +	AUD1_MODE_DISABLE,
>> +	AUD2_MODE_DISABLE,
>> +	AUD2_MODE_ENABLE
> 
> Combining AUD1 and AUD2 in the same enum, with the
> " - AUD2_MODE_DISABLE" below, is really a hack. Let's keep
> hardware-related valeus in drivers/gpu/drm/xlnx/zynqmp_disp_regs.h.
OK, I will revert this part.
> 
> Overall I'm not really fond of this rework I'm afraid, I think the
> result is way less readable. Given that this isn't required yet as
> support for the TPG or the PL input isn't part of this series, unless it
> can be rewritten in a better way already, I'd prefer dropping this patch
> for now and including it in a series that enables TPG or PL input.
Yes, I agree. It's not a good time to add this patch now. I will rewrite 
it in a better way. Please drop this patch.

> 
> I also think it could be beneficial to split the patch in two, it seems
> to do a bit too much.
Would you please give some suggestions?
> 
>>   };
>>   
>>   /**
>> @@ -542,92 +569,102 @@ static void zynqmp_disp_avbuf_disable_channels(struct zynqmp_disp_avbuf *avbuf)
>>   }
>>   
>>   /**
>> - * zynqmp_disp_avbuf_enable_audio - Enable audio
>> + * zynqmp_disp_avbuf_output_select - Select the buffer manager outputs
>>    * @avbuf: Audio/video buffer manager
>> + * @layer: The layer
>> + * @mode: The mode for this layer
>>    *
>> - * Enable all audio buffers with a non-live (memory) source.
>> + * Select the buffer manager outputs for @layer.
>>    */
>> -static void zynqmp_disp_avbuf_enable_audio(struct zynqmp_disp_avbuf *avbuf)
>> +static void zynqmp_disp_avbuf_output_select(struct zynqmp_disp_avbuf *avbuf,
>> +					   struct zynqmp_disp_layer *layer,
>> +					   u32 mode)
>>   {
>> -	u32 val;
>> +	u32 reg;
>>   
>> -	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
>> -	val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
>> -	val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MEM;
>> -	val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN;
>> -	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
>> +	reg = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
>> +
>> +	/* Select audio mode when the layer is NULL */
> 
> This is also a hack, I don't really like it. I'd much prefer keeping
> audio handling in separate functions.
OK, I will keep the audio functions.
Thanks,
Quanyang
> 
>> +	if (layer == NULL) {
>> +		if (mode >= AUD2_MODE_DISABLE) {
>> +			reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK;
>> +			reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK,
>> +					(mode - AUD2_MODE_DISABLE));
>> +		} else {
>> +			reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
>> +			reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK, mode);
>> +		}
>> +	} else if (is_layer_vid(layer)) {
>> +		reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
>> +		reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK, mode);
>> +	} else {
>> +		reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
>> +		reg |= FIELD_PREP(ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK, mode);
>> +	}
>> +
>> +	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, reg);
>>   }
>>   
>>   /**
>> - * zynqmp_disp_avbuf_disable_audio - Disable audio
>> + * zynqmp_disp_avbuf_enable_audio - Enable audio
>>    * @avbuf: Audio/video buffer manager
>>    *
>> - * Disable all audio buffers.
>> + * Enable all audio buffers.
>>    */
>> -static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf *avbuf)
>> +static void zynqmp_disp_avbuf_enable_audio(struct zynqmp_disp_avbuf *avbuf)
>>   {
>> -	u32 val;
>> -
>> -	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
>> -	val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
>> -	val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_DISABLE;
>> -	val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN;
>> -	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
>> +	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD1_MODE_MEM);
>> +	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD2_MODE_ENABLE);
>>   }
>>   
>>   /**
>> - * zynqmp_disp_avbuf_enable_video - Enable a video layer
>> + * zynqmp_disp_avbuf_disable_audio - Disable audio
>>    * @avbuf: Audio/video buffer manager
>> - * @layer: The layer
>> - * @mode: Operating mode of layer
>>    *
>> - * Enable the video/graphics buffer for @layer.
>> + * Disable all audio buffers.
>>    */
>> -static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf *avbuf,
>> -					   struct zynqmp_disp_layer *layer,
>> -					   enum zynqmp_disp_layer_mode mode)
>> +static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf *avbuf)
>>   {
>> -	u32 val;
>> -
>> -	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
>> -	if (is_layer_vid(layer)) {
>> -		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
>> -		if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
>> -			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM;
>> -		else
>> -			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_LIVE;
>> -	} else {
>> -		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
>> -		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM;
>> -		if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
>> -			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM;
>> -		else
>> -			val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_LIVE;
>> -	}
>> -	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
>> +	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD1_MODE_DISABLE);
>> +	zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD2_MODE_DISABLE);
>>   }
>>   
>>   /**
>> - * zynqmp_disp_avbuf_disable_video - Disable a video layer
>> - * @avbuf: Audio/video buffer manager
>> + * zynqmp_disp_avbuf_set_layer_output - Set layer output
>>    * @layer: The layer
>> + * @mode: The layer mode
>>    *
>> - * Disable the video/graphics buffer for @layer.
>> + * Set output for @layer
>>    */
>> -static void zynqmp_disp_avbuf_disable_video(struct zynqmp_disp_avbuf *avbuf,
>> -					    struct zynqmp_disp_layer *layer)
>> +static void zynqmp_disp_avbuf_set_layer_output(struct zynqmp_disp_layer *layer,
>> +					   enum zynqmp_disp_layer_mode mode)
>>   {
>> -	u32 val;
>> -
>> -	val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
>> -	if (is_layer_vid(layer)) {
>> -		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
>> -		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE;
>> -	} else {
>> -		val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
>> -		val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_DISABLE;
>> +	struct zynqmp_disp *disp = layer->disp;
>> +	int val;
>> +
>> +	switch (mode) {
>> +	case ZYNQMP_DISP_LAYER_LIVE:
>> +		val = is_layer_vid(layer) ? VID_MODE_LIVE : GFX_MODE_LIVE;
>> +		break;
>> +	case ZYNQMP_DISP_LAYER_MEM:
>> +		val = is_layer_vid(layer) ? VID_MODE_MEM : GFX_MODE_MEM;
>> +		break;
>> +	case ZYNQMP_DISP_LAYER_TPG:
>> +		if (!is_layer_vid(layer)) {
>> +			dev_err(disp->dev, "gfx layer has no tpg mode\n");
>> +			return;
>> +		}
>> +		val = VID_MODE_TPG;
>> +		break;
>> +	case ZYNQMP_DISP_LAYER_DISABLE:
>> +		val = is_layer_vid(layer) ? VID_MODE_NONE : GFX_MODE_DISABLE;
>> +		break;
>> +	default:
>> +		dev_err(disp->dev, "invalid layer mode\n");
>> +		return;
>>   	}
>> -	zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
>> +
>> +	zynqmp_disp_avbuf_output_select(&disp->avbuf, layer, val);
>>   }
>>   
>>   /**
>> @@ -1030,11 +1067,10 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer *layer,
>>    */
>>   static void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
>>   {
>> -	zynqmp_disp_avbuf_enable_video(&layer->disp->avbuf, layer,
>> -				       ZYNQMP_DISP_LAYER_NONLIVE);
>> +	zynqmp_disp_avbuf_set_layer_output(layer, ZYNQMP_DISP_LAYER_MEM);
>>   	zynqmp_disp_blend_layer_enable(&layer->disp->blend, layer);
>>   
>> -	layer->mode = ZYNQMP_DISP_LAYER_NONLIVE;
>> +	layer->mode = ZYNQMP_DISP_LAYER_MEM;
>>   }
>>   
>>   /**
>> @@ -1051,7 +1087,7 @@ static void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer)
>>   	for (i = 0; i < layer->drm_fmt->num_planes; i++)
>>   		dmaengine_terminate_sync(layer->dmas[i].chan);
>>   
>> -	zynqmp_disp_avbuf_disable_video(&layer->disp->avbuf, layer);
>> +	zynqmp_disp_avbuf_set_layer_output(layer, ZYNQMP_DISP_LAYER_DISABLE);
>>   	zynqmp_disp_blend_layer_disable(&layer->disp->blend, layer);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
>> index f92a006d5070..4316e324102d 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
>> @@ -118,25 +118,10 @@
>>   #define ZYNQMP_DISP_AV_BUF_STC_SNAPSHOT0		0x60
>>   #define ZYNQMP_DISP_AV_BUF_STC_SNAPSHOT1		0x64
>>   #define ZYNQMP_DISP_AV_BUF_OUTPUT			0x70
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_SHIFT		0
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK		(0x3 << 0)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_LIVE		(0 << 0)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM		(1 << 0)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_PATTERN		(2 << 0)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE		(3 << 0)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_SHIFT		2
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK		(0x3 << 2)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_DISABLE		(0 << 2)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM		(1 << 2)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_LIVE		(2 << 2)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_NONE		(3 << 2)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_SHIFT		4
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK		(0x3 << 4)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_PL		(0 << 4)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MEM		(1 << 4)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_PATTERN		(2 << 4)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_DISABLE		(3 << 4)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN		BIT(6)
>> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK		GENMASK(1, 0)
>> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK		GENMASK(3, 2)
>> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK		GENMASK(5, 4)
>> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK		BIT(6)
>>   #define ZYNQMP_DISP_AV_BUF_HCOUNT_VCOUNT_INT0		0x74
>>   #define ZYNQMP_DISP_AV_BUF_HCOUNT_VCOUNT_INT1		0x78
>>   #define ZYNQMP_DISP_AV_BUF_PATTERN_GEN_SELECT		0x100
> 
> Following my comment above, let's write this
> 
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_LIVE			0
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_MEM			1
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_PATTERN		2
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_NONE			3
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1(v)		((v) << 0)
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK		GENMASK(1, 0)
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2(v)		((v) << 2)
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK		GENMASK(3, 2)
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID3(v)		((v) << 4)
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID3_MASK		GENMASK(5, 4)
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID4(v)		((v) << 6)
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID4_MASK		GENMASK(7, 6)
> 
> Or possibly better,
> 
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_LIVE			0
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_MEM			1
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_PATTERN		2
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_NONE			3
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID(n, v)		((v) << ((n) * 2))
> #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID_MASK(n)		GENMASK((n)+1, (n))
>