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(-)
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,
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;
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
*/ static void zynqmp_disp_avbuf_set_format(struct zynqmp_disp_avbuf *avbuf,
- @layer: The layer
- @fmt: The format information
- Set the video buffer manager format for @layer to @fmt.
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
*/ static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf *avbuf,
- @layer: The layer
- @mode: Operating mode of layer
- Enable the video/graphics buffer for @layer.
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
*/ static void zynqmp_disp_avbuf_disable_video(struct zynqmp_disp_avbuf *avbuf,
- @layer: The layer
- Disable the video/graphics buffer for @layer.
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;
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
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;elseval |= 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;elseval |= 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
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;elseval |= 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;elseval |= 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))
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;elseval |= 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;elseval |= 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))
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Paul Cercueil -
quanyang.wang -
quanyang.wang＠windriver.com