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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Paul,
Thank you for your review.
I will update all of this in V2.
Thanks,
Quanyang

On 5/18/21 1:09 AM, Paul Cercueil wrote:
Hi,

Le jeu., mai 13 2021 at 19:45:40 +0800, quanyang.wang@xxxxxxxxxxxxx a écrit :
From: Quanyang Wang <quanyang.wang@xxxxxxxxxxxxx>

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@xxxxxxxxxxxxx>
---
 drivers/gpu/drm/xlnx/zynqmp_disp.c      | 166 ++++++++++++++----------
 drivers/gpu/drm/xlnx/zynqmp_disp_regs.h |  15 +--
 2 files changed, 101 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index c55e24412f8c..a82bc88a98aa 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,98 @@ 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)

You can put 'mode' on a new line to avoid getting over 80 characters.

 {
-    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_MEM;
-    val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN;
-    zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
+    u32 reg;
+
+    reg = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);

Empty line here (spacing before comment)

+    /* 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 |= (mode - AUD2_MODE_DISABLE)
+                << ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_SHIFT;

Please consider using the FIELD_PREP() macro from <linux/bitfield.h>. Then you can get rid of your *_SHIFT macros.

+        } else {
+            reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
+            reg |= mode << ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_SHIFT;
+        }
+    } else if (is_layer_vid(layer)) {
+        reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
+        reg |= mode << ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_SHIFT;
+    } else {
+        reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
+        reg |= mode << ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_SHIFT;
+    }

Empty line here (spacing after block)

+    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;

Same as above with FIELD_PREP().

-    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

You're missing a space after the dash character.

  * @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;
+    int val;
+    struct zynqmp_disp *disp = layer->disp;

I'd swap these two lines above - variables are usually defined in "reverse christmas tree" order, the longest line first, the smallest line last. No big deal though.

+
+    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);

While you're at it, you can add an empty line here (spacing after block)

+    zynqmp_disp_avbuf_output_select(&disp->avbuf, layer, val);
 }

 /**
@@ -1030,11 +1063,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 +1083,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..dad3e356d9ab 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
@@ -120,23 +120,12 @@
 #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_AUD2_MASK        (0x1 << 6)
+#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_SHIFT        6

Please use BIT() or GENMASK(). You don't need the _SHIFT macros if you use FIELD_PREP() / FIELD_GET().

Cheers,
-Paul

 #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





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux