* [PATCH v3 01/15] drm/msm/dpu: add formats check for writeback encoder
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 6:40 ` Dmitry Baryshkov
2023-12-12 0:22 ` [PATCH v3 02/15] drm/msm/dpu: rename dpu_encoder_phys_wb_setup_cdp to match its functionality Abhinav Kumar
` (13 subsequent siblings)
14 siblings, 1 reply; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm, linux-kernel
In preparation for adding more formats to dpu writeback add
format validation to it to fail any unsupported formats.
changes in v3:
- rebase on top of msm-next
- replace drm_atomic_helper_check_wb_encoder_state() with
drm_atomic_helper_check_wb_connector_state() due to the
rebase
changes in v2:
- correct some grammar in the commit text
Fixes: d7d0e73f7de3 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for writeback")
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index bb94909caa25..425415d45ec1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -272,6 +272,7 @@ static int dpu_encoder_phys_wb_atomic_check(
{
struct drm_framebuffer *fb;
const struct drm_display_mode *mode = &crtc_state->mode;
+ int ret;
DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
phys_enc->hw_wb->idx, mode->name, mode->hdisplay, mode->vdisplay);
@@ -308,6 +309,12 @@ static int dpu_encoder_phys_wb_atomic_check(
return -EINVAL;
}
+ ret = drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
+ if (ret < 0) {
+ DPU_ERROR("invalid pixel format %p4cc\n", &fb->format->format);
+ return ret;
+ }
+
return 0;
}
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 02/15] drm/msm/dpu: rename dpu_encoder_phys_wb_setup_cdp to match its functionality
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
2023-12-12 0:22 ` [PATCH v3 01/15] drm/msm/dpu: add formats check for writeback encoder Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 03/15] drm/msm/dpu: fix writeback programming for YUV cases Abhinav Kumar
` (12 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm, linux-kernel
dpu_encoder_phys_wb_setup_cdp() is not programming the chroma down
prefetch block. Its setting up the display ctl path for writeback.
Hence rename it to dpu_encoder_phys_wb_setup_ctl() to match what its
actually doing.
Fixes: d7d0e73f7de3 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for writeback")
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 425415d45ec1..8f05f2a1fc24 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -207,10 +207,10 @@ static void dpu_encoder_phys_wb_setup_fb(struct dpu_encoder_phys *phys_enc,
}
/**
- * dpu_encoder_phys_wb_setup_cdp - setup chroma down prefetch block
+ * dpu_encoder_phys_wb_setup_ctl - setup wb pipeline for ctl path
* @phys_enc:Pointer to physical encoder
*/
-static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
+static void dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
{
struct dpu_hw_wb *hw_wb;
struct dpu_hw_ctl *ctl;
@@ -382,7 +382,7 @@ static void dpu_encoder_phys_wb_setup(
dpu_encoder_phys_wb_setup_fb(phys_enc, fb);
- dpu_encoder_phys_wb_setup_cdp(phys_enc);
+ dpu_encoder_phys_wb_setup_ctl(phys_enc);
}
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 03/15] drm/msm/dpu: fix writeback programming for YUV cases
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
2023-12-12 0:22 ` [PATCH v3 01/15] drm/msm/dpu: add formats check for writeback encoder Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 02/15] drm/msm/dpu: rename dpu_encoder_phys_wb_setup_cdp to match its functionality Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 04/15] drm/msm/dpu: move csc matrices to dpu_hw_util Abhinav Kumar
` (11 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm, linux-kernel
For YUV cases, setting the required format bits was missed
out in the register programming. Lets fix it now in preparation
of adding YUV formats support for writeback.
changes in v2:
- dropped the fixes tag as its not a fix but adding
new functionality
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
index ed0e80616129..e75995f7fcea 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
@@ -89,6 +89,9 @@ static void dpu_hw_wb_setup_format(struct dpu_hw_wb *ctx,
dst_format |= BIT(14); /* DST_ALPHA_X */
}
+ if (DPU_FORMAT_IS_YUV(fmt))
+ dst_format |= BIT(15);
+
pattern = (fmt->element[3] << 24) |
(fmt->element[2] << 16) |
(fmt->element[1] << 8) |
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 04/15] drm/msm/dpu: move csc matrices to dpu_hw_util
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
` (2 preceding siblings ...)
2023-12-12 0:22 ` [PATCH v3 03/15] drm/msm/dpu: fix writeback programming for YUV cases Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 6:41 ` Dmitry Baryshkov
2023-12-12 0:22 ` [PATCH v3 05/15] drm/msm/dpu: add cdm blocks to sc7280 dpu_hw_catalog Abhinav Kumar
` (10 subsequent siblings)
14 siblings, 1 reply; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm, linux-kernel
Since the type and usage of CSC matrices is spanning across DPU
lets introduce a helper to the dpu_hw_util to return the CSC
corresponding to the request type. This will help to add more
supported CSC types such as the RGB to YUV one which is used in
the case of CDM.
changes in v3:
- drop the extra wrapper and export the matrices directly
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 30 ++++++++++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 31 +--------------------
2 files changed, 31 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
index fe083b2e5696..aa50005042d1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
@@ -19,6 +19,36 @@
#define MISR_CTRL_STATUS_CLEAR BIT(10)
#define MISR_CTRL_FREE_RUN_MASK BIT(31)
+static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
+ {
+ /* S15.16 format */
+ 0x00012A00, 0x00000000, 0x00019880,
+ 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
+ 0x00012A00, 0x00020480, 0x00000000,
+ },
+ /* signed bias */
+ { 0xfff0, 0xff80, 0xff80,},
+ { 0x0, 0x0, 0x0,},
+ /* unsigned clamp */
+ { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
+ { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
+};
+
+static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
+ {
+ /* S15.16 format */
+ 0x00012A00, 0x00000000, 0x00019880,
+ 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
+ 0x00012A00, 0x00020480, 0x00000000,
+ },
+ /* signed bias */
+ { 0xffc0, 0xfe00, 0xfe00,},
+ { 0x0, 0x0, 0x0,},
+ /* unsigned clamp */
+ { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
+ { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
+};
+
/*
* This is the common struct maintained by each sub block
* for mapping the register offsets in this block to the
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 3235ab132540..ff975ad51145 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -21,6 +21,7 @@
#include "dpu_kms.h"
#include "dpu_formats.h"
#include "dpu_hw_sspp.h"
+#include "dpu_hw_util.h"
#include "dpu_trace.h"
#include "dpu_crtc.h"
#include "dpu_vbif.h"
@@ -508,36 +509,6 @@ static void _dpu_plane_setup_pixel_ext(struct dpu_hw_scaler3_cfg *scale_cfg,
}
}
-static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
- {
- /* S15.16 format */
- 0x00012A00, 0x00000000, 0x00019880,
- 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
- 0x00012A00, 0x00020480, 0x00000000,
- },
- /* signed bias */
- { 0xfff0, 0xff80, 0xff80,},
- { 0x0, 0x0, 0x0,},
- /* unsigned clamp */
- { 0x10, 0xeb, 0x10, 0xf0, 0x10, 0xf0,},
- { 0x00, 0xff, 0x00, 0xff, 0x00, 0xff,},
-};
-
-static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
- {
- /* S15.16 format */
- 0x00012A00, 0x00000000, 0x00019880,
- 0x00012A00, 0xFFFF9B80, 0xFFFF3000,
- 0x00012A00, 0x00020480, 0x00000000,
- },
- /* signed bias */
- { 0xffc0, 0xfe00, 0xfe00,},
- { 0x0, 0x0, 0x0,},
- /* unsigned clamp */
- { 0x40, 0x3ac, 0x40, 0x3c0, 0x40, 0x3c0,},
- { 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
-};
-
static const struct dpu_csc_cfg *_dpu_plane_get_csc(struct dpu_sw_pipe *pipe,
const struct dpu_format *fmt)
{
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 05/15] drm/msm/dpu: add cdm blocks to sc7280 dpu_hw_catalog
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
` (3 preceding siblings ...)
2023-12-12 0:22 ` [PATCH v3 04/15] drm/msm/dpu: move csc matrices to dpu_hw_util Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 06/15] drm/msm/dpu: add cdm blocks to sm8250 dpu_hw_catalog Abhinav Kumar
` (9 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm, linux-kernel
Add CDM blocks to the sc7280 dpu_hw_catalog to support
YUV format output from writeback block.
changes in v3:
- change the comment from sub-blk to clk for CDM
changes in v2:
- remove explicit zero assignment for features
- move sc7280_cdm to dpu_hw_catalog from the sc7280
catalog file as its definition can be re-used
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
.../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10 ++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 13 +++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 5 +++++
4 files changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
index 209675de6742..19c2b7454796 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
@@ -248,6 +248,7 @@ const struct dpu_mdss_cfg dpu_sc7280_cfg = {
.mdss_ver = &sc7280_mdss_ver,
.caps = &sc7280_dpu_caps,
.mdp = &sc7280_mdp,
+ .cdm = &sc7280_cdm,
.ctl_count = ARRAY_SIZE(sc7280_ctl),
.ctl = sc7280_ctl,
.sspp_count = ARRAY_SIZE(sc7280_sspp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index d52aae54bbd5..b304bebedb84 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -426,6 +426,16 @@ static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
.ctl = {.name = "ctl", .base = 0xF80, .len = 0x10},
};
+/*************************************************************
+ * CDM block config
+ *************************************************************/
+static const struct dpu_cdm_cfg sc7280_cdm = {
+ .name = "cdm_0",
+ .id = CDM_0,
+ .len = 0x228,
+ .base = 0x79200,
+};
+
/*************************************************************
* VBIF sub blocks config
*************************************************************/
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index e3c0d007481b..ba82ef4560a6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -682,6 +682,17 @@ struct dpu_vbif_cfg {
u32 memtype[MAX_XIN_COUNT];
};
+/**
+ * struct dpu_cdm_cfg - information of chroma down blocks
+ * @name string name for debug purposes
+ * @id enum identifying this block
+ * @base register offset of this block
+ * @features bit mask identifying sub-blocks/features
+ */
+struct dpu_cdm_cfg {
+ DPU_HW_BLK_INFO;
+};
+
/**
* Define CDP use cases
* @DPU_PERF_CDP_UDAGE_RT: real-time use cases
@@ -805,6 +816,8 @@ struct dpu_mdss_cfg {
u32 wb_count;
const struct dpu_wb_cfg *wb;
+ const struct dpu_cdm_cfg *cdm;
+
u32 ad_count;
u32 dspp_count;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
index a6702b2bfc68..f319c8232ea5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
@@ -185,6 +185,11 @@ enum dpu_dsc {
DSC_MAX
};
+enum dpu_cdm {
+ CDM_0 = 1,
+ CDM_MAX
+};
+
enum dpu_pingpong {
PINGPONG_NONE,
PINGPONG_0,
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 06/15] drm/msm/dpu: add cdm blocks to sm8250 dpu_hw_catalog
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
` (4 preceding siblings ...)
2023-12-12 0:22 ` [PATCH v3 05/15] drm/msm/dpu: add cdm blocks to sc7280 dpu_hw_catalog Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 07/15] drm/msm/dpu: add dpu_hw_cdm abstraction for CDM block Abhinav Kumar
` (8 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm, linux-kernel
Add CDM blocks to the sm8250 dpu_hw_catalog to support
YUV format output from writeback block.
changes in v2:
- re-use the cdm definition from sc7280
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
index 2359c16e9206..58b0f50518c8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
@@ -384,6 +384,7 @@ const struct dpu_mdss_cfg dpu_sm8250_cfg = {
.mdss_ver = &sm8250_mdss_ver,
.caps = &sm8250_dpu_caps,
.mdp = &sm8250_mdp,
+ .cdm = &sc7280_cdm,
.ctl_count = ARRAY_SIZE(sm8250_ctl),
.ctl = sm8250_ctl,
.sspp_count = ARRAY_SIZE(sm8250_sspp),
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 07/15] drm/msm/dpu: add dpu_hw_cdm abstraction for CDM block
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
` (5 preceding siblings ...)
2023-12-12 0:22 ` [PATCH v3 06/15] drm/msm/dpu: add cdm blocks to sm8250 dpu_hw_catalog Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 9:40 ` Dmitry Baryshkov
2023-12-12 0:22 ` [PATCH v3 08/15] drm/msm/dpu: add cdm blocks to RM Abhinav Kumar
` (7 subsequent siblings)
14 siblings, 1 reply; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
kernel test robot, linux-kernel
CDM block comes with its own set of registers and operations
which can be done. In-line with other hardware blocks, this
change adds the dpu_hw_cdm abstraction for the CDM block.
changes in v3:
- fix commit text from sub-blk to blk for CDM
- fix kbot issue for missing static for dpu_hw_cdm_enable()
- fix kbot issue for incorrect documentation style
- add more documentation for enums and struct in dpu_hw_cdm.h
- drop "enable" parameter from bind_pingpong_blk() as we can
just use PINGPONG_NONE for disable cases
- drop unnecessary bit operation for zero value of cdm_cfg
changes in v2:
- replace bit magic with relevant defines
- use drmm_kzalloc instead of kzalloc/free
- some formatting fixes
- inline _setup_cdm_ops()
- protect bind_pingpong_blk with core_rev check
- drop setup_csc_data() and setup_cdwn() ops as they
are merged into enable()
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202312101815.B3ZH7Pfy-lkp@intel.com/
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c | 263 ++++++++++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h | 130 ++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 1 +
4 files changed, 395 insertions(+)
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 49671364fdcf..b1173128b5b9 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -63,6 +63,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
disp/dpu1/dpu_encoder_phys_wb.o \
disp/dpu1/dpu_formats.o \
disp/dpu1/dpu_hw_catalog.o \
+ disp/dpu1/dpu_hw_cdm.o \
disp/dpu1/dpu_hw_ctl.o \
disp/dpu1/dpu_hw_dsc.o \
disp/dpu1/dpu_hw_dsc_1_2.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
new file mode 100644
index 000000000000..4976f8a05ce7
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
@@ -0,0 +1,263 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023, The Linux Foundation. All rights reserved.
+ */
+
+#include <drm/drm_managed.h>
+
+#include "dpu_hw_mdss.h"
+#include "dpu_hw_util.h"
+#include "dpu_hw_catalog.h"
+#include "dpu_hw_cdm.h"
+#include "dpu_kms.h"
+
+#define CDM_CSC_10_OPMODE 0x000
+#define CDM_CSC_10_BASE 0x004
+
+#define CDM_CDWN2_OP_MODE 0x100
+#define CDM_CDWN2_CLAMP_OUT 0x104
+#define CDM_CDWN2_PARAMS_3D_0 0x108
+#define CDM_CDWN2_PARAMS_3D_1 0x10C
+#define CDM_CDWN2_COEFF_COSITE_H_0 0x110
+#define CDM_CDWN2_COEFF_COSITE_H_1 0x114
+#define CDM_CDWN2_COEFF_COSITE_H_2 0x118
+#define CDM_CDWN2_COEFF_OFFSITE_H_0 0x11C
+#define CDM_CDWN2_COEFF_OFFSITE_H_1 0x120
+#define CDM_CDWN2_COEFF_OFFSITE_H_2 0x124
+#define CDM_CDWN2_COEFF_COSITE_V 0x128
+#define CDM_CDWN2_COEFF_OFFSITE_V 0x12C
+#define CDM_CDWN2_OUT_SIZE 0x130
+
+#define CDM_HDMI_PACK_OP_MODE 0x200
+#define CDM_CSC_10_MATRIX_COEFF_0 0x004
+
+#define CDM_MUX 0x224
+
+/* CDM CDWN2 sub-block bit definitions */
+#define CDM_CDWN2_OP_MODE_EN BIT(0)
+#define CDM_CDWN2_OP_MODE_ENABLE_H BIT(1)
+#define CDM_CDWN2_OP_MODE_ENABLE_V BIT(2)
+#define CDM_CDWN2_OP_MODE_METHOD_H_AVG BIT(3)
+#define CDM_CDWN2_OP_MODE_METHOD_H_COSITE BIT(4)
+#define CDM_CDWN2_OP_MODE_METHOD_V_AVG BIT(5)
+#define CDM_CDWN2_OP_MODE_METHOD_V_COSITE BIT(6)
+#define CDM_CDWN2_OP_MODE_BITS_OUT_8BIT BIT(7)
+#define CDM_CDWN2_OP_MODE_METHOD_H_OFFSITE GENMASK(4, 3)
+#define CDM_CDWN2_OP_MODE_METHOD_V_OFFSITE GENMASK(6, 5)
+#define CDM_CDWN2_V_PIXEL_DROP_MASK GENMASK(6, 5)
+#define CDM_CDWN2_H_PIXEL_DROP_MASK GENMASK(4, 3)
+
+/* CDM CSC10 sub-block bit definitions */
+#define CDM_CSC10_OP_MODE_EN BIT(0)
+#define CDM_CSC10_OP_MODE_SRC_FMT_YUV BIT(1)
+#define CDM_CSC10_OP_MODE_DST_FMT_YUV BIT(2)
+
+/* CDM HDMI pack sub-block bit definitions */
+#define CDM_HDMI_PACK_OP_MODE_EN BIT(0)
+
+/*
+ * Horizontal coefficients for cosite chroma downscale
+ * s13 representation of coefficients
+ */
+static u32 cosite_h_coeff[] = {0x00000016, 0x000001cc, 0x0100009e};
+
+/*
+ * Horizontal coefficients for offsite chroma downscale
+ */
+static u32 offsite_h_coeff[] = {0x000b0005, 0x01db01eb, 0x00e40046};
+
+/*
+ * Vertical coefficients for cosite chroma downscale
+ */
+static u32 cosite_v_coeff[] = {0x00080004};
+/*
+ * Vertical coefficients for offsite chroma downscale
+ */
+static u32 offsite_v_coeff[] = {0x00060002};
+
+static int dpu_hw_cdm_setup_cdwn(struct dpu_hw_cdm *ctx, struct dpu_hw_cdm_cfg *cfg)
+{
+ struct dpu_hw_blk_reg_map *c = &ctx->hw;
+ u32 opmode = 0;
+ u32 out_size = 0;
+
+ if (cfg->output_bit_depth != CDM_CDWN_OUTPUT_10BIT)
+ opmode |= CDM_CDWN2_OP_MODE_BITS_OUT_8BIT;
+
+ /* ENABLE DWNS_H bit */
+ opmode |= CDM_CDWN2_OP_MODE_ENABLE_H;
+
+ switch (cfg->h_cdwn_type) {
+ case CDM_CDWN_DISABLE:
+ /* CLEAR METHOD_H field */
+ opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
+ /* CLEAR DWNS_H bit */
+ opmode &= ~CDM_CDWN2_OP_MODE_ENABLE_H;
+ break;
+ case CDM_CDWN_PIXEL_DROP:
+ /* Clear METHOD_H field (pixel drop is 0) */
+ opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
+ break;
+ case CDM_CDWN_AVG:
+ /* Clear METHOD_H field (Average is 0x1) */
+ opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
+ opmode |= CDM_CDWN2_OP_MODE_METHOD_H_AVG;
+ break;
+ case CDM_CDWN_COSITE:
+ /* Clear METHOD_H field (Average is 0x2) */
+ opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
+ opmode |= CDM_CDWN2_OP_MODE_METHOD_H_COSITE;
+ /* Co-site horizontal coefficients */
+ DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_0,
+ cosite_h_coeff[0]);
+ DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_1,
+ cosite_h_coeff[1]);
+ DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_2,
+ cosite_h_coeff[2]);
+ break;
+ case CDM_CDWN_OFFSITE:
+ /* Clear METHOD_H field (Average is 0x3) */
+ opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
+ opmode |= CDM_CDWN2_OP_MODE_METHOD_H_OFFSITE;
+
+ /* Off-site horizontal coefficients */
+ DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_0,
+ offsite_h_coeff[0]);
+ DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_1,
+ offsite_h_coeff[1]);
+ DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_2,
+ offsite_h_coeff[2]);
+ break;
+ default:
+ DPU_ERROR("%s invalid horz down sampling type\n", __func__);
+ return -EINVAL;
+ }
+
+ /* ENABLE DWNS_V bit */
+ opmode |= CDM_CDWN2_OP_MODE_ENABLE_V;
+
+ switch (cfg->v_cdwn_type) {
+ case CDM_CDWN_DISABLE:
+ /* CLEAR METHOD_V field */
+ opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
+ /* CLEAR DWNS_V bit */
+ opmode &= ~CDM_CDWN2_OP_MODE_ENABLE_V;
+ break;
+ case CDM_CDWN_PIXEL_DROP:
+ /* Clear METHOD_V field (pixel drop is 0) */
+ opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
+ break;
+ case CDM_CDWN_AVG:
+ /* Clear METHOD_V field (Average is 0x1) */
+ opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
+ opmode |= CDM_CDWN2_OP_MODE_METHOD_V_AVG;
+ break;
+ case CDM_CDWN_COSITE:
+ /* Clear METHOD_V field (Average is 0x2) */
+ opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
+ opmode |= CDM_CDWN2_OP_MODE_METHOD_V_COSITE;
+ /* Co-site vertical coefficients */
+ DPU_REG_WRITE(c,
+ CDM_CDWN2_COEFF_COSITE_V,
+ cosite_v_coeff[0]);
+ break;
+ case CDM_CDWN_OFFSITE:
+ /* Clear METHOD_V field (Average is 0x3) */
+ opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
+ opmode |= CDM_CDWN2_OP_MODE_METHOD_V_OFFSITE;
+
+ /* Off-site vertical coefficients */
+ DPU_REG_WRITE(c,
+ CDM_CDWN2_COEFF_OFFSITE_V,
+ offsite_v_coeff[0]);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (cfg->v_cdwn_type || cfg->h_cdwn_type)
+ opmode |= CDM_CDWN2_OP_MODE_EN; /* EN CDWN module */
+ else
+ opmode &= ~CDM_CDWN2_OP_MODE_EN;
+
+ out_size = (cfg->output_width & 0xFFFF) | ((cfg->output_height & 0xFFFF) << 16);
+ DPU_REG_WRITE(c, CDM_CDWN2_OUT_SIZE, out_size);
+ DPU_REG_WRITE(c, CDM_CDWN2_OP_MODE, opmode);
+ DPU_REG_WRITE(c, CDM_CDWN2_CLAMP_OUT, ((0x3FF << 16) | 0x0));
+
+ return 0;
+}
+
+static int dpu_hw_cdm_enable(struct dpu_hw_cdm *ctx, struct dpu_hw_cdm_cfg *cdm)
+{
+ struct dpu_hw_blk_reg_map *c = &ctx->hw;
+ const struct dpu_format *fmt;
+ u32 opmode = 0;
+ u32 csc = 0;
+
+ if (!ctx || !cdm)
+ return -EINVAL;
+
+ fmt = cdm->output_fmt;
+
+ if (!DPU_FORMAT_IS_YUV(fmt))
+ return -EINVAL;
+
+ dpu_hw_csc_setup(&ctx->hw, CDM_CSC_10_MATRIX_COEFF_0, cdm->csc_cfg, true);
+ dpu_hw_cdm_setup_cdwn(ctx, cdm);
+
+ if (cdm->output_type == CDM_CDWN_OUTPUT_HDMI) {
+ if (fmt->chroma_sample != DPU_CHROMA_H1V2)
+ return -EINVAL; /*unsupported format */
+ opmode = CDM_HDMI_PACK_OP_MODE_EN;
+ opmode |= (fmt->chroma_sample << 1);
+ }
+
+ csc |= CDM_CSC10_OP_MODE_DST_FMT_YUV;
+ csc &= ~CDM_CSC10_OP_MODE_SRC_FMT_YUV;
+ csc |= CDM_CSC10_OP_MODE_EN;
+
+ if (ctx && ctx->ops.bind_pingpong_blk)
+ ctx->ops.bind_pingpong_blk(ctx, cdm->pp_id);
+
+ DPU_REG_WRITE(c, CDM_CSC_10_OPMODE, csc);
+ DPU_REG_WRITE(c, CDM_HDMI_PACK_OP_MODE, opmode);
+ return 0;
+}
+
+static void dpu_hw_cdm_bind_pingpong_blk(struct dpu_hw_cdm *ctx, const enum dpu_pingpong pp)
+{
+ struct dpu_hw_blk_reg_map *c;
+ int mux_cfg = 0xF; /* Disabled */
+
+ c = &ctx->hw;
+
+ if (pp)
+ mux_cfg = (pp - PINGPONG_0) & 0x7;
+
+ DPU_REG_WRITE(c, CDM_MUX, mux_cfg);
+}
+
+struct dpu_hw_cdm *dpu_hw_cdm_init(struct drm_device *dev,
+ const struct dpu_cdm_cfg *cfg, void __iomem *addr,
+ const struct dpu_mdss_version *mdss_rev)
+{
+ struct dpu_hw_cdm *c;
+
+ c = drmm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+ if (!c)
+ return ERR_PTR(-ENOMEM);
+
+ c->hw.blk_addr = addr + cfg->base;
+ c->hw.log_mask = DPU_DBG_MASK_CDM;
+
+ /* Assign ops */
+ c->idx = cfg->id;
+ c->caps = cfg;
+
+ c->ops.enable = dpu_hw_cdm_enable;
+ if (mdss_rev->core_major_ver >= 5)
+ c->ops.bind_pingpong_blk = dpu_hw_cdm_bind_pingpong_blk;
+
+ return c;
+}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
new file mode 100644
index 000000000000..e7d57dbd6103
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
@@ -0,0 +1,130 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _DPU_HW_CDM_H
+#define _DPU_HW_CDM_H
+
+#include "dpu_hw_mdss.h"
+#include "dpu_hw_top.h"
+
+struct dpu_hw_cdm;
+
+/**
+ * struct dpu_hw_cdm_cfg : current configuration of CDM block
+ *
+ * @output_width: output ROI width of CDM block
+ * @output_height: output ROI height of CDM block
+ * @output_bit_depth: output bit-depth of CDM block
+ * @h_cdwn_type: downsample type used for horizontal pixels
+ * @v_cdwn_type: downsample type used for vertical pixels
+ * @output_fmt: handle to dpu_format of CDM block
+ * @csc_cfg: handle to CSC matrix programmed for CDM block
+ * @output_type: interface to which CDM is paired (HDMI/WB)
+ * @pp_id: ping-pong block to which CDM is bound to
+ */
+struct dpu_hw_cdm_cfg {
+ u32 output_width;
+ u32 output_height;
+ u32 output_bit_depth;
+ u32 h_cdwn_type;
+ u32 v_cdwn_type;
+ const struct dpu_format *output_fmt;
+ const struct dpu_csc_cfg *csc_cfg;
+ u32 output_type;
+ int pp_id;
+};
+
+/*
+ * These values are used indicate which type of downsample is used
+ * in the horizontal/vertical direction for the CDM block.
+ */
+enum dpu_hw_cdwn_type {
+ CDM_CDWN_DISABLE,
+ CDM_CDWN_PIXEL_DROP,
+ CDM_CDWN_AVG,
+ CDM_CDWN_COSITE,
+ CDM_CDWN_OFFSITE,
+};
+
+/*
+ * CDM block can be paired with WB or HDMI block. These values match
+ * the input with which the CDM block is paired.
+ */
+enum dpu_hw_cdwn_output_type {
+ CDM_CDWN_OUTPUT_HDMI,
+ CDM_CDWN_OUTPUT_WB,
+};
+
+/*
+ * CDM block can give an 8-bit or 10-bit output. These values
+ * are used to indicate the output bit depth of CDM block
+ */
+enum dpu_hw_cdwn_output_bit_depth {
+ CDM_CDWN_OUTPUT_8BIT,
+ CDM_CDWN_OUTPUT_10BIT,
+};
+
+/**
+ * struct dpu_hw_cdm_ops : Interface to the chroma down Hw driver functions
+ * Assumption is these functions will be called after
+ * clocks are enabled
+ * @enable: Enables the output to interface and programs the
+ * output packer
+ * @bind_pingpong_blk: enable/disable the connection with pingpong which
+ * will feed pixels to this cdm
+ */
+struct dpu_hw_cdm_ops {
+ /**
+ * Enable the CDM module
+ * @cdm Pointer to chroma down context
+ */
+ int (*enable)(struct dpu_hw_cdm *cdm, struct dpu_hw_cdm_cfg *cfg);
+
+ /**
+ * Enable/disable the connection with pingpong
+ * @cdm Pointer to chroma down context
+ * @pp pingpong block id.
+ */
+ void (*bind_pingpong_blk)(struct dpu_hw_cdm *cdm, const enum dpu_pingpong pp);
+};
+
+/**
+ * struct dpu_hw_cdm - cdm description
+ * @base: Hardware block base structure
+ * @hw: Block hardware details
+ * @idx: CDM index
+ * @caps: Pointer to cdm_cfg
+ * @ops: handle to operations possible for this CDM
+ */
+struct dpu_hw_cdm {
+ struct dpu_hw_blk base;
+ struct dpu_hw_blk_reg_map hw;
+
+ /* chroma down */
+ const struct dpu_cdm_cfg *caps;
+ enum dpu_cdm idx;
+
+ /* ops */
+ struct dpu_hw_cdm_ops ops;
+};
+
+/**
+ * dpu_hw_cdm_init - initializes the cdm hw driver object.
+ * should be called once before accessing every cdm.
+ * @dev: DRM device handle
+ * @cdm: CDM catalog entry for which driver object is required
+ * @addr : mapped register io address of MDSS
+ * @mdss_rev: mdss hw core revision
+ */
+struct dpu_hw_cdm *dpu_hw_cdm_init(struct drm_device *dev,
+ const struct dpu_cdm_cfg *cdm, void __iomem *addr,
+ const struct dpu_mdss_version *mdss_rev);
+
+static inline struct dpu_hw_cdm *to_dpu_hw_cdm(struct dpu_hw_blk *hw)
+{
+ return container_of(hw, struct dpu_hw_cdm, base);
+}
+
+#endif /*_DPU_HW_CDM_H */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
index f319c8232ea5..9db4cf61bd29 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
@@ -466,6 +466,7 @@ struct dpu_mdss_color {
#define DPU_DBG_MASK_ROT (1 << 9)
#define DPU_DBG_MASK_DSPP (1 << 10)
#define DPU_DBG_MASK_DSC (1 << 11)
+#define DPU_DBG_MASK_CDM (1 << 12)
/**
* struct dpu_hw_tear_check - Struct contains parameters to configure
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 08/15] drm/msm/dpu: add cdm blocks to RM
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
` (6 preceding siblings ...)
2023-12-12 0:22 ` [PATCH v3 07/15] drm/msm/dpu: add dpu_hw_cdm abstraction for CDM block Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 09/15] drm/msm/dpu: add support to allocate CDM from RM Abhinav Kumar
` (6 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm, linux-kernel
Add the RM APIs necessary to initialize and allocate CDM
blocks to be used by the rest of the DPU pipeline.
changes in v2:
- treat cdm_init() failure as fatal
- fixed the commit text
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 13 +++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 ++
2 files changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 0bb28cf4a6cb..7ed476b96304 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -8,6 +8,7 @@
#include "dpu_kms.h"
#include "dpu_hw_lm.h"
#include "dpu_hw_ctl.h"
+#include "dpu_hw_cdm.h"
#include "dpu_hw_pingpong.h"
#include "dpu_hw_sspp.h"
#include "dpu_hw_intf.h"
@@ -176,6 +177,18 @@ int dpu_rm_init(struct drm_device *dev,
rm->hw_sspp[sspp->id - SSPP_NONE] = hw;
}
+ if (cat->cdm) {
+ struct dpu_hw_cdm *hw;
+
+ hw = dpu_hw_cdm_init(dev, cat->cdm, mmio, cat->mdss_ver);
+ if (IS_ERR(hw)) {
+ rc = PTR_ERR(hw);
+ DPU_ERROR("failed cdm object creation: err %d\n", rc);
+ goto fail;
+ }
+ rm->cdm_blk = &hw->base;
+ }
+
return 0;
fail:
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
index 36752d837be4..e3f83ebc656b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
@@ -22,6 +22,7 @@ struct dpu_global_state;
* @hw_wb: array of wb hardware resources
* @dspp_blks: array of dspp hardware resources
* @hw_sspp: array of sspp hardware resources
+ * @cdm_blk: cdm hardware resource
*/
struct dpu_rm {
struct dpu_hw_blk *pingpong_blks[PINGPONG_MAX - PINGPONG_0];
@@ -33,6 +34,7 @@ struct dpu_rm {
struct dpu_hw_blk *merge_3d_blks[MERGE_3D_MAX - MERGE_3D_0];
struct dpu_hw_blk *dsc_blks[DSC_MAX - DSC_0];
struct dpu_hw_sspp *hw_sspp[SSPP_MAX - SSPP_NONE];
+ struct dpu_hw_blk *cdm_blk;
};
/**
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 09/15] drm/msm/dpu: add support to allocate CDM from RM
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
` (7 preceding siblings ...)
2023-12-12 0:22 ` [PATCH v3 08/15] drm/msm/dpu: add cdm blocks to RM Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 10/15] drm/msm/dpu: add CDM related logic to dpu_hw_ctl layer Abhinav Kumar
` (5 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm, linux-kernel
Even though there is usually only one CDM block, it can be
used by either HDMI, DisplayPort OR Writeback interfaces.
Hence its allocation needs to be tracked properly by the
resource manager to ensure appropriate availability of the
block.
changes in v2:
- move needs_cdm to topology struct
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 38 +++++++++++++++++++--
drivers/gpu/drm/msm/msm_drv.h | 2 ++
4 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
index 9db4cf61bd29..5df545904057 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
@@ -98,6 +98,7 @@ enum dpu_hw_blk_type {
DPU_HW_BLK_DSPP,
DPU_HW_BLK_MERGE_3D,
DPU_HW_BLK_DSC,
+ DPU_HW_BLK_CDM,
DPU_HW_BLK_MAX,
};
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index df6271017b80..a0cd36e45a01 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -135,6 +135,7 @@ struct dpu_global_state {
uint32_t ctl_to_enc_id[CTL_MAX - CTL_0];
uint32_t dspp_to_enc_id[DSPP_MAX - DSPP_0];
uint32_t dsc_to_enc_id[DSC_MAX - DSC_0];
+ uint32_t cdm_to_enc_id;
};
struct dpu_global_state
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 7ed476b96304..b58a9c2ae326 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -435,6 +435,26 @@ static int _dpu_rm_reserve_dsc(struct dpu_rm *rm,
return 0;
}
+static int _dpu_rm_reserve_cdm(struct dpu_rm *rm,
+ struct dpu_global_state *global_state,
+ struct drm_encoder *enc)
+{
+ /* try allocating only one CDM block */
+ if (!rm->cdm_blk) {
+ DPU_ERROR("CDM block does not exist\n");
+ return -EIO;
+ }
+
+ if (global_state->cdm_to_enc_id) {
+ DPU_ERROR("CDM_0 is already allocated\n");
+ return -EIO;
+ }
+
+ global_state->cdm_to_enc_id = enc->base.id;
+
+ return 0;
+}
+
static int _dpu_rm_make_reservation(
struct dpu_rm *rm,
struct dpu_global_state *global_state,
@@ -460,6 +480,14 @@ static int _dpu_rm_make_reservation(
if (ret)
return ret;
+ if (reqs->topology.needs_cdm) {
+ ret = _dpu_rm_reserve_cdm(rm, global_state, enc);
+ if (ret) {
+ DPU_ERROR("unable to find CDM blk\n");
+ return ret;
+ }
+ }
+
return ret;
}
@@ -470,9 +498,9 @@ static int _dpu_rm_populate_requirements(
{
reqs->topology = req_topology;
- DRM_DEBUG_KMS("num_lm: %d num_dsc: %d num_intf: %d\n",
+ DRM_DEBUG_KMS("num_lm: %d num_dsc: %d num_intf: %d cdm: %d\n",
reqs->topology.num_lm, reqs->topology.num_dsc,
- reqs->topology.num_intf);
+ reqs->topology.num_intf, reqs->topology.needs_cdm);
return 0;
}
@@ -501,6 +529,7 @@ void dpu_rm_release(struct dpu_global_state *global_state,
ARRAY_SIZE(global_state->dsc_to_enc_id), enc->base.id);
_dpu_rm_clear_mapping(global_state->dspp_to_enc_id,
ARRAY_SIZE(global_state->dspp_to_enc_id), enc->base.id);
+ _dpu_rm_clear_mapping(&global_state->cdm_to_enc_id, 1, enc->base.id);
}
int dpu_rm_reserve(
@@ -574,6 +603,11 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
hw_to_enc_id = global_state->dsc_to_enc_id;
max_blks = ARRAY_SIZE(rm->dsc_blks);
break;
+ case DPU_HW_BLK_CDM:
+ hw_blks = &rm->cdm_blk;
+ hw_to_enc_id = &global_state->cdm_to_enc_id;
+ max_blks = 1;
+ break;
default:
DPU_ERROR("blk type %d not managed by rm\n", type);
return 0;
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index c0446fa66b98..16a7cbc0b7dd 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -90,12 +90,14 @@ enum msm_event_wait {
* @num_intf: number of interfaces the panel is mounted on
* @num_dspp: number of dspp blocks used
* @num_dsc: number of Display Stream Compression (DSC) blocks used
+ * @needs_cdm: indicates whether cdm block is needed for this display topology
*/
struct msm_display_topology {
u32 num_lm;
u32 num_intf;
u32 num_dspp;
u32 num_dsc;
+ bool needs_cdm;
};
/* Commit/Event thread specific structure */
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 10/15] drm/msm/dpu: add CDM related logic to dpu_hw_ctl layer
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
` (8 preceding siblings ...)
2023-12-12 0:22 ` [PATCH v3 09/15] drm/msm/dpu: add support to allocate CDM from RM Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 11/15] drm/msm/dpu: add an API to setup the CDM block for writeback Abhinav Kumar
` (4 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
kernel test robot, linux-kernel
CDM block will need its own logic to program the flush and active
bits in the dpu_hw_ctl layer.
Make necessary changes in dpu_hw_ctl to support CDM programming.
changes in v3:
- drop unused cdm_active as reported by kbot
- retained the R-b as its a trivial change
changes in v2:
- remove unused empty line
- pass in cdm_num to update_pending_flush_cdm()
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202312102047.S0I69pCs-lkp@intel.com/
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 33 ++++++++++++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 12 ++++++++
2 files changed, 45 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index e7b680a151d6..e76565c3e6a4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -32,11 +32,13 @@
#define CTL_DSC_ACTIVE 0x0E8
#define CTL_WB_ACTIVE 0x0EC
#define CTL_INTF_ACTIVE 0x0F4
+#define CTL_CDM_ACTIVE 0x0F8
#define CTL_FETCH_PIPE_ACTIVE 0x0FC
#define CTL_MERGE_3D_FLUSH 0x100
#define CTL_DSC_FLUSH 0x104
#define CTL_WB_FLUSH 0x108
#define CTL_INTF_FLUSH 0x110
+#define CTL_CDM_FLUSH 0x114
#define CTL_INTF_MASTER 0x134
#define CTL_DSPP_n_FLUSH(n) ((0x13C) + ((n) * 4))
@@ -46,6 +48,7 @@
#define DPU_REG_RESET_TIMEOUT_US 2000
#define MERGE_3D_IDX 23
#define DSC_IDX 22
+#define CDM_IDX 26
#define INTF_IDX 31
#define WB_IDX 16
#define DSPP_IDX 29 /* From DPU hw rev 7.x.x */
@@ -107,6 +110,7 @@ static inline void dpu_hw_ctl_clear_pending_flush(struct dpu_hw_ctl *ctx)
ctx->pending_wb_flush_mask = 0;
ctx->pending_merge_3d_flush_mask = 0;
ctx->pending_dsc_flush_mask = 0;
+ ctx->pending_cdm_flush_mask = 0;
memset(ctx->pending_dspp_flush_mask, 0,
sizeof(ctx->pending_dspp_flush_mask));
@@ -151,6 +155,10 @@ static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH,
ctx->pending_dsc_flush_mask);
+ if (ctx->pending_flush_mask & BIT(CDM_IDX))
+ DPU_REG_WRITE(&ctx->hw, CTL_CDM_FLUSH,
+ ctx->pending_cdm_flush_mask);
+
DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
}
@@ -282,6 +290,13 @@ static void dpu_hw_ctl_update_pending_flush_wb(struct dpu_hw_ctl *ctx,
}
}
+static void dpu_hw_ctl_update_pending_flush_cdm(struct dpu_hw_ctl *ctx, enum dpu_cdm cdm_num)
+{
+ /* update pending flush only if CDM_0 is flushed */
+ if (cdm_num == CDM_0)
+ ctx->pending_flush_mask |= BIT(CDM_IDX);
+}
+
static void dpu_hw_ctl_update_pending_flush_wb_v1(struct dpu_hw_ctl *ctx,
enum dpu_wb wb)
{
@@ -310,6 +325,12 @@ static void dpu_hw_ctl_update_pending_flush_dsc_v1(struct dpu_hw_ctl *ctx,
ctx->pending_flush_mask |= BIT(DSC_IDX);
}
+static void dpu_hw_ctl_update_pending_flush_cdm_v1(struct dpu_hw_ctl *ctx, enum dpu_cdm cdm_num)
+{
+ ctx->pending_cdm_flush_mask |= BIT(cdm_num - CDM_0);
+ ctx->pending_flush_mask |= BIT(CDM_IDX);
+}
+
static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx,
enum dpu_dspp dspp, u32 dspp_sub_blk)
{
@@ -543,6 +564,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
if (cfg->dsc)
DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
+
+ if (cfg->cdm)
+ DPU_REG_WRITE(c, CTL_CDM_ACTIVE, cfg->cdm);
}
static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
@@ -586,6 +610,7 @@ static void dpu_hw_ctl_reset_intf_cfg_v1(struct dpu_hw_ctl *ctx,
u32 wb_active = 0;
u32 merge3d_active = 0;
u32 dsc_active;
+ u32 cdm_active;
/*
* This API resets each portion of the CTL path namely,
@@ -621,6 +646,12 @@ static void dpu_hw_ctl_reset_intf_cfg_v1(struct dpu_hw_ctl *ctx,
dsc_active &= ~cfg->dsc;
DPU_REG_WRITE(c, CTL_DSC_ACTIVE, dsc_active);
}
+
+ if (cfg->cdm) {
+ cdm_active = DPU_REG_READ(c, CTL_CDM_ACTIVE);
+ cdm_active &= ~cfg->cdm;
+ DPU_REG_WRITE(c, CTL_CDM_ACTIVE, cdm_active);
+ }
}
static void dpu_hw_ctl_set_fetch_pipe_active(struct dpu_hw_ctl *ctx,
@@ -654,12 +685,14 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops,
ops->update_pending_flush_wb = dpu_hw_ctl_update_pending_flush_wb_v1;
ops->update_pending_flush_dsc =
dpu_hw_ctl_update_pending_flush_dsc_v1;
+ ops->update_pending_flush_cdm = dpu_hw_ctl_update_pending_flush_cdm_v1;
} else {
ops->trigger_flush = dpu_hw_ctl_trigger_flush;
ops->setup_intf_cfg = dpu_hw_ctl_intf_cfg;
ops->update_pending_flush_intf =
dpu_hw_ctl_update_pending_flush_intf;
ops->update_pending_flush_wb = dpu_hw_ctl_update_pending_flush_wb;
+ ops->update_pending_flush_cdm = dpu_hw_ctl_update_pending_flush_cdm;
}
ops->clear_pending_flush = dpu_hw_ctl_clear_pending_flush;
ops->update_pending_flush = dpu_hw_ctl_update_pending_flush;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
index 279ebd8dfbff..ff85b5ee0acf 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
@@ -39,6 +39,7 @@ struct dpu_hw_stage_cfg {
* @mode_3d: 3d mux configuration
* @merge_3d: 3d merge block used
* @intf_mode_sel: Interface mode, cmd / vid
+ * @cdm: CDM block used
* @stream_sel: Stream selection for multi-stream interfaces
* @dsc: DSC BIT masks used
*/
@@ -48,6 +49,7 @@ struct dpu_hw_intf_cfg {
enum dpu_3d_blend_mode mode_3d;
enum dpu_merge_3d merge_3d;
enum dpu_ctl_mode_sel intf_mode_sel;
+ enum dpu_cdm cdm;
int stream_sel;
unsigned int dsc;
};
@@ -166,6 +168,14 @@ struct dpu_hw_ctl_ops {
void (*update_pending_flush_dsc)(struct dpu_hw_ctl *ctx,
enum dpu_dsc blk);
+ /**
+ * OR in the given flushbits to the cached pending_(cdm_)flush_mask
+ * No effect on hardware
+ * @ctx: ctl path ctx pointer
+ * @cdm_num: idx of cdm to be flushed
+ */
+ void (*update_pending_flush_cdm)(struct dpu_hw_ctl *ctx, enum dpu_cdm cdm_num);
+
/**
* Write the value of the pending_flush_mask to hardware
* @ctx : ctl path ctx pointer
@@ -239,6 +249,7 @@ struct dpu_hw_ctl_ops {
* @pending_intf_flush_mask: pending INTF flush
* @pending_wb_flush_mask: pending WB flush
* @pending_dsc_flush_mask: pending DSC flush
+ * @pending_cdm_flush_mask: pending CDM flush
* @ops: operation list
*/
struct dpu_hw_ctl {
@@ -256,6 +267,7 @@ struct dpu_hw_ctl {
u32 pending_merge_3d_flush_mask;
u32 pending_dspp_flush_mask[DSPP_MAX - DSPP_0];
u32 pending_dsc_flush_mask;
+ u32 pending_cdm_flush_mask;
/* ops */
struct dpu_hw_ctl_ops ops;
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 11/15] drm/msm/dpu: add an API to setup the CDM block for writeback
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
` (9 preceding siblings ...)
2023-12-12 0:22 ` [PATCH v3 10/15] drm/msm/dpu: add CDM related logic to dpu_hw_ctl layer Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 7:17 ` Dmitry Baryshkov
2023-12-12 0:22 ` [PATCH v3 12/15] drm/msm/dpu: plug-in the cdm related bits to writeback setup Abhinav Kumar
` (3 subsequent siblings)
14 siblings, 1 reply; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
kernel test robot, linux-kernel
Add an API dpu_encoder_helper_phys_setup_cdm() which can be used by
the writeback encoder to setup the CDM block.
Currently, this is defined and used within the writeback's physical
encoder layer however, the function can be modified to be used to setup
the CDM block even for non-writeback interfaces.
Until those modifications are planned and made, keep it local to
writeback.
changes in v3:
- call bind_pingpong_blk() directly as disable() is dropped
- add dpu_csc10_rgb2yuv_601l to dpu_hw_util.h and use it
- fix kbot error on the function doc
- document that dpu_encoder_helper_phys_setup_cdm() doesn't handle
DPU_CHROMA_H1V2
changes in v2:
- add the RGB2YUV CSC matrix to dpu util as needed by CDM
- use dpu_hw_get_csc_cfg() to get and program CSC
- drop usage of setup_csc_data() and setup_cdwn() cdm ops
as they both have been merged into enable()
- drop reduntant hw_cdm and hw_pp checks
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202312102149.qmbCdsg2-lkp@intel.com/
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 6 ++
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 93 ++++++++++++++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 14 +++
3 files changed, 112 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index e2934a6702d1..bdb89cbbcfb8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -14,8 +14,10 @@
#include "dpu_hw_intf.h"
#include "dpu_hw_wb.h"
#include "dpu_hw_pingpong.h"
+#include "dpu_hw_cdm.h"
#include "dpu_hw_ctl.h"
#include "dpu_hw_top.h"
+#include "dpu_hw_util.h"
#include "dpu_encoder.h"
#include "dpu_crtc.h"
@@ -150,6 +152,7 @@ enum dpu_intr_idx {
* @hw_pp: Hardware interface to the ping pong registers
* @hw_intf: Hardware interface to the intf registers
* @hw_wb: Hardware interface to the wb registers
+ * @hw_cdm: Hardware interface to the CDM registers
* @dpu_kms: Pointer to the dpu_kms top level
* @cached_mode: DRM mode cached at mode_set time, acted on in enable
* @enabled: Whether the encoder has enabled and running a mode
@@ -178,6 +181,7 @@ struct dpu_encoder_phys {
struct dpu_hw_pingpong *hw_pp;
struct dpu_hw_intf *hw_intf;
struct dpu_hw_wb *hw_wb;
+ struct dpu_hw_cdm *hw_cdm;
struct dpu_kms *dpu_kms;
struct drm_display_mode cached_mode;
enum dpu_enc_split_role split_role;
@@ -207,6 +211,7 @@ static inline int dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
* @wbirq_refcount: Reference count of writeback interrupt
* @wb_done_timeout_cnt: number of wb done irq timeout errors
* @wb_cfg: writeback block config to store fb related details
+ * @cdm_cfg: cdm block config needed to store writeback block's CDM configuration
* @wb_conn: backpointer to writeback connector
* @wb_job: backpointer to current writeback job
* @dest: dpu buffer layout for current writeback output buffer
@@ -216,6 +221,7 @@ struct dpu_encoder_phys_wb {
atomic_t wbirq_refcount;
int wb_done_timeout_cnt;
struct dpu_hw_wb_cfg wb_cfg;
+ struct dpu_hw_cdm_cfg cdm_cfg;
struct drm_writeback_connector *wb_conn;
struct drm_writeback_job *wb_job;
struct dpu_hw_fmt_layout dest;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 8f05f2a1fc24..85cb8596737d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -259,6 +259,96 @@ static void dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
}
}
+/**
+ * dpu_encoder_helper_phys_setup_cdm - setup chroma down sampling block
+ * This API does not handle DPU_CHROMA_H1V2.
+ * @phys_enc:Pointer to physical encoder
+ */
+static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
+{
+ struct dpu_hw_cdm *hw_cdm;
+ struct dpu_hw_cdm_cfg *cdm_cfg;
+ struct dpu_hw_pingpong *hw_pp;
+ struct dpu_encoder_phys_wb *wb_enc;
+ const struct msm_format *format;
+ const struct dpu_format *dpu_fmt;
+ struct drm_writeback_job *wb_job;
+ int ret;
+
+ if (!phys_enc)
+ return;
+
+ wb_enc = to_dpu_encoder_phys_wb(phys_enc);
+ cdm_cfg = &wb_enc->cdm_cfg;
+ hw_pp = phys_enc->hw_pp;
+ hw_cdm = phys_enc->hw_cdm;
+ wb_job = wb_enc->wb_job;
+
+ format = msm_framebuffer_format(wb_enc->wb_job->fb);
+ dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format, wb_job->fb->modifier);
+
+ if (!hw_cdm)
+ return;
+
+ if (!DPU_FORMAT_IS_YUV(dpu_fmt)) {
+ DPU_DEBUG("[enc:%d] cdm_disable fmt:%x\n", DRMID(phys_enc->parent),
+ dpu_fmt->base.pixel_format);
+ if (hw_cdm->ops.bind_pingpong_blk)
+ hw_cdm->ops.bind_pingpong_blk(hw_cdm, PINGPONG_NONE);
+
+ return;
+ }
+
+ memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
+
+ cdm_cfg->output_width = wb_job->fb->width;
+ cdm_cfg->output_height = wb_job->fb->height;
+ cdm_cfg->output_fmt = dpu_fmt;
+ cdm_cfg->output_type = CDM_CDWN_OUTPUT_WB;
+ cdm_cfg->output_bit_depth = DPU_FORMAT_IS_DX(dpu_fmt) ?
+ CDM_CDWN_OUTPUT_10BIT : CDM_CDWN_OUTPUT_8BIT;
+ cdm_cfg->csc_cfg = &dpu_csc10_rgb2yuv_601l;
+
+ /* enable 10 bit logic */
+ switch (cdm_cfg->output_fmt->chroma_sample) {
+ case DPU_CHROMA_RGB:
+ cdm_cfg->h_cdwn_type = CDM_CDWN_DISABLE;
+ cdm_cfg->v_cdwn_type = CDM_CDWN_DISABLE;
+ break;
+ case DPU_CHROMA_H2V1:
+ cdm_cfg->h_cdwn_type = CDM_CDWN_COSITE;
+ cdm_cfg->v_cdwn_type = CDM_CDWN_DISABLE;
+ break;
+ case DPU_CHROMA_420:
+ cdm_cfg->h_cdwn_type = CDM_CDWN_COSITE;
+ cdm_cfg->v_cdwn_type = CDM_CDWN_OFFSITE;
+ break;
+ case DPU_CHROMA_H1V2:
+ default:
+ DPU_ERROR("[enc:%d] unsupported chroma sampling type\n",
+ DRMID(phys_enc->parent));
+ cdm_cfg->h_cdwn_type = CDM_CDWN_DISABLE;
+ cdm_cfg->v_cdwn_type = CDM_CDWN_DISABLE;
+ break;
+ }
+
+ DPU_DEBUG("[enc:%d] cdm_enable:%d,%d,%X,%d,%d,%d,%d]\n",
+ DRMID(phys_enc->parent), cdm_cfg->output_width,
+ cdm_cfg->output_height, cdm_cfg->output_fmt->base.pixel_format,
+ cdm_cfg->output_type, cdm_cfg->output_bit_depth,
+ cdm_cfg->h_cdwn_type, cdm_cfg->v_cdwn_type);
+
+ if (hw_cdm->ops.enable) {
+ cdm_cfg->pp_id = hw_pp->idx;
+ ret = hw_cdm->ops.enable(hw_cdm, cdm_cfg);
+ if (ret < 0) {
+ DPU_ERROR("[enc:%d] failed to enable CDM; ret:%d\n",
+ DRMID(phys_enc->parent), ret);
+ return;
+ }
+ }
+}
+
/**
* dpu_encoder_phys_wb_atomic_check - verify and fixup given atomic states
* @phys_enc: Pointer to physical encoder
@@ -382,8 +472,9 @@ static void dpu_encoder_phys_wb_setup(
dpu_encoder_phys_wb_setup_fb(phys_enc, fb);
- dpu_encoder_phys_wb_setup_ctl(phys_enc);
+ dpu_encoder_helper_phys_setup_cdm(phys_enc);
+ dpu_encoder_phys_wb_setup_ctl(phys_enc);
}
/**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
index aa50005042d1..c0aaad2023da 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
@@ -19,6 +19,8 @@
#define MISR_CTRL_STATUS_CLEAR BIT(10)
#define MISR_CTRL_FREE_RUN_MASK BIT(31)
+#define TO_S15D16(_x_)((_x_) << 7)
+
static const struct dpu_csc_cfg dpu_csc_YUV2RGB_601L = {
{
/* S15.16 format */
@@ -49,6 +51,18 @@ static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
{ 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
};
+static const struct dpu_csc_cfg dpu_csc10_rgb2yuv_601l = {
+ {
+ TO_S15D16(0x0083), TO_S15D16(0x0102), TO_S15D16(0x0032),
+ TO_S15D16(0x1fb5), TO_S15D16(0x1f6c), TO_S15D16(0x00e1),
+ TO_S15D16(0x00e1), TO_S15D16(0x1f45), TO_S15D16(0x1fdc)
+ },
+ { 0x00, 0x00, 0x00 },
+ { 0x0040, 0x0200, 0x0200 },
+ { 0x000, 0x3ff, 0x000, 0x3ff, 0x000, 0x3ff },
+ { 0x040, 0x3ac, 0x040, 0x3c0, 0x040, 0x3c0 },
+};
+
/*
* This is the common struct maintained by each sub block
* for mapping the register offsets in this block to the
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 12/15] drm/msm/dpu: plug-in the cdm related bits to writeback setup
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
` (10 preceding siblings ...)
2023-12-12 0:22 ` [PATCH v3 11/15] drm/msm/dpu: add an API to setup the CDM block for writeback Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 13/15] drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output Abhinav Kumar
` (2 subsequent siblings)
14 siblings, 0 replies; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm, linux-kernel
To setup and enable CDM block for the writeback pipeline, lets
add the pieces together to set the active bits and the flush
bits for the CDM block.
changes in v2:
- passed the cdm idx to update_pending_flush_cdm()
(have retained the R-b as its a minor change)
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 85cb8596737d..0a28198886d5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -214,6 +214,7 @@ static void dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
{
struct dpu_hw_wb *hw_wb;
struct dpu_hw_ctl *ctl;
+ struct dpu_hw_cdm *hw_cdm;
if (!phys_enc) {
DPU_ERROR("invalid encoder\n");
@@ -222,6 +223,7 @@ static void dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
hw_wb = phys_enc->hw_wb;
ctl = phys_enc->hw_ctl;
+ hw_cdm = phys_enc->hw_cdm;
if (test_bit(DPU_CTL_ACTIVE_CFG, &ctl->caps->features) &&
(phys_enc->hw_ctl &&
@@ -238,6 +240,9 @@ static void dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
if (mode_3d && hw_pp && hw_pp->merge_3d)
intf_cfg.merge_3d = hw_pp->merge_3d->idx;
+ if (hw_cdm)
+ intf_cfg.cdm = hw_cdm->idx;
+
if (phys_enc->hw_pp->merge_3d && phys_enc->hw_pp->merge_3d->ops.setup_3d_mode)
phys_enc->hw_pp->merge_3d->ops.setup_3d_mode(phys_enc->hw_pp->merge_3d,
mode_3d);
@@ -418,6 +423,7 @@ static void _dpu_encoder_phys_wb_update_flush(struct dpu_encoder_phys *phys_enc)
struct dpu_hw_wb *hw_wb;
struct dpu_hw_ctl *hw_ctl;
struct dpu_hw_pingpong *hw_pp;
+ struct dpu_hw_cdm *hw_cdm;
u32 pending_flush = 0;
if (!phys_enc)
@@ -426,6 +432,7 @@ static void _dpu_encoder_phys_wb_update_flush(struct dpu_encoder_phys *phys_enc)
hw_wb = phys_enc->hw_wb;
hw_pp = phys_enc->hw_pp;
hw_ctl = phys_enc->hw_ctl;
+ hw_cdm = phys_enc->hw_cdm;
DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
@@ -441,6 +448,9 @@ static void _dpu_encoder_phys_wb_update_flush(struct dpu_encoder_phys *phys_enc)
hw_ctl->ops.update_pending_flush_merge_3d(hw_ctl,
hw_pp->merge_3d->idx);
+ if (hw_cdm && hw_ctl->ops.update_pending_flush_cdm)
+ hw_ctl->ops.update_pending_flush_cdm(hw_ctl, hw_cdm->idx);
+
if (hw_ctl->ops.get_pending_flush)
pending_flush = hw_ctl->ops.get_pending_flush(hw_ctl);
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 13/15] drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
` (11 preceding siblings ...)
2023-12-12 0:22 ` [PATCH v3 12/15] drm/msm/dpu: plug-in the cdm related bits to writeback setup Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 7:21 ` Dmitry Baryshkov
2023-12-12 0:22 ` [PATCH v3 14/15] drm/msm/dpu: introduce separate wb2_format arrays for rgb and yuv Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 15/15] drm/msm/dpu: add cdm blocks to dpu snapshot Abhinav Kumar
14 siblings, 1 reply; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm, linux-kernel
Reserve CDM blocks for writeback if the format of the output fb
is YUV. At the moment, the reservation is done only for writeback
but can easily be extended by relaxing the checks once other
interfaces are ready to output YUV.
changes in v3:
- squash CDM disable during encoder cleanup into this change
changes in v2:
- use needs_cdm from topology struct
- drop fb related checks from atomic_mode_set()
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 37 +++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 889e9bb42715..989ee8c0e5b4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -16,6 +16,7 @@
#include <drm/drm_crtc.h>
#include <drm/drm_file.h>
#include <drm/drm_probe_helper.h>
+#include <drm/drm_framebuffer.h>
#include "msm_drv.h"
#include "dpu_kms.h"
@@ -26,6 +27,7 @@
#include "dpu_hw_dspp.h"
#include "dpu_hw_dsc.h"
#include "dpu_hw_merge3d.h"
+#include "dpu_hw_cdm.h"
#include "dpu_formats.h"
#include "dpu_encoder_phys.h"
#include "dpu_crtc.h"
@@ -582,6 +584,7 @@ static int dpu_encoder_virt_atomic_check(
struct drm_display_mode *adj_mode;
struct msm_display_topology topology;
struct dpu_global_state *global_state;
+ struct drm_framebuffer *fb;
struct drm_dsc_config *dsc;
int i = 0;
int ret = 0;
@@ -622,6 +625,22 @@ static int dpu_encoder_virt_atomic_check(
topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
+ /*
+ * Use CDM only for writeback at the moment as other interfaces cannot handle it.
+ * if writeback itself cannot handle cdm for some reason it will fail in its atomic_check()
+ * earlier.
+ */
+ if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
+ fb = conn_state->writeback_job->fb;
+
+ if (fb && DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(fb))))
+ topology.needs_cdm = true;
+ if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
+ crtc_state->mode_changed = true;
+ else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
+ crtc_state->mode_changed = true;
+ }
+
/*
* Release and Allocate resources on every modeset
* Dont allocate when active is false.
@@ -1062,6 +1081,15 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
dpu_enc->dsc_mask = dsc_mask;
+ if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
+ struct dpu_hw_blk *hw_cdm = NULL;
+
+ dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
+ drm_enc->base.id, DPU_HW_BLK_CDM,
+ &hw_cdm, 1);
+ dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL;
+ }
+
cstate = to_dpu_crtc_state(crtc_state);
for (i = 0; i < num_lm; i++) {
@@ -2050,6 +2078,15 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
phys_enc->hw_pp->merge_3d->idx);
}
+ if (phys_enc->hw_cdm) {
+ if (phys_enc->hw_cdm->ops.bind_pingpong_blk && phys_enc->hw_pp)
+ phys_enc->hw_cdm->ops.bind_pingpong_blk(phys_enc->hw_cdm,
+ PINGPONG_NONE);
+ if (phys_enc->hw_ctl->ops.update_pending_flush_cdm)
+ phys_enc->hw_ctl->ops.update_pending_flush_cdm(phys_enc->hw_ctl,
+ phys_enc->hw_cdm->idx);
+ }
+
if (dpu_enc->dsc) {
dpu_encoder_unprep_dsc(dpu_enc);
dpu_enc->dsc = NULL;
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 14/15] drm/msm/dpu: introduce separate wb2_format arrays for rgb and yuv
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
` (12 preceding siblings ...)
2023-12-12 0:22 ` [PATCH v3 13/15] drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
2023-12-12 7:22 ` Dmitry Baryshkov
2023-12-12 0:22 ` [PATCH v3 15/15] drm/msm/dpu: add cdm blocks to dpu snapshot Abhinav Kumar
14 siblings, 1 reply; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm, linux-kernel
Lets rename the existing wb2_formats array wb2_formats_rgb to indicate
that it has only RGB formats and can be used on any chipset having a WB
block.
Introduce a new wb2_formats_rgb_yuv array to the catalog to
indicate support for YUV formats to writeback in addition to RGB.
Chipsets which have support for CDM block will use the newly added
wb2_formats_rgb_yuv array.
changes in v3:
- change type of wb2_formats_rgb/wb2_formats_rgb_yuv to u32
to fix checkpatch warnings
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
.../msm/disp/dpu1/catalog/dpu_10_0_sm8650.h | 4 +-
.../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 4 +-
.../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h | 4 +-
.../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 4 +-
.../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 4 +-
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 37 ++++++++++++++++++-
6 files changed, 46 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_10_0_sm8650.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_10_0_sm8650.h
index 04d2a73dd942..eb5dfff2ec4f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_10_0_sm8650.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_10_0_sm8650.h
@@ -341,8 +341,8 @@ static const struct dpu_wb_cfg sm8650_wb[] = {
.name = "wb_2", .id = WB_2,
.base = 0x65000, .len = 0x2c8,
.features = WB_SM8250_MASK,
- .format_list = wb2_formats,
- .num_formats = ARRAY_SIZE(wb2_formats),
+ .format_list = wb2_formats_rgb,
+ .num_formats = ARRAY_SIZE(wb2_formats_rgb),
.xin_id = 6,
.vbif_idx = VBIF_RT,
.maxlinewidth = 4096,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
index 58b0f50518c8..a57d50b1f028 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
@@ -336,8 +336,8 @@ static const struct dpu_wb_cfg sm8250_wb[] = {
.name = "wb_2", .id = WB_2,
.base = 0x65000, .len = 0x2c8,
.features = WB_SM8250_MASK,
- .format_list = wb2_formats,
- .num_formats = ARRAY_SIZE(wb2_formats),
+ .format_list = wb2_formats_rgb_yuv,
+ .num_formats = ARRAY_SIZE(wb2_formats_rgb_yuv),
.clk_ctrl = DPU_CLK_CTRL_WB2,
.xin_id = 6,
.vbif_idx = VBIF_RT,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
index bcfedfc8251a..7382ebb6e5b2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
@@ -157,8 +157,8 @@ static const struct dpu_wb_cfg sc7180_wb[] = {
.name = "wb_2", .id = WB_2,
.base = 0x65000, .len = 0x2c8,
.features = WB_SM8250_MASK,
- .format_list = wb2_formats,
- .num_formats = ARRAY_SIZE(wb2_formats),
+ .format_list = wb2_formats_rgb,
+ .num_formats = ARRAY_SIZE(wb2_formats_rgb),
.clk_ctrl = DPU_CLK_CTRL_WB2,
.xin_id = 6,
.vbif_idx = VBIF_RT,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
index 19c2b7454796..2f153e0b5c6a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
@@ -169,8 +169,8 @@ static const struct dpu_wb_cfg sc7280_wb[] = {
.name = "wb_2", .id = WB_2,
.base = 0x65000, .len = 0x2c8,
.features = WB_SM8250_MASK,
- .format_list = wb2_formats,
- .num_formats = ARRAY_SIZE(wb2_formats),
+ .format_list = wb2_formats_rgb_yuv,
+ .num_formats = ARRAY_SIZE(wb2_formats_rgb_yuv),
.clk_ctrl = DPU_CLK_CTRL_WB2,
.xin_id = 6,
.vbif_idx = VBIF_RT,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
index bf56265967c0..ad48defa154f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
@@ -315,8 +315,8 @@ static const struct dpu_wb_cfg sm8550_wb[] = {
.name = "wb_2", .id = WB_2,
.base = 0x65000, .len = 0x2c8,
.features = WB_SM8250_MASK,
- .format_list = wb2_formats,
- .num_formats = ARRAY_SIZE(wb2_formats),
+ .format_list = wb2_formats_rgb,
+ .num_formats = ARRAY_SIZE(wb2_formats_rgb),
.xin_id = 6,
.vbif_idx = VBIF_RT,
.maxlinewidth = 4096,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index b304bebedb84..54e8717403a0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -202,7 +202,7 @@ static const u32 rotation_v2_formats[] = {
/* TODO add formats after validation */
};
-static const uint32_t wb2_formats[] = {
+static const u32 wb2_formats_rgb[] = {
DRM_FORMAT_RGB565,
DRM_FORMAT_BGR565,
DRM_FORMAT_RGB888,
@@ -236,6 +236,41 @@ static const uint32_t wb2_formats[] = {
DRM_FORMAT_XBGR4444,
};
+static const u32 wb2_formats_rgb_yuv[] = {
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_BGR565,
+ DRM_FORMAT_RGB888,
+ DRM_FORMAT_ARGB8888,
+ DRM_FORMAT_RGBA8888,
+ DRM_FORMAT_ABGR8888,
+ DRM_FORMAT_XRGB8888,
+ DRM_FORMAT_RGBX8888,
+ DRM_FORMAT_XBGR8888,
+ DRM_FORMAT_ARGB1555,
+ DRM_FORMAT_RGBA5551,
+ DRM_FORMAT_XRGB1555,
+ DRM_FORMAT_RGBX5551,
+ DRM_FORMAT_ARGB4444,
+ DRM_FORMAT_RGBA4444,
+ DRM_FORMAT_RGBX4444,
+ DRM_FORMAT_XRGB4444,
+ DRM_FORMAT_BGR565,
+ DRM_FORMAT_BGR888,
+ DRM_FORMAT_ABGR8888,
+ DRM_FORMAT_BGRA8888,
+ DRM_FORMAT_BGRX8888,
+ DRM_FORMAT_XBGR8888,
+ DRM_FORMAT_ABGR1555,
+ DRM_FORMAT_BGRA5551,
+ DRM_FORMAT_XBGR1555,
+ DRM_FORMAT_BGRX5551,
+ DRM_FORMAT_ABGR4444,
+ DRM_FORMAT_BGRA4444,
+ DRM_FORMAT_BGRX4444,
+ DRM_FORMAT_XBGR4444,
+ DRM_FORMAT_NV12,
+};
+
/*************************************************************
* SSPP sub blocks config
*************************************************************/
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 15/15] drm/msm/dpu: add cdm blocks to dpu snapshot
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
` (13 preceding siblings ...)
2023-12-12 0:22 ` [PATCH v3 14/15] drm/msm/dpu: introduce separate wb2_format arrays for rgb and yuv Abhinav Kumar
@ 2023-12-12 0:22 ` Abhinav Kumar
14 siblings, 0 replies; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 0:22 UTC (permalink / raw
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, seanpaul, quic_jesszhan, linux-arm-msm, linux-kernel
Now that CDM block support has been added to DPU lets also add its
entry to the DPU snapshot to help debugging.
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index dc24fe4bb3b0..59647ad19906 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -947,6 +947,10 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
}
}
+ if (cat->cdm)
+ msm_disp_snapshot_add_block(disp_state, cat->cdm->len,
+ dpu_kms->mmio + cat->cdm->base, cat->cdm->name);
+
pm_runtime_put_sync(&dpu_kms->pdev->dev);
}
--
2.40.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v3 01/15] drm/msm/dpu: add formats check for writeback encoder
2023-12-12 0:22 ` [PATCH v3 01/15] drm/msm/dpu: add formats check for writeback encoder Abhinav Kumar
@ 2023-12-12 6:40 ` Dmitry Baryshkov
2023-12-12 17:16 ` Abhinav Kumar
0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Baryshkov @ 2023-12-12 6:40 UTC (permalink / raw
To: Abhinav Kumar
Cc: freedreno, Rob Clark, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter, dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
linux-kernel
On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> In preparation for adding more formats to dpu writeback add
> format validation to it to fail any unsupported formats.
>
> changes in v3:
> - rebase on top of msm-next
> - replace drm_atomic_helper_check_wb_encoder_state() with
> drm_atomic_helper_check_wb_connector_state() due to the
> rebase
>
> changes in v2:
> - correct some grammar in the commit text
>
> Fixes: d7d0e73f7de3 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for writeback")
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> index bb94909caa25..425415d45ec1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> @@ -272,6 +272,7 @@ static int dpu_encoder_phys_wb_atomic_check(
> {
> struct drm_framebuffer *fb;
> const struct drm_display_mode *mode = &crtc_state->mode;
> + int ret;
>
> DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
> phys_enc->hw_wb->idx, mode->name, mode->hdisplay, mode->vdisplay);
> @@ -308,6 +309,12 @@ static int dpu_encoder_phys_wb_atomic_check(
> return -EINVAL;
> }
>
> + ret = drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
> + if (ret < 0) {
> + DPU_ERROR("invalid pixel format %p4cc\n", &fb->format->format);
> + return ret;
> + }
There is no guarantee that there will be no other checks added to this
helper. So, I think this message is incorrect. If you wish, you can
promote the level of the message in the helper itself.
On the other hand, we rarely print such messages by default. Most of
the checks use drm_dbg.
> +
> return 0;
> }
>
> --
> 2.40.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 04/15] drm/msm/dpu: move csc matrices to dpu_hw_util
2023-12-12 0:22 ` [PATCH v3 04/15] drm/msm/dpu: move csc matrices to dpu_hw_util Abhinav Kumar
@ 2023-12-12 6:41 ` Dmitry Baryshkov
0 siblings, 0 replies; 25+ messages in thread
From: Dmitry Baryshkov @ 2023-12-12 6:41 UTC (permalink / raw
To: Abhinav Kumar
Cc: freedreno, Rob Clark, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter, dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
linux-kernel
On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Since the type and usage of CSC matrices is spanning across DPU
> lets introduce a helper to the dpu_hw_util to return the CSC
> corresponding to the request type. This will help to add more
> supported CSC types such as the RGB to YUV one which is used in
> the case of CDM.
>
> changes in v3:
> - drop the extra wrapper and export the matrices directly
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 30 ++++++++++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 31 +--------------------
> 2 files changed, 31 insertions(+), 30 deletions(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 11/15] drm/msm/dpu: add an API to setup the CDM block for writeback
2023-12-12 0:22 ` [PATCH v3 11/15] drm/msm/dpu: add an API to setup the CDM block for writeback Abhinav Kumar
@ 2023-12-12 7:17 ` Dmitry Baryshkov
0 siblings, 0 replies; 25+ messages in thread
From: Dmitry Baryshkov @ 2023-12-12 7:17 UTC (permalink / raw
To: Abhinav Kumar
Cc: freedreno, Rob Clark, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter, dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
kernel test robot, linux-kernel
On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Add an API dpu_encoder_helper_phys_setup_cdm() which can be used by
> the writeback encoder to setup the CDM block.
>
> Currently, this is defined and used within the writeback's physical
> encoder layer however, the function can be modified to be used to setup
> the CDM block even for non-writeback interfaces.
>
> Until those modifications are planned and made, keep it local to
> writeback.
>
> changes in v3:
> - call bind_pingpong_blk() directly as disable() is dropped
> - add dpu_csc10_rgb2yuv_601l to dpu_hw_util.h and use it
> - fix kbot error on the function doc
> - document that dpu_encoder_helper_phys_setup_cdm() doesn't handle
> DPU_CHROMA_H1V2
>
> changes in v2:
> - add the RGB2YUV CSC matrix to dpu util as needed by CDM
> - use dpu_hw_get_csc_cfg() to get and program CSC
> - drop usage of setup_csc_data() and setup_cdwn() cdm ops
> as they both have been merged into enable()
> - drop reduntant hw_cdm and hw_pp checks
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202312102149.qmbCdsg2-lkp@intel.com/
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 6 ++
> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 93 ++++++++++++++++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 14 +++
> 3 files changed, 112 insertions(+), 1 deletion(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 13/15] drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output
2023-12-12 0:22 ` [PATCH v3 13/15] drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output Abhinav Kumar
@ 2023-12-12 7:21 ` Dmitry Baryshkov
0 siblings, 0 replies; 25+ messages in thread
From: Dmitry Baryshkov @ 2023-12-12 7:21 UTC (permalink / raw
To: Abhinav Kumar
Cc: freedreno, Rob Clark, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter, dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
linux-kernel
On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Reserve CDM blocks for writeback if the format of the output fb
> is YUV. At the moment, the reservation is done only for writeback
> but can easily be extended by relaxing the checks once other
> interfaces are ready to output YUV.
>
> changes in v3:
> - squash CDM disable during encoder cleanup into this change
>
> changes in v2:
> - use needs_cdm from topology struct
> - drop fb related checks from atomic_mode_set()
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Minor nit below which I should probably handle while applying the patch.
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 37 +++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 889e9bb42715..989ee8c0e5b4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -16,6 +16,7 @@
> #include <drm/drm_crtc.h>
> #include <drm/drm_file.h>
> #include <drm/drm_probe_helper.h>
> +#include <drm/drm_framebuffer.h>
>
> #include "msm_drv.h"
> #include "dpu_kms.h"
> @@ -26,6 +27,7 @@
> #include "dpu_hw_dspp.h"
> #include "dpu_hw_dsc.h"
> #include "dpu_hw_merge3d.h"
> +#include "dpu_hw_cdm.h"
> #include "dpu_formats.h"
> #include "dpu_encoder_phys.h"
> #include "dpu_crtc.h"
> @@ -582,6 +584,7 @@ static int dpu_encoder_virt_atomic_check(
> struct drm_display_mode *adj_mode;
> struct msm_display_topology topology;
> struct dpu_global_state *global_state;
> + struct drm_framebuffer *fb;
> struct drm_dsc_config *dsc;
> int i = 0;
> int ret = 0;
> @@ -622,6 +625,22 @@ static int dpu_encoder_virt_atomic_check(
>
> topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
>
> + /*
> + * Use CDM only for writeback at the moment as other interfaces cannot handle it.
> + * if writeback itself cannot handle cdm for some reason it will fail in its atomic_check()
> + * earlier.
What about s/handle/use/ ?
> + */
> + if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
> + fb = conn_state->writeback_job->fb;
> +
> + if (fb && DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(fb))))
> + topology.needs_cdm = true;
> + if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
> + crtc_state->mode_changed = true;
> + else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> + crtc_state->mode_changed = true;
> + }
> +
> /*
> * Release and Allocate resources on every modeset
> * Dont allocate when active is false.
> @@ -1062,6 +1081,15 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>
> dpu_enc->dsc_mask = dsc_mask;
>
> + if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
> + struct dpu_hw_blk *hw_cdm = NULL;
> +
> + dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> + drm_enc->base.id, DPU_HW_BLK_CDM,
> + &hw_cdm, 1);
> + dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL;
> + }
> +
> cstate = to_dpu_crtc_state(crtc_state);
>
> for (i = 0; i < num_lm; i++) {
> @@ -2050,6 +2078,15 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
> phys_enc->hw_pp->merge_3d->idx);
> }
>
> + if (phys_enc->hw_cdm) {
> + if (phys_enc->hw_cdm->ops.bind_pingpong_blk && phys_enc->hw_pp)
> + phys_enc->hw_cdm->ops.bind_pingpong_blk(phys_enc->hw_cdm,
> + PINGPONG_NONE);
> + if (phys_enc->hw_ctl->ops.update_pending_flush_cdm)
> + phys_enc->hw_ctl->ops.update_pending_flush_cdm(phys_enc->hw_ctl,
> + phys_enc->hw_cdm->idx);
> + }
> +
> if (dpu_enc->dsc) {
> dpu_encoder_unprep_dsc(dpu_enc);
> dpu_enc->dsc = NULL;
> --
> 2.40.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 14/15] drm/msm/dpu: introduce separate wb2_format arrays for rgb and yuv
2023-12-12 0:22 ` [PATCH v3 14/15] drm/msm/dpu: introduce separate wb2_format arrays for rgb and yuv Abhinav Kumar
@ 2023-12-12 7:22 ` Dmitry Baryshkov
0 siblings, 0 replies; 25+ messages in thread
From: Dmitry Baryshkov @ 2023-12-12 7:22 UTC (permalink / raw
To: Abhinav Kumar
Cc: freedreno, Rob Clark, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter, dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
linux-kernel
On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Lets rename the existing wb2_formats array wb2_formats_rgb to indicate
> that it has only RGB formats and can be used on any chipset having a WB
> block.
>
> Introduce a new wb2_formats_rgb_yuv array to the catalog to
> indicate support for YUV formats to writeback in addition to RGB.
>
> Chipsets which have support for CDM block will use the newly added
> wb2_formats_rgb_yuv array.
>
> changes in v3:
> - change type of wb2_formats_rgb/wb2_formats_rgb_yuv to u32
> to fix checkpatch warnings
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> .../msm/disp/dpu1/catalog/dpu_10_0_sm8650.h | 4 +-
> .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 4 +-
> .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h | 4 +-
> .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 4 +-
> .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 4 +-
> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 37 ++++++++++++++++++-
> 6 files changed, 46 insertions(+), 11 deletions(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 07/15] drm/msm/dpu: add dpu_hw_cdm abstraction for CDM block
2023-12-12 0:22 ` [PATCH v3 07/15] drm/msm/dpu: add dpu_hw_cdm abstraction for CDM block Abhinav Kumar
@ 2023-12-12 9:40 ` Dmitry Baryshkov
2023-12-12 17:51 ` Abhinav Kumar
0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Baryshkov @ 2023-12-12 9:40 UTC (permalink / raw
To: Abhinav Kumar
Cc: freedreno, Rob Clark, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter, dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
kernel test robot, linux-kernel
On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> CDM block comes with its own set of registers and operations
> which can be done. In-line with other hardware blocks, this
> change adds the dpu_hw_cdm abstraction for the CDM block.
>
> changes in v3:
> - fix commit text from sub-blk to blk for CDM
> - fix kbot issue for missing static for dpu_hw_cdm_enable()
> - fix kbot issue for incorrect documentation style
> - add more documentation for enums and struct in dpu_hw_cdm.h
> - drop "enable" parameter from bind_pingpong_blk() as we can
> just use PINGPONG_NONE for disable cases
> - drop unnecessary bit operation for zero value of cdm_cfg
>
> changes in v2:
> - replace bit magic with relevant defines
> - use drmm_kzalloc instead of kzalloc/free
> - some formatting fixes
> - inline _setup_cdm_ops()
> - protect bind_pingpong_blk with core_rev check
> - drop setup_csc_data() and setup_cdwn() ops as they
> are merged into enable()
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202312101815.B3ZH7Pfy-lkp@intel.com/
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> drivers/gpu/drm/msm/Makefile | 1 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c | 263 ++++++++++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h | 130 ++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 1 +
> 4 files changed, 395 insertions(+)
> create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
> create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
>
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 49671364fdcf..b1173128b5b9 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -63,6 +63,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
> disp/dpu1/dpu_encoder_phys_wb.o \
> disp/dpu1/dpu_formats.o \
> disp/dpu1/dpu_hw_catalog.o \
> + disp/dpu1/dpu_hw_cdm.o \
> disp/dpu1/dpu_hw_ctl.o \
> disp/dpu1/dpu_hw_dsc.o \
> disp/dpu1/dpu_hw_dsc_1_2.o \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
> new file mode 100644
> index 000000000000..4976f8a05ce7
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
> @@ -0,0 +1,263 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <drm/drm_managed.h>
> +
> +#include "dpu_hw_mdss.h"
> +#include "dpu_hw_util.h"
> +#include "dpu_hw_catalog.h"
> +#include "dpu_hw_cdm.h"
> +#include "dpu_kms.h"
> +
> +#define CDM_CSC_10_OPMODE 0x000
> +#define CDM_CSC_10_BASE 0x004
> +
> +#define CDM_CDWN2_OP_MODE 0x100
> +#define CDM_CDWN2_CLAMP_OUT 0x104
> +#define CDM_CDWN2_PARAMS_3D_0 0x108
> +#define CDM_CDWN2_PARAMS_3D_1 0x10C
> +#define CDM_CDWN2_COEFF_COSITE_H_0 0x110
> +#define CDM_CDWN2_COEFF_COSITE_H_1 0x114
> +#define CDM_CDWN2_COEFF_COSITE_H_2 0x118
> +#define CDM_CDWN2_COEFF_OFFSITE_H_0 0x11C
> +#define CDM_CDWN2_COEFF_OFFSITE_H_1 0x120
> +#define CDM_CDWN2_COEFF_OFFSITE_H_2 0x124
> +#define CDM_CDWN2_COEFF_COSITE_V 0x128
> +#define CDM_CDWN2_COEFF_OFFSITE_V 0x12C
> +#define CDM_CDWN2_OUT_SIZE 0x130
> +
> +#define CDM_HDMI_PACK_OP_MODE 0x200
> +#define CDM_CSC_10_MATRIX_COEFF_0 0x004
> +
> +#define CDM_MUX 0x224
> +
> +/* CDM CDWN2 sub-block bit definitions */
> +#define CDM_CDWN2_OP_MODE_EN BIT(0)
> +#define CDM_CDWN2_OP_MODE_ENABLE_H BIT(1)
> +#define CDM_CDWN2_OP_MODE_ENABLE_V BIT(2)
> +#define CDM_CDWN2_OP_MODE_METHOD_H_AVG BIT(3)
> +#define CDM_CDWN2_OP_MODE_METHOD_H_COSITE BIT(4)
> +#define CDM_CDWN2_OP_MODE_METHOD_V_AVG BIT(5)
> +#define CDM_CDWN2_OP_MODE_METHOD_V_COSITE BIT(6)
> +#define CDM_CDWN2_OP_MODE_BITS_OUT_8BIT BIT(7)
> +#define CDM_CDWN2_OP_MODE_METHOD_H_OFFSITE GENMASK(4, 3)
> +#define CDM_CDWN2_OP_MODE_METHOD_V_OFFSITE GENMASK(6, 5)
I think it might be easier to define
enum {
CDM_CDWN2_METHOD_DROP_PIXEL = 0,
CDM_CDWN2_METHOD_AVG = 1,
CDM_CDWN2_METHOD_ = 2,
CDM_CDWN2_METHOD_DROP_PIXEL = 3,
};
then use FIELD_PREP()
> +#define CDM_CDWN2_V_PIXEL_DROP_MASK GENMASK(6, 5)
> +#define CDM_CDWN2_H_PIXEL_DROP_MASK GENMASK(4, 3)
Why are they called foo_DROP_bar?
> +
> +/* CDM CSC10 sub-block bit definitions */
> +#define CDM_CSC10_OP_MODE_EN BIT(0)
> +#define CDM_CSC10_OP_MODE_SRC_FMT_YUV BIT(1)
> +#define CDM_CSC10_OP_MODE_DST_FMT_YUV BIT(2)
> +
> +/* CDM HDMI pack sub-block bit definitions */
> +#define CDM_HDMI_PACK_OP_MODE_EN BIT(0)
> +
> +/*
> + * Horizontal coefficients for cosite chroma downscale
> + * s13 representation of coefficients
> + */
> +static u32 cosite_h_coeff[] = {0x00000016, 0x000001cc, 0x0100009e};
> +
> +/*
> + * Horizontal coefficients for offsite chroma downscale
> + */
> +static u32 offsite_h_coeff[] = {0x000b0005, 0x01db01eb, 0x00e40046};
> +
> +/*
> + * Vertical coefficients for cosite chroma downscale
> + */
> +static u32 cosite_v_coeff[] = {0x00080004};
> +/*
> + * Vertical coefficients for offsite chroma downscale
> + */
> +static u32 offsite_v_coeff[] = {0x00060002};
> +
> +static int dpu_hw_cdm_setup_cdwn(struct dpu_hw_cdm *ctx, struct dpu_hw_cdm_cfg *cfg)
> +{
> + struct dpu_hw_blk_reg_map *c = &ctx->hw;
> + u32 opmode = 0;
> + u32 out_size = 0;
No need to init it, please drop.
> +
> + if (cfg->output_bit_depth != CDM_CDWN_OUTPUT_10BIT)
> + opmode |= CDM_CDWN2_OP_MODE_BITS_OUT_8BIT;
> +
> + /* ENABLE DWNS_H bit */
> + opmode |= CDM_CDWN2_OP_MODE_ENABLE_H;
> +
> + switch (cfg->h_cdwn_type) {
> + case CDM_CDWN_DISABLE:
> + /* CLEAR METHOD_H field */
> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
> + /* CLEAR DWNS_H bit */
> + opmode &= ~CDM_CDWN2_OP_MODE_ENABLE_H;
Please, can we get rid of clears for the zero-initialised variable? If
you move the 10bit/8bit check after this switch, you can drop the = 0
from the variable definition and instead have:
switch (type) {
case DISABLE:
opmode = 0;
break;
case PIXEL_DROP:
opmode = CDM_CDWN2_OP_MODE_ENABLE_H |
FIELD_PREP(CDM_CDWM2_OP_MODE_METHOD_H,
CDM_CDWN2_METHOD_DROP_PIXEL);
break;
case AVG:
opmode = CDM_CDWN2_OP_MODE_ENABLE_H |
FIELD_PREP(CDM_CDWM2_OP_MODE_METHOD_H,
CDM_CDWN2_METHOD_AVG);
break;
// etc.
}
Same for the v_type.
Also could you please drop useless comments which repeat what is being
done in the next line?
> + break;
> + case CDM_CDWN_PIXEL_DROP:
> + /* Clear METHOD_H field (pixel drop is 0) */
> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
> + break;
> + case CDM_CDWN_AVG:
> + /* Clear METHOD_H field (Average is 0x1) */
> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
> + opmode |= CDM_CDWN2_OP_MODE_METHOD_H_AVG;
> + break;
> + case CDM_CDWN_COSITE:
> + /* Clear METHOD_H field (Average is 0x2) */
So, is Average 0x1 or 0x2? Or 0x3 as written below?
> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
> + opmode |= CDM_CDWN2_OP_MODE_METHOD_H_COSITE;
> + /* Co-site horizontal coefficients */
> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_0,
> + cosite_h_coeff[0]);
> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_1,
> + cosite_h_coeff[1]);
> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_2,
> + cosite_h_coeff[2]);
> + break;
> + case CDM_CDWN_OFFSITE:
> + /* Clear METHOD_H field (Average is 0x3) */
> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
> + opmode |= CDM_CDWN2_OP_MODE_METHOD_H_OFFSITE;
> +
> + /* Off-site horizontal coefficients */
> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_0,
> + offsite_h_coeff[0]);
> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_1,
> + offsite_h_coeff[1]);
> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_2,
> + offsite_h_coeff[2]);
> + break;
> + default:
> + DPU_ERROR("%s invalid horz down sampling type\n", __func__);
> + return -EINVAL;
> + }
> +
> + /* ENABLE DWNS_V bit */
> + opmode |= CDM_CDWN2_OP_MODE_ENABLE_V;
> +
> + switch (cfg->v_cdwn_type) {
> + case CDM_CDWN_DISABLE:
> + /* CLEAR METHOD_V field */
> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
> + /* CLEAR DWNS_V bit */
> + opmode &= ~CDM_CDWN2_OP_MODE_ENABLE_V;
> + break;
> + case CDM_CDWN_PIXEL_DROP:
> + /* Clear METHOD_V field (pixel drop is 0) */
> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
> + break;
> + case CDM_CDWN_AVG:
> + /* Clear METHOD_V field (Average is 0x1) */
> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
> + opmode |= CDM_CDWN2_OP_MODE_METHOD_V_AVG;
> + break;
> + case CDM_CDWN_COSITE:
> + /* Clear METHOD_V field (Average is 0x2) */
> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
> + opmode |= CDM_CDWN2_OP_MODE_METHOD_V_COSITE;
> + /* Co-site vertical coefficients */
> + DPU_REG_WRITE(c,
> + CDM_CDWN2_COEFF_COSITE_V,
> + cosite_v_coeff[0]);
> + break;
> + case CDM_CDWN_OFFSITE:
> + /* Clear METHOD_V field (Average is 0x3) */
> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
> + opmode |= CDM_CDWN2_OP_MODE_METHOD_V_OFFSITE;
> +
> + /* Off-site vertical coefficients */
> + DPU_REG_WRITE(c,
> + CDM_CDWN2_COEFF_OFFSITE_V,
> + offsite_v_coeff[0]);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (cfg->v_cdwn_type || cfg->h_cdwn_type)
> + opmode |= CDM_CDWN2_OP_MODE_EN; /* EN CDWN module */
> + else
> + opmode &= ~CDM_CDWN2_OP_MODE_EN;
> +
> + out_size = (cfg->output_width & 0xFFFF) | ((cfg->output_height & 0xFFFF) << 16);
> + DPU_REG_WRITE(c, CDM_CDWN2_OUT_SIZE, out_size);
> + DPU_REG_WRITE(c, CDM_CDWN2_OP_MODE, opmode);
> + DPU_REG_WRITE(c, CDM_CDWN2_CLAMP_OUT, ((0x3FF << 16) | 0x0));
> +
> + return 0;
> +}
> +
> +static int dpu_hw_cdm_enable(struct dpu_hw_cdm *ctx, struct dpu_hw_cdm_cfg *cdm)
> +{
> + struct dpu_hw_blk_reg_map *c = &ctx->hw;
> + const struct dpu_format *fmt;
> + u32 opmode = 0;
> + u32 csc = 0;
> +
> + if (!ctx || !cdm)
> + return -EINVAL;
> +
> + fmt = cdm->output_fmt;
> +
> + if (!DPU_FORMAT_IS_YUV(fmt))
> + return -EINVAL;
> +
> + dpu_hw_csc_setup(&ctx->hw, CDM_CSC_10_MATRIX_COEFF_0, cdm->csc_cfg, true);
> + dpu_hw_cdm_setup_cdwn(ctx, cdm);
> +
> + if (cdm->output_type == CDM_CDWN_OUTPUT_HDMI) {
> + if (fmt->chroma_sample != DPU_CHROMA_H1V2)
> + return -EINVAL; /*unsupported format */
> + opmode = CDM_HDMI_PACK_OP_MODE_EN;
> + opmode |= (fmt->chroma_sample << 1);
> + }
> +
> + csc |= CDM_CSC10_OP_MODE_DST_FMT_YUV;
> + csc &= ~CDM_CSC10_OP_MODE_SRC_FMT_YUV;
> + csc |= CDM_CSC10_OP_MODE_EN;
> +
> + if (ctx && ctx->ops.bind_pingpong_blk)
> + ctx->ops.bind_pingpong_blk(ctx, cdm->pp_id);
> +
> + DPU_REG_WRITE(c, CDM_CSC_10_OPMODE, csc);
> + DPU_REG_WRITE(c, CDM_HDMI_PACK_OP_MODE, opmode);
> + return 0;
> +}
> +
> +static void dpu_hw_cdm_bind_pingpong_blk(struct dpu_hw_cdm *ctx, const enum dpu_pingpong pp)
> +{
> + struct dpu_hw_blk_reg_map *c;
> + int mux_cfg = 0xF; /* Disabled */
lowercase hex. And it is easier to move it to the if (pp) condition,
like it was done for INTF or WB.
> +
> + c = &ctx->hw;
> +
> + if (pp)
> + mux_cfg = (pp - PINGPONG_0) & 0x7;
> +
> + DPU_REG_WRITE(c, CDM_MUX, mux_cfg);
> +}
> +
> +struct dpu_hw_cdm *dpu_hw_cdm_init(struct drm_device *dev,
> + const struct dpu_cdm_cfg *cfg, void __iomem *addr,
> + const struct dpu_mdss_version *mdss_rev)
> +{
> + struct dpu_hw_cdm *c;
> +
> + c = drmm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> + if (!c)
> + return ERR_PTR(-ENOMEM);
> +
> + c->hw.blk_addr = addr + cfg->base;
> + c->hw.log_mask = DPU_DBG_MASK_CDM;
> +
> + /* Assign ops */
> + c->idx = cfg->id;
> + c->caps = cfg;
> +
> + c->ops.enable = dpu_hw_cdm_enable;
> + if (mdss_rev->core_major_ver >= 5)
> + c->ops.bind_pingpong_blk = dpu_hw_cdm_bind_pingpong_blk;
> +
> + return c;
> +}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
> new file mode 100644
> index 000000000000..e7d57dbd6103
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef _DPU_HW_CDM_H
> +#define _DPU_HW_CDM_H
> +
> +#include "dpu_hw_mdss.h"
> +#include "dpu_hw_top.h"
> +
> +struct dpu_hw_cdm;
> +
> +/**
> + * struct dpu_hw_cdm_cfg : current configuration of CDM block
> + *
> + * @output_width: output ROI width of CDM block
> + * @output_height: output ROI height of CDM block
> + * @output_bit_depth: output bit-depth of CDM block
> + * @h_cdwn_type: downsample type used for horizontal pixels
> + * @v_cdwn_type: downsample type used for vertical pixels
> + * @output_fmt: handle to dpu_format of CDM block
> + * @csc_cfg: handle to CSC matrix programmed for CDM block
> + * @output_type: interface to which CDM is paired (HDMI/WB)
> + * @pp_id: ping-pong block to which CDM is bound to
> + */
> +struct dpu_hw_cdm_cfg {
> + u32 output_width;
> + u32 output_height;
> + u32 output_bit_depth;
> + u32 h_cdwn_type;
> + u32 v_cdwn_type;
> + const struct dpu_format *output_fmt;
> + const struct dpu_csc_cfg *csc_cfg;
> + u32 output_type;
> + int pp_id;
> +};
> +
> +/*
> + * These values are used indicate which type of downsample is used
> + * in the horizontal/vertical direction for the CDM block.
> + */
> +enum dpu_hw_cdwn_type {
> + CDM_CDWN_DISABLE,
> + CDM_CDWN_PIXEL_DROP,
> + CDM_CDWN_AVG,
> + CDM_CDWN_COSITE,
> + CDM_CDWN_OFFSITE,
> +};
> +
> +/*
> + * CDM block can be paired with WB or HDMI block. These values match
> + * the input with which the CDM block is paired.
> + */
> +enum dpu_hw_cdwn_output_type {
> + CDM_CDWN_OUTPUT_HDMI,
> + CDM_CDWN_OUTPUT_WB,
> +};
> +
> +/*
> + * CDM block can give an 8-bit or 10-bit output. These values
> + * are used to indicate the output bit depth of CDM block
> + */
> +enum dpu_hw_cdwn_output_bit_depth {
> + CDM_CDWN_OUTPUT_8BIT,
> + CDM_CDWN_OUTPUT_10BIT,
> +};
> +
> +/**
> + * struct dpu_hw_cdm_ops : Interface to the chroma down Hw driver functions
> + * Assumption is these functions will be called after
> + * clocks are enabled
> + * @enable: Enables the output to interface and programs the
> + * output packer
> + * @bind_pingpong_blk: enable/disable the connection with pingpong which
> + * will feed pixels to this cdm
> + */
> +struct dpu_hw_cdm_ops {
> + /**
> + * Enable the CDM module
> + * @cdm Pointer to chroma down context
> + */
> + int (*enable)(struct dpu_hw_cdm *cdm, struct dpu_hw_cdm_cfg *cfg);
> +
> + /**
> + * Enable/disable the connection with pingpong
> + * @cdm Pointer to chroma down context
> + * @pp pingpong block id.
> + */
> + void (*bind_pingpong_blk)(struct dpu_hw_cdm *cdm, const enum dpu_pingpong pp);
> +};
> +
> +/**
> + * struct dpu_hw_cdm - cdm description
> + * @base: Hardware block base structure
> + * @hw: Block hardware details
> + * @idx: CDM index
> + * @caps: Pointer to cdm_cfg
> + * @ops: handle to operations possible for this CDM
> + */
> +struct dpu_hw_cdm {
> + struct dpu_hw_blk base;
> + struct dpu_hw_blk_reg_map hw;
> +
> + /* chroma down */
> + const struct dpu_cdm_cfg *caps;
> + enum dpu_cdm idx;
> +
> + /* ops */
> + struct dpu_hw_cdm_ops ops;
> +};
> +
> +/**
> + * dpu_hw_cdm_init - initializes the cdm hw driver object.
> + * should be called once before accessing every cdm.
> + * @dev: DRM device handle
> + * @cdm: CDM catalog entry for which driver object is required
> + * @addr : mapped register io address of MDSS
> + * @mdss_rev: mdss hw core revision
> + */
> +struct dpu_hw_cdm *dpu_hw_cdm_init(struct drm_device *dev,
> + const struct dpu_cdm_cfg *cdm, void __iomem *addr,
> + const struct dpu_mdss_version *mdss_rev);
> +
> +static inline struct dpu_hw_cdm *to_dpu_hw_cdm(struct dpu_hw_blk *hw)
> +{
> + return container_of(hw, struct dpu_hw_cdm, base);
> +}
> +
> +#endif /*_DPU_HW_CDM_H */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> index f319c8232ea5..9db4cf61bd29 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> @@ -466,6 +466,7 @@ struct dpu_mdss_color {
> #define DPU_DBG_MASK_ROT (1 << 9)
> #define DPU_DBG_MASK_DSPP (1 << 10)
> #define DPU_DBG_MASK_DSC (1 << 11)
> +#define DPU_DBG_MASK_CDM (1 << 12)
>
> /**
> * struct dpu_hw_tear_check - Struct contains parameters to configure
> --
> 2.40.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 01/15] drm/msm/dpu: add formats check for writeback encoder
2023-12-12 6:40 ` Dmitry Baryshkov
@ 2023-12-12 17:16 ` Abhinav Kumar
2023-12-12 21:32 ` Dmitry Baryshkov
0 siblings, 1 reply; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 17:16 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: freedreno, Rob Clark, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter, dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
linux-kernel
On 12/11/2023 10:40 PM, Dmitry Baryshkov wrote:
> On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> In preparation for adding more formats to dpu writeback add
>> format validation to it to fail any unsupported formats.
>>
>> changes in v3:
>> - rebase on top of msm-next
>> - replace drm_atomic_helper_check_wb_encoder_state() with
>> drm_atomic_helper_check_wb_connector_state() due to the
>> rebase
>>
>> changes in v2:
>> - correct some grammar in the commit text
>>
>> Fixes: d7d0e73f7de3 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for writeback")
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> index bb94909caa25..425415d45ec1 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> @@ -272,6 +272,7 @@ static int dpu_encoder_phys_wb_atomic_check(
>> {
>> struct drm_framebuffer *fb;
>> const struct drm_display_mode *mode = &crtc_state->mode;
>> + int ret;
>>
>> DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
>> phys_enc->hw_wb->idx, mode->name, mode->hdisplay, mode->vdisplay);
>> @@ -308,6 +309,12 @@ static int dpu_encoder_phys_wb_atomic_check(
>> return -EINVAL;
>> }
>>
>> + ret = drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
>> + if (ret < 0) {
>> + DPU_ERROR("invalid pixel format %p4cc\n", &fb->format->format);
>> + return ret;
>> + }
>
> There is no guarantee that there will be no other checks added to this
> helper. So, I think this message is incorrect. If you wish, you can
> promote the level of the message in the helper itself.
> On the other hand, we rarely print such messages by default. Most of
> the checks use drm_dbg.
>
hmm...actually drm_atomic_helper_check_wb_connector_state() already has
a debug message to indicate invalid pixel formats.
You are right, i should perhaps just say that "atomic_check failed" or
something.
I can make this a DPU_DEBUG. Actually I didnt know that we are not
supposed to print out atomic_check() errors. Is it perhaps because its
okay for check to fail?
But then we would not know why it failed.
>> +
>> return 0;
>> }
>>
>> --
>> 2.40.1
>>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 07/15] drm/msm/dpu: add dpu_hw_cdm abstraction for CDM block
2023-12-12 9:40 ` Dmitry Baryshkov
@ 2023-12-12 17:51 ` Abhinav Kumar
2023-12-12 21:29 ` Dmitry Baryshkov
0 siblings, 1 reply; 25+ messages in thread
From: Abhinav Kumar @ 2023-12-12 17:51 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: freedreno, Rob Clark, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter, dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
kernel test robot, linux-kernel
On 12/12/2023 1:40 AM, Dmitry Baryshkov wrote:
> On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> CDM block comes with its own set of registers and operations
>> which can be done. In-line with other hardware blocks, this
>> change adds the dpu_hw_cdm abstraction for the CDM block.
>>
>> changes in v3:
>> - fix commit text from sub-blk to blk for CDM
>> - fix kbot issue for missing static for dpu_hw_cdm_enable()
>> - fix kbot issue for incorrect documentation style
>> - add more documentation for enums and struct in dpu_hw_cdm.h
>> - drop "enable" parameter from bind_pingpong_blk() as we can
>> just use PINGPONG_NONE for disable cases
>> - drop unnecessary bit operation for zero value of cdm_cfg
>>
>> changes in v2:
>> - replace bit magic with relevant defines
>> - use drmm_kzalloc instead of kzalloc/free
>> - some formatting fixes
>> - inline _setup_cdm_ops()
>> - protect bind_pingpong_blk with core_rev check
>> - drop setup_csc_data() and setup_cdwn() ops as they
>> are merged into enable()
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202312101815.B3ZH7Pfy-lkp@intel.com/
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>> drivers/gpu/drm/msm/Makefile | 1 +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c | 263 ++++++++++++++++++++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h | 130 ++++++++++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 1 +
>> 4 files changed, 395 insertions(+)
>> create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
>> create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
>>
>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
>> index 49671364fdcf..b1173128b5b9 100644
>> --- a/drivers/gpu/drm/msm/Makefile
>> +++ b/drivers/gpu/drm/msm/Makefile
>> @@ -63,6 +63,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
>> disp/dpu1/dpu_encoder_phys_wb.o \
>> disp/dpu1/dpu_formats.o \
>> disp/dpu1/dpu_hw_catalog.o \
>> + disp/dpu1/dpu_hw_cdm.o \
>> disp/dpu1/dpu_hw_ctl.o \
>> disp/dpu1/dpu_hw_dsc.o \
>> disp/dpu1/dpu_hw_dsc_1_2.o \
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
>> new file mode 100644
>> index 000000000000..4976f8a05ce7
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
>> @@ -0,0 +1,263 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <drm/drm_managed.h>
>> +
>> +#include "dpu_hw_mdss.h"
>> +#include "dpu_hw_util.h"
>> +#include "dpu_hw_catalog.h"
>> +#include "dpu_hw_cdm.h"
>> +#include "dpu_kms.h"
>> +
>> +#define CDM_CSC_10_OPMODE 0x000
>> +#define CDM_CSC_10_BASE 0x004
>> +
>> +#define CDM_CDWN2_OP_MODE 0x100
>> +#define CDM_CDWN2_CLAMP_OUT 0x104
>> +#define CDM_CDWN2_PARAMS_3D_0 0x108
>> +#define CDM_CDWN2_PARAMS_3D_1 0x10C
>> +#define CDM_CDWN2_COEFF_COSITE_H_0 0x110
>> +#define CDM_CDWN2_COEFF_COSITE_H_1 0x114
>> +#define CDM_CDWN2_COEFF_COSITE_H_2 0x118
>> +#define CDM_CDWN2_COEFF_OFFSITE_H_0 0x11C
>> +#define CDM_CDWN2_COEFF_OFFSITE_H_1 0x120
>> +#define CDM_CDWN2_COEFF_OFFSITE_H_2 0x124
>> +#define CDM_CDWN2_COEFF_COSITE_V 0x128
>> +#define CDM_CDWN2_COEFF_OFFSITE_V 0x12C
>> +#define CDM_CDWN2_OUT_SIZE 0x130
>> +
>> +#define CDM_HDMI_PACK_OP_MODE 0x200
>> +#define CDM_CSC_10_MATRIX_COEFF_0 0x004
>> +
>> +#define CDM_MUX 0x224
>> +
>> +/* CDM CDWN2 sub-block bit definitions */
>> +#define CDM_CDWN2_OP_MODE_EN BIT(0)
>> +#define CDM_CDWN2_OP_MODE_ENABLE_H BIT(1)
>> +#define CDM_CDWN2_OP_MODE_ENABLE_V BIT(2)
>> +#define CDM_CDWN2_OP_MODE_METHOD_H_AVG BIT(3)
>> +#define CDM_CDWN2_OP_MODE_METHOD_H_COSITE BIT(4)
>> +#define CDM_CDWN2_OP_MODE_METHOD_V_AVG BIT(5)
>> +#define CDM_CDWN2_OP_MODE_METHOD_V_COSITE BIT(6)
>> +#define CDM_CDWN2_OP_MODE_BITS_OUT_8BIT BIT(7)
>> +#define CDM_CDWN2_OP_MODE_METHOD_H_OFFSITE GENMASK(4, 3)
>> +#define CDM_CDWN2_OP_MODE_METHOD_V_OFFSITE GENMASK(6, 5)
>
> I think it might be easier to define
>
> enum {
> CDM_CDWN2_METHOD_DROP_PIXEL = 0,
> CDM_CDWN2_METHOD_AVG = 1,
> CDM_CDWN2_METHOD_ = 2,
> CDM_CDWN2_METHOD_DROP_PIXEL = 3,
> };
>
> then use FIELD_PREP()
>
Ok, let me explore that option.
We already have below enum in dpu_hw_cdm.h, we can re-use that
+enum dpu_hw_cdwn_type {
+ CDM_CDWN_DISABLE,
+ CDM_CDWN_PIXEL_DROP,
+ CDM_CDWN_AVG,
+ CDM_CDWN_COSITE,
+ CDM_CDWN_OFFSITE,
+};
>> +#define CDM_CDWN2_V_PIXEL_DROP_MASK GENMASK(6, 5)
>> +#define CDM_CDWN2_H_PIXEL_DROP_MASK GENMASK(4, 3)
>
> Why are they called foo_DROP_bar?
>
Because they are always used with ~
And as per the documentation 0 for this field is pixel drop.
If we are dropping the clearing altogether, then these masks will go
away too.
>> +
>> +/* CDM CSC10 sub-block bit definitions */
>> +#define CDM_CSC10_OP_MODE_EN BIT(0)
>> +#define CDM_CSC10_OP_MODE_SRC_FMT_YUV BIT(1)
>> +#define CDM_CSC10_OP_MODE_DST_FMT_YUV BIT(2)
>> +
>> +/* CDM HDMI pack sub-block bit definitions */
>> +#define CDM_HDMI_PACK_OP_MODE_EN BIT(0)
>> +
>> +/*
>> + * Horizontal coefficients for cosite chroma downscale
>> + * s13 representation of coefficients
>> + */
>> +static u32 cosite_h_coeff[] = {0x00000016, 0x000001cc, 0x0100009e};
>> +
>> +/*
>> + * Horizontal coefficients for offsite chroma downscale
>> + */
>> +static u32 offsite_h_coeff[] = {0x000b0005, 0x01db01eb, 0x00e40046};
>> +
>> +/*
>> + * Vertical coefficients for cosite chroma downscale
>> + */
>> +static u32 cosite_v_coeff[] = {0x00080004};
>> +/*
>> + * Vertical coefficients for offsite chroma downscale
>> + */
>> +static u32 offsite_v_coeff[] = {0x00060002};
>> +
>> +static int dpu_hw_cdm_setup_cdwn(struct dpu_hw_cdm *ctx, struct dpu_hw_cdm_cfg *cfg)
>> +{
>> + struct dpu_hw_blk_reg_map *c = &ctx->hw;
>> + u32 opmode = 0;
>> + u32 out_size = 0;
>
> No need to init it, please drop.
>
alright.
>> +
>> + if (cfg->output_bit_depth != CDM_CDWN_OUTPUT_10BIT)
>> + opmode |= CDM_CDWN2_OP_MODE_BITS_OUT_8BIT;
>> +
>> + /* ENABLE DWNS_H bit */
>> + opmode |= CDM_CDWN2_OP_MODE_ENABLE_H;
>> +
>> + switch (cfg->h_cdwn_type) {
>> + case CDM_CDWN_DISABLE:
>> + /* CLEAR METHOD_H field */
>> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
>> + /* CLEAR DWNS_H bit */
>> + opmode &= ~CDM_CDWN2_OP_MODE_ENABLE_H;
>
> Please, can we get rid of clears for the zero-initialised variable? If
> you move the 10bit/8bit check after this switch, you can drop the = 0
> from the variable definition and instead have:
>
Ok, will refine further.
> switch (type) {
> case DISABLE:
> opmode = 0;
> break;
> case PIXEL_DROP:
> opmode = CDM_CDWN2_OP_MODE_ENABLE_H |
> FIELD_PREP(CDM_CDWM2_OP_MODE_METHOD_H,
> CDM_CDWN2_METHOD_DROP_PIXEL);
> break;
> case AVG:
> opmode = CDM_CDWN2_OP_MODE_ENABLE_H |
> FIELD_PREP(CDM_CDWM2_OP_MODE_METHOD_H,
> CDM_CDWN2_METHOD_AVG);
> break;
> // etc.
> }
>
> Same for the v_type.
>
> Also could you please drop useless comments which repeat what is being
> done in the next line?
>
ack :)
>> + break;
>> + case CDM_CDWN_PIXEL_DROP:
>> + /* Clear METHOD_H field (pixel drop is 0) */
>> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
>> + break;
>> + case CDM_CDWN_AVG:
>> + /* Clear METHOD_H field (Average is 0x1) */
>> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
>> + opmode |= CDM_CDWN2_OP_MODE_METHOD_H_AVG;
>> + break;
>> + case CDM_CDWN_COSITE:
>> + /* Clear METHOD_H field (Average is 0x2) */
>
> So, is Average 0x1 or 0x2? Or 0x3 as written below?
>
The comment is wrong. It should say Co-Site is 0x2, Off-Site is 0x3.
And for the record, average is 0x1 :)
>> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
>> + opmode |= CDM_CDWN2_OP_MODE_METHOD_H_COSITE;
>> + /* Co-site horizontal coefficients */
>> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_0,
>> + cosite_h_coeff[0]);
>> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_1,
>> + cosite_h_coeff[1]);
>> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_2,
>> + cosite_h_coeff[2]);
>> + break;
>> + case CDM_CDWN_OFFSITE:
>> + /* Clear METHOD_H field (Average is 0x3) */
>> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
>> + opmode |= CDM_CDWN2_OP_MODE_METHOD_H_OFFSITE;
>> +
>> + /* Off-site horizontal coefficients */
>> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_0,
>> + offsite_h_coeff[0]);
>> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_1,
>> + offsite_h_coeff[1]);
>> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_2,
>> + offsite_h_coeff[2]);
>> + break;
>> + default:
>> + DPU_ERROR("%s invalid horz down sampling type\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + /* ENABLE DWNS_V bit */
>> + opmode |= CDM_CDWN2_OP_MODE_ENABLE_V;
>> +
>> + switch (cfg->v_cdwn_type) {
>> + case CDM_CDWN_DISABLE:
>> + /* CLEAR METHOD_V field */
>> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
>> + /* CLEAR DWNS_V bit */
>> + opmode &= ~CDM_CDWN2_OP_MODE_ENABLE_V;
>> + break;
>> + case CDM_CDWN_PIXEL_DROP:
>> + /* Clear METHOD_V field (pixel drop is 0) */
>> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
>> + break;
>> + case CDM_CDWN_AVG:
>> + /* Clear METHOD_V field (Average is 0x1) */
>> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
>> + opmode |= CDM_CDWN2_OP_MODE_METHOD_V_AVG;
>> + break;
>> + case CDM_CDWN_COSITE:
>> + /* Clear METHOD_V field (Average is 0x2) */
>> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
>> + opmode |= CDM_CDWN2_OP_MODE_METHOD_V_COSITE;
>> + /* Co-site vertical coefficients */
>> + DPU_REG_WRITE(c,
>> + CDM_CDWN2_COEFF_COSITE_V,
>> + cosite_v_coeff[0]);
>> + break;
>> + case CDM_CDWN_OFFSITE:
>> + /* Clear METHOD_V field (Average is 0x3) */
>> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
>> + opmode |= CDM_CDWN2_OP_MODE_METHOD_V_OFFSITE;
>> +
>> + /* Off-site vertical coefficients */
>> + DPU_REG_WRITE(c,
>> + CDM_CDWN2_COEFF_OFFSITE_V,
>> + offsite_v_coeff[0]);
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + if (cfg->v_cdwn_type || cfg->h_cdwn_type)
>> + opmode |= CDM_CDWN2_OP_MODE_EN; /* EN CDWN module */
>> + else
>> + opmode &= ~CDM_CDWN2_OP_MODE_EN;
>> +
>> + out_size = (cfg->output_width & 0xFFFF) | ((cfg->output_height & 0xFFFF) << 16);
>> + DPU_REG_WRITE(c, CDM_CDWN2_OUT_SIZE, out_size);
>> + DPU_REG_WRITE(c, CDM_CDWN2_OP_MODE, opmode);
>> + DPU_REG_WRITE(c, CDM_CDWN2_CLAMP_OUT, ((0x3FF << 16) | 0x0));
>> +
>> + return 0;
>> +}
>> +
>> +static int dpu_hw_cdm_enable(struct dpu_hw_cdm *ctx, struct dpu_hw_cdm_cfg *cdm)
>> +{
>> + struct dpu_hw_blk_reg_map *c = &ctx->hw;
>> + const struct dpu_format *fmt;
>> + u32 opmode = 0;
>> + u32 csc = 0;
>> +
>> + if (!ctx || !cdm)
>> + return -EINVAL;
>> +
>> + fmt = cdm->output_fmt;
>> +
>> + if (!DPU_FORMAT_IS_YUV(fmt))
>> + return -EINVAL;
>> +
>> + dpu_hw_csc_setup(&ctx->hw, CDM_CSC_10_MATRIX_COEFF_0, cdm->csc_cfg, true);
>> + dpu_hw_cdm_setup_cdwn(ctx, cdm);
>> +
>> + if (cdm->output_type == CDM_CDWN_OUTPUT_HDMI) {
>> + if (fmt->chroma_sample != DPU_CHROMA_H1V2)
>> + return -EINVAL; /*unsupported format */
>> + opmode = CDM_HDMI_PACK_OP_MODE_EN;
>> + opmode |= (fmt->chroma_sample << 1);
>> + }
>> +
>> + csc |= CDM_CSC10_OP_MODE_DST_FMT_YUV;
>> + csc &= ~CDM_CSC10_OP_MODE_SRC_FMT_YUV;
>> + csc |= CDM_CSC10_OP_MODE_EN;
>> +
>> + if (ctx && ctx->ops.bind_pingpong_blk)
>> + ctx->ops.bind_pingpong_blk(ctx, cdm->pp_id);
>> +
>> + DPU_REG_WRITE(c, CDM_CSC_10_OPMODE, csc);
>> + DPU_REG_WRITE(c, CDM_HDMI_PACK_OP_MODE, opmode);
>> + return 0;
>> +}
>> +
>> +static void dpu_hw_cdm_bind_pingpong_blk(struct dpu_hw_cdm *ctx, const enum dpu_pingpong pp)
>> +{
>> + struct dpu_hw_blk_reg_map *c;
>> + int mux_cfg = 0xF; /* Disabled */
>
> lowercase hex. And it is easier to move it to the if (pp) condition,
> like it was done for INTF or WB.
>
ack for the lowercase hex.
hmm, i followed the dpu_hw_dsc_1_2 model, so basically for 0
(PINGPONG_NONE) cases it will stay at 0xf.
>> +
>> + c = &ctx->hw;
>> +
>> + if (pp)
>> + mux_cfg = (pp - PINGPONG_0) & 0x7;
>> +
>> + DPU_REG_WRITE(c, CDM_MUX, mux_cfg);
>> +}
>> +
>> +struct dpu_hw_cdm *dpu_hw_cdm_init(struct drm_device *dev,
>> + const struct dpu_cdm_cfg *cfg, void __iomem *addr,
>> + const struct dpu_mdss_version *mdss_rev)
>> +{
>> + struct dpu_hw_cdm *c;
>> +
>> + c = drmm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>> + if (!c)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + c->hw.blk_addr = addr + cfg->base;
>> + c->hw.log_mask = DPU_DBG_MASK_CDM;
>> +
>> + /* Assign ops */
>> + c->idx = cfg->id;
>> + c->caps = cfg;
>> +
>> + c->ops.enable = dpu_hw_cdm_enable;
>> + if (mdss_rev->core_major_ver >= 5)
>> + c->ops.bind_pingpong_blk = dpu_hw_cdm_bind_pingpong_blk;
>> +
>> + return c;
>> +}
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
>> new file mode 100644
>> index 000000000000..e7d57dbd6103
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
>> @@ -0,0 +1,130 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2023, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#ifndef _DPU_HW_CDM_H
>> +#define _DPU_HW_CDM_H
>> +
>> +#include "dpu_hw_mdss.h"
>> +#include "dpu_hw_top.h"
>> +
>> +struct dpu_hw_cdm;
>> +
>> +/**
>> + * struct dpu_hw_cdm_cfg : current configuration of CDM block
>> + *
>> + * @output_width: output ROI width of CDM block
>> + * @output_height: output ROI height of CDM block
>> + * @output_bit_depth: output bit-depth of CDM block
>> + * @h_cdwn_type: downsample type used for horizontal pixels
>> + * @v_cdwn_type: downsample type used for vertical pixels
>> + * @output_fmt: handle to dpu_format of CDM block
>> + * @csc_cfg: handle to CSC matrix programmed for CDM block
>> + * @output_type: interface to which CDM is paired (HDMI/WB)
>> + * @pp_id: ping-pong block to which CDM is bound to
>> + */
>> +struct dpu_hw_cdm_cfg {
>> + u32 output_width;
>> + u32 output_height;
>> + u32 output_bit_depth;
>> + u32 h_cdwn_type;
>> + u32 v_cdwn_type;
>> + const struct dpu_format *output_fmt;
>> + const struct dpu_csc_cfg *csc_cfg;
>> + u32 output_type;
>> + int pp_id;
>> +};
>> +
>> +/*
>> + * These values are used indicate which type of downsample is used
>> + * in the horizontal/vertical direction for the CDM block.
>> + */
>> +enum dpu_hw_cdwn_type {
>> + CDM_CDWN_DISABLE,
>> + CDM_CDWN_PIXEL_DROP,
>> + CDM_CDWN_AVG,
>> + CDM_CDWN_COSITE,
>> + CDM_CDWN_OFFSITE,
>> +};
>> +
>> +/*
>> + * CDM block can be paired with WB or HDMI block. These values match
>> + * the input with which the CDM block is paired.
>> + */
>> +enum dpu_hw_cdwn_output_type {
>> + CDM_CDWN_OUTPUT_HDMI,
>> + CDM_CDWN_OUTPUT_WB,
>> +};
>> +
>> +/*
>> + * CDM block can give an 8-bit or 10-bit output. These values
>> + * are used to indicate the output bit depth of CDM block
>> + */
>> +enum dpu_hw_cdwn_output_bit_depth {
>> + CDM_CDWN_OUTPUT_8BIT,
>> + CDM_CDWN_OUTPUT_10BIT,
>> +};
>> +
>> +/**
>> + * struct dpu_hw_cdm_ops : Interface to the chroma down Hw driver functions
>> + * Assumption is these functions will be called after
>> + * clocks are enabled
>> + * @enable: Enables the output to interface and programs the
>> + * output packer
>> + * @bind_pingpong_blk: enable/disable the connection with pingpong which
>> + * will feed pixels to this cdm
>> + */
>> +struct dpu_hw_cdm_ops {
>> + /**
>> + * Enable the CDM module
>> + * @cdm Pointer to chroma down context
>> + */
>> + int (*enable)(struct dpu_hw_cdm *cdm, struct dpu_hw_cdm_cfg *cfg);
>> +
>> + /**
>> + * Enable/disable the connection with pingpong
>> + * @cdm Pointer to chroma down context
>> + * @pp pingpong block id.
>> + */
>> + void (*bind_pingpong_blk)(struct dpu_hw_cdm *cdm, const enum dpu_pingpong pp);
>> +};
>> +
>> +/**
>> + * struct dpu_hw_cdm - cdm description
>> + * @base: Hardware block base structure
>> + * @hw: Block hardware details
>> + * @idx: CDM index
>> + * @caps: Pointer to cdm_cfg
>> + * @ops: handle to operations possible for this CDM
>> + */
>> +struct dpu_hw_cdm {
>> + struct dpu_hw_blk base;
>> + struct dpu_hw_blk_reg_map hw;
>> +
>> + /* chroma down */
>> + const struct dpu_cdm_cfg *caps;
>> + enum dpu_cdm idx;
>> +
>> + /* ops */
>> + struct dpu_hw_cdm_ops ops;
>> +};
>> +
>> +/**
>> + * dpu_hw_cdm_init - initializes the cdm hw driver object.
>> + * should be called once before accessing every cdm.
>> + * @dev: DRM device handle
>> + * @cdm: CDM catalog entry for which driver object is required
>> + * @addr : mapped register io address of MDSS
>> + * @mdss_rev: mdss hw core revision
>> + */
>> +struct dpu_hw_cdm *dpu_hw_cdm_init(struct drm_device *dev,
>> + const struct dpu_cdm_cfg *cdm, void __iomem *addr,
>> + const struct dpu_mdss_version *mdss_rev);
>> +
>> +static inline struct dpu_hw_cdm *to_dpu_hw_cdm(struct dpu_hw_blk *hw)
>> +{
>> + return container_of(hw, struct dpu_hw_cdm, base);
>> +}
>> +
>> +#endif /*_DPU_HW_CDM_H */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>> index f319c8232ea5..9db4cf61bd29 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>> @@ -466,6 +466,7 @@ struct dpu_mdss_color {
>> #define DPU_DBG_MASK_ROT (1 << 9)
>> #define DPU_DBG_MASK_DSPP (1 << 10)
>> #define DPU_DBG_MASK_DSC (1 << 11)
>> +#define DPU_DBG_MASK_CDM (1 << 12)
>>
>> /**
>> * struct dpu_hw_tear_check - Struct contains parameters to configure
>> --
>> 2.40.1
>>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 07/15] drm/msm/dpu: add dpu_hw_cdm abstraction for CDM block
2023-12-12 17:51 ` Abhinav Kumar
@ 2023-12-12 21:29 ` Dmitry Baryshkov
0 siblings, 0 replies; 25+ messages in thread
From: Dmitry Baryshkov @ 2023-12-12 21:29 UTC (permalink / raw
To: Abhinav Kumar
Cc: freedreno, Rob Clark, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter, dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
kernel test robot, linux-kernel
On Tue, 12 Dec 2023 at 19:51, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/12/2023 1:40 AM, Dmitry Baryshkov wrote:
> > On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> CDM block comes with its own set of registers and operations
> >> which can be done. In-line with other hardware blocks, this
> >> change adds the dpu_hw_cdm abstraction for the CDM block.
> >>
> >> changes in v3:
> >> - fix commit text from sub-blk to blk for CDM
> >> - fix kbot issue for missing static for dpu_hw_cdm_enable()
> >> - fix kbot issue for incorrect documentation style
> >> - add more documentation for enums and struct in dpu_hw_cdm.h
> >> - drop "enable" parameter from bind_pingpong_blk() as we can
> >> just use PINGPONG_NONE for disable cases
> >> - drop unnecessary bit operation for zero value of cdm_cfg
> >>
> >> changes in v2:
> >> - replace bit magic with relevant defines
> >> - use drmm_kzalloc instead of kzalloc/free
> >> - some formatting fixes
> >> - inline _setup_cdm_ops()
> >> - protect bind_pingpong_blk with core_rev check
> >> - drop setup_csc_data() and setup_cdwn() ops as they
> >> are merged into enable()
> >>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Closes: https://lore.kernel.org/oe-kbuild-all/202312101815.B3ZH7Pfy-lkp@intel.com/
> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >> ---
> >> drivers/gpu/drm/msm/Makefile | 1 +
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c | 263 ++++++++++++++++++++
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h | 130 ++++++++++
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 1 +
> >> 4 files changed, 395 insertions(+)
> >> create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
> >> create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
> >>
> >> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> >> index 49671364fdcf..b1173128b5b9 100644
> >> --- a/drivers/gpu/drm/msm/Makefile
> >> +++ b/drivers/gpu/drm/msm/Makefile
> >> @@ -63,6 +63,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
> >> disp/dpu1/dpu_encoder_phys_wb.o \
> >> disp/dpu1/dpu_formats.o \
> >> disp/dpu1/dpu_hw_catalog.o \
> >> + disp/dpu1/dpu_hw_cdm.o \
> >> disp/dpu1/dpu_hw_ctl.o \
> >> disp/dpu1/dpu_hw_dsc.o \
> >> disp/dpu1/dpu_hw_dsc_1_2.o \
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
> >> new file mode 100644
> >> index 000000000000..4976f8a05ce7
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c
> >> @@ -0,0 +1,263 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2023, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +#include <drm/drm_managed.h>
> >> +
> >> +#include "dpu_hw_mdss.h"
> >> +#include "dpu_hw_util.h"
> >> +#include "dpu_hw_catalog.h"
> >> +#include "dpu_hw_cdm.h"
> >> +#include "dpu_kms.h"
> >> +
> >> +#define CDM_CSC_10_OPMODE 0x000
> >> +#define CDM_CSC_10_BASE 0x004
> >> +
> >> +#define CDM_CDWN2_OP_MODE 0x100
> >> +#define CDM_CDWN2_CLAMP_OUT 0x104
> >> +#define CDM_CDWN2_PARAMS_3D_0 0x108
> >> +#define CDM_CDWN2_PARAMS_3D_1 0x10C
> >> +#define CDM_CDWN2_COEFF_COSITE_H_0 0x110
> >> +#define CDM_CDWN2_COEFF_COSITE_H_1 0x114
> >> +#define CDM_CDWN2_COEFF_COSITE_H_2 0x118
> >> +#define CDM_CDWN2_COEFF_OFFSITE_H_0 0x11C
> >> +#define CDM_CDWN2_COEFF_OFFSITE_H_1 0x120
> >> +#define CDM_CDWN2_COEFF_OFFSITE_H_2 0x124
> >> +#define CDM_CDWN2_COEFF_COSITE_V 0x128
> >> +#define CDM_CDWN2_COEFF_OFFSITE_V 0x12C
> >> +#define CDM_CDWN2_OUT_SIZE 0x130
> >> +
> >> +#define CDM_HDMI_PACK_OP_MODE 0x200
> >> +#define CDM_CSC_10_MATRIX_COEFF_0 0x004
> >> +
> >> +#define CDM_MUX 0x224
> >> +
> >> +/* CDM CDWN2 sub-block bit definitions */
> >> +#define CDM_CDWN2_OP_MODE_EN BIT(0)
> >> +#define CDM_CDWN2_OP_MODE_ENABLE_H BIT(1)
> >> +#define CDM_CDWN2_OP_MODE_ENABLE_V BIT(2)
> >> +#define CDM_CDWN2_OP_MODE_METHOD_H_AVG BIT(3)
> >> +#define CDM_CDWN2_OP_MODE_METHOD_H_COSITE BIT(4)
> >> +#define CDM_CDWN2_OP_MODE_METHOD_V_AVG BIT(5)
> >> +#define CDM_CDWN2_OP_MODE_METHOD_V_COSITE BIT(6)
> >> +#define CDM_CDWN2_OP_MODE_BITS_OUT_8BIT BIT(7)
> >> +#define CDM_CDWN2_OP_MODE_METHOD_H_OFFSITE GENMASK(4, 3)
> >> +#define CDM_CDWN2_OP_MODE_METHOD_V_OFFSITE GENMASK(6, 5)
> >
> > I think it might be easier to define
> >
> > enum {
> > CDM_CDWN2_METHOD_DROP_PIXEL = 0,
> > CDM_CDWN2_METHOD_AVG = 1,
> > CDM_CDWN2_METHOD_ = 2,
> > CDM_CDWN2_METHOD_DROP_PIXEL = 3,
> > };
> >
> > then use FIELD_PREP()
> >
>
> Ok, let me explore that option.
>
> We already have below enum in dpu_hw_cdm.h, we can re-use that
>
> +enum dpu_hw_cdwn_type {
> + CDM_CDWN_DISABLE,
> + CDM_CDWN_PIXEL_DROP,
> + CDM_CDWN_AVG,
> + CDM_CDWN_COSITE,
> + CDM_CDWN_OFFSITE,
> +};
>
> >> +#define CDM_CDWN2_V_PIXEL_DROP_MASK GENMASK(6, 5)
> >> +#define CDM_CDWN2_H_PIXEL_DROP_MASK GENMASK(4, 3)
> >
> > Why are they called foo_DROP_bar?
> >
>
> Because they are always used with ~
>
> And as per the documentation 0 for this field is pixel drop.
>
> If we are dropping the clearing altogether, then these masks will go
> away too.
>
> >> +
> >> +/* CDM CSC10 sub-block bit definitions */
> >> +#define CDM_CSC10_OP_MODE_EN BIT(0)
> >> +#define CDM_CSC10_OP_MODE_SRC_FMT_YUV BIT(1)
> >> +#define CDM_CSC10_OP_MODE_DST_FMT_YUV BIT(2)
> >> +
> >> +/* CDM HDMI pack sub-block bit definitions */
> >> +#define CDM_HDMI_PACK_OP_MODE_EN BIT(0)
> >> +
> >> +/*
> >> + * Horizontal coefficients for cosite chroma downscale
> >> + * s13 representation of coefficients
> >> + */
> >> +static u32 cosite_h_coeff[] = {0x00000016, 0x000001cc, 0x0100009e};
> >> +
> >> +/*
> >> + * Horizontal coefficients for offsite chroma downscale
> >> + */
> >> +static u32 offsite_h_coeff[] = {0x000b0005, 0x01db01eb, 0x00e40046};
> >> +
> >> +/*
> >> + * Vertical coefficients for cosite chroma downscale
> >> + */
> >> +static u32 cosite_v_coeff[] = {0x00080004};
> >> +/*
> >> + * Vertical coefficients for offsite chroma downscale
> >> + */
> >> +static u32 offsite_v_coeff[] = {0x00060002};
> >> +
> >> +static int dpu_hw_cdm_setup_cdwn(struct dpu_hw_cdm *ctx, struct dpu_hw_cdm_cfg *cfg)
> >> +{
> >> + struct dpu_hw_blk_reg_map *c = &ctx->hw;
> >> + u32 opmode = 0;
> >> + u32 out_size = 0;
> >
> > No need to init it, please drop.
> >
>
> alright.
>
> >> +
> >> + if (cfg->output_bit_depth != CDM_CDWN_OUTPUT_10BIT)
> >> + opmode |= CDM_CDWN2_OP_MODE_BITS_OUT_8BIT;
> >> +
> >> + /* ENABLE DWNS_H bit */
> >> + opmode |= CDM_CDWN2_OP_MODE_ENABLE_H;
> >> +
> >> + switch (cfg->h_cdwn_type) {
> >> + case CDM_CDWN_DISABLE:
> >> + /* CLEAR METHOD_H field */
> >> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
> >> + /* CLEAR DWNS_H bit */
> >> + opmode &= ~CDM_CDWN2_OP_MODE_ENABLE_H;
> >
> > Please, can we get rid of clears for the zero-initialised variable? If
> > you move the 10bit/8bit check after this switch, you can drop the = 0
> > from the variable definition and instead have:
> >
>
> Ok, will refine further.
>
> > switch (type) {
> > case DISABLE:
> > opmode = 0;
> > break;
> > case PIXEL_DROP:
> > opmode = CDM_CDWN2_OP_MODE_ENABLE_H |
> > FIELD_PREP(CDM_CDWM2_OP_MODE_METHOD_H,
> > CDM_CDWN2_METHOD_DROP_PIXEL);
> > break;
> > case AVG:
> > opmode = CDM_CDWN2_OP_MODE_ENABLE_H |
> > FIELD_PREP(CDM_CDWM2_OP_MODE_METHOD_H,
> > CDM_CDWN2_METHOD_AVG);
> > break;
> > // etc.
> > }
> >
> > Same for the v_type.
> >
>
> > Also could you please drop useless comments which repeat what is being
> > done in the next line?
> >
>
> ack :)
>
> >> + break;
> >> + case CDM_CDWN_PIXEL_DROP:
> >> + /* Clear METHOD_H field (pixel drop is 0) */
> >> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
> >> + break;
> >> + case CDM_CDWN_AVG:
> >> + /* Clear METHOD_H field (Average is 0x1) */
> >> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
> >> + opmode |= CDM_CDWN2_OP_MODE_METHOD_H_AVG;
> >> + break;
> >> + case CDM_CDWN_COSITE:
> >> + /* Clear METHOD_H field (Average is 0x2) */
> >
> > So, is Average 0x1 or 0x2? Or 0x3 as written below?
> >
>
> The comment is wrong. It should say Co-Site is 0x2, Off-Site is 0x3.
>
> And for the record, average is 0x1 :)
>
>
> >> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
> >> + opmode |= CDM_CDWN2_OP_MODE_METHOD_H_COSITE;
> >> + /* Co-site horizontal coefficients */
> >> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_0,
> >> + cosite_h_coeff[0]);
> >> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_1,
> >> + cosite_h_coeff[1]);
> >> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_2,
> >> + cosite_h_coeff[2]);
> >> + break;
> >> + case CDM_CDWN_OFFSITE:
> >> + /* Clear METHOD_H field (Average is 0x3) */
> >> + opmode &= ~CDM_CDWN2_H_PIXEL_DROP_MASK;
> >> + opmode |= CDM_CDWN2_OP_MODE_METHOD_H_OFFSITE;
> >> +
> >> + /* Off-site horizontal coefficients */
> >> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_0,
> >> + offsite_h_coeff[0]);
> >> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_1,
> >> + offsite_h_coeff[1]);
> >> + DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_2,
> >> + offsite_h_coeff[2]);
> >> + break;
> >> + default:
> >> + DPU_ERROR("%s invalid horz down sampling type\n", __func__);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* ENABLE DWNS_V bit */
> >> + opmode |= CDM_CDWN2_OP_MODE_ENABLE_V;
> >> +
> >> + switch (cfg->v_cdwn_type) {
> >> + case CDM_CDWN_DISABLE:
> >> + /* CLEAR METHOD_V field */
> >> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
> >> + /* CLEAR DWNS_V bit */
> >> + opmode &= ~CDM_CDWN2_OP_MODE_ENABLE_V;
> >> + break;
> >> + case CDM_CDWN_PIXEL_DROP:
> >> + /* Clear METHOD_V field (pixel drop is 0) */
> >> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
> >> + break;
> >> + case CDM_CDWN_AVG:
> >> + /* Clear METHOD_V field (Average is 0x1) */
> >> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
> >> + opmode |= CDM_CDWN2_OP_MODE_METHOD_V_AVG;
> >> + break;
> >> + case CDM_CDWN_COSITE:
> >> + /* Clear METHOD_V field (Average is 0x2) */
> >> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
> >> + opmode |= CDM_CDWN2_OP_MODE_METHOD_V_COSITE;
> >> + /* Co-site vertical coefficients */
> >> + DPU_REG_WRITE(c,
> >> + CDM_CDWN2_COEFF_COSITE_V,
> >> + cosite_v_coeff[0]);
> >> + break;
> >> + case CDM_CDWN_OFFSITE:
> >> + /* Clear METHOD_V field (Average is 0x3) */
> >> + opmode &= ~CDM_CDWN2_V_PIXEL_DROP_MASK;
> >> + opmode |= CDM_CDWN2_OP_MODE_METHOD_V_OFFSITE;
> >> +
> >> + /* Off-site vertical coefficients */
> >> + DPU_REG_WRITE(c,
> >> + CDM_CDWN2_COEFF_OFFSITE_V,
> >> + offsite_v_coeff[0]);
> >> + break;
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (cfg->v_cdwn_type || cfg->h_cdwn_type)
> >> + opmode |= CDM_CDWN2_OP_MODE_EN; /* EN CDWN module */
> >> + else
> >> + opmode &= ~CDM_CDWN2_OP_MODE_EN;
> >> +
> >> + out_size = (cfg->output_width & 0xFFFF) | ((cfg->output_height & 0xFFFF) << 16);
> >> + DPU_REG_WRITE(c, CDM_CDWN2_OUT_SIZE, out_size);
> >> + DPU_REG_WRITE(c, CDM_CDWN2_OP_MODE, opmode);
> >> + DPU_REG_WRITE(c, CDM_CDWN2_CLAMP_OUT, ((0x3FF << 16) | 0x0));
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int dpu_hw_cdm_enable(struct dpu_hw_cdm *ctx, struct dpu_hw_cdm_cfg *cdm)
> >> +{
> >> + struct dpu_hw_blk_reg_map *c = &ctx->hw;
> >> + const struct dpu_format *fmt;
> >> + u32 opmode = 0;
> >> + u32 csc = 0;
> >> +
> >> + if (!ctx || !cdm)
> >> + return -EINVAL;
> >> +
> >> + fmt = cdm->output_fmt;
> >> +
> >> + if (!DPU_FORMAT_IS_YUV(fmt))
> >> + return -EINVAL;
> >> +
> >> + dpu_hw_csc_setup(&ctx->hw, CDM_CSC_10_MATRIX_COEFF_0, cdm->csc_cfg, true);
> >> + dpu_hw_cdm_setup_cdwn(ctx, cdm);
> >> +
> >> + if (cdm->output_type == CDM_CDWN_OUTPUT_HDMI) {
> >> + if (fmt->chroma_sample != DPU_CHROMA_H1V2)
> >> + return -EINVAL; /*unsupported format */
> >> + opmode = CDM_HDMI_PACK_OP_MODE_EN;
> >> + opmode |= (fmt->chroma_sample << 1);
> >> + }
> >> +
> >> + csc |= CDM_CSC10_OP_MODE_DST_FMT_YUV;
> >> + csc &= ~CDM_CSC10_OP_MODE_SRC_FMT_YUV;
> >> + csc |= CDM_CSC10_OP_MODE_EN;
> >> +
> >> + if (ctx && ctx->ops.bind_pingpong_blk)
> >> + ctx->ops.bind_pingpong_blk(ctx, cdm->pp_id);
> >> +
> >> + DPU_REG_WRITE(c, CDM_CSC_10_OPMODE, csc);
> >> + DPU_REG_WRITE(c, CDM_HDMI_PACK_OP_MODE, opmode);
> >> + return 0;
> >> +}
> >> +
> >> +static void dpu_hw_cdm_bind_pingpong_blk(struct dpu_hw_cdm *ctx, const enum dpu_pingpong pp)
> >> +{
> >> + struct dpu_hw_blk_reg_map *c;
> >> + int mux_cfg = 0xF; /* Disabled */
> >
> > lowercase hex. And it is easier to move it to the if (pp) condition,
> > like it was done for INTF or WB.
> >
>
> ack for the lowercase hex.
>
> hmm, i followed the dpu_hw_dsc_1_2 model, so basically for 0
> (PINGPONG_NONE) cases it will stay at 0xf.
My preference was towards more explicit code, like INTF or WB.
>
> >> +
> >> + c = &ctx->hw;
> >> +
> >> + if (pp)
> >> + mux_cfg = (pp - PINGPONG_0) & 0x7;
> >> +
> >> + DPU_REG_WRITE(c, CDM_MUX, mux_cfg);
> >> +}
> >> +
> >> +struct dpu_hw_cdm *dpu_hw_cdm_init(struct drm_device *dev,
> >> + const struct dpu_cdm_cfg *cfg, void __iomem *addr,
> >> + const struct dpu_mdss_version *mdss_rev)
> >> +{
> >> + struct dpu_hw_cdm *c;
> >> +
> >> + c = drmm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> >> + if (!c)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + c->hw.blk_addr = addr + cfg->base;
> >> + c->hw.log_mask = DPU_DBG_MASK_CDM;
> >> +
> >> + /* Assign ops */
> >> + c->idx = cfg->id;
> >> + c->caps = cfg;
> >> +
> >> + c->ops.enable = dpu_hw_cdm_enable;
> >> + if (mdss_rev->core_major_ver >= 5)
> >> + c->ops.bind_pingpong_blk = dpu_hw_cdm_bind_pingpong_blk;
> >> +
> >> + return c;
> >> +}
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
> >> new file mode 100644
> >> index 000000000000..e7d57dbd6103
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h
> >> @@ -0,0 +1,130 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +/*
> >> + * Copyright (c) 2023, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +#ifndef _DPU_HW_CDM_H
> >> +#define _DPU_HW_CDM_H
> >> +
> >> +#include "dpu_hw_mdss.h"
> >> +#include "dpu_hw_top.h"
> >> +
> >> +struct dpu_hw_cdm;
> >> +
> >> +/**
> >> + * struct dpu_hw_cdm_cfg : current configuration of CDM block
> >> + *
> >> + * @output_width: output ROI width of CDM block
> >> + * @output_height: output ROI height of CDM block
> >> + * @output_bit_depth: output bit-depth of CDM block
> >> + * @h_cdwn_type: downsample type used for horizontal pixels
> >> + * @v_cdwn_type: downsample type used for vertical pixels
> >> + * @output_fmt: handle to dpu_format of CDM block
> >> + * @csc_cfg: handle to CSC matrix programmed for CDM block
> >> + * @output_type: interface to which CDM is paired (HDMI/WB)
> >> + * @pp_id: ping-pong block to which CDM is bound to
> >> + */
> >> +struct dpu_hw_cdm_cfg {
> >> + u32 output_width;
> >> + u32 output_height;
> >> + u32 output_bit_depth;
> >> + u32 h_cdwn_type;
> >> + u32 v_cdwn_type;
> >> + const struct dpu_format *output_fmt;
> >> + const struct dpu_csc_cfg *csc_cfg;
> >> + u32 output_type;
> >> + int pp_id;
> >> +};
> >> +
> >> +/*
> >> + * These values are used indicate which type of downsample is used
> >> + * in the horizontal/vertical direction for the CDM block.
> >> + */
> >> +enum dpu_hw_cdwn_type {
> >> + CDM_CDWN_DISABLE,
> >> + CDM_CDWN_PIXEL_DROP,
> >> + CDM_CDWN_AVG,
> >> + CDM_CDWN_COSITE,
> >> + CDM_CDWN_OFFSITE,
> >> +};
> >> +
> >> +/*
> >> + * CDM block can be paired with WB or HDMI block. These values match
> >> + * the input with which the CDM block is paired.
> >> + */
> >> +enum dpu_hw_cdwn_output_type {
> >> + CDM_CDWN_OUTPUT_HDMI,
> >> + CDM_CDWN_OUTPUT_WB,
> >> +};
> >> +
> >> +/*
> >> + * CDM block can give an 8-bit or 10-bit output. These values
> >> + * are used to indicate the output bit depth of CDM block
> >> + */
> >> +enum dpu_hw_cdwn_output_bit_depth {
> >> + CDM_CDWN_OUTPUT_8BIT,
> >> + CDM_CDWN_OUTPUT_10BIT,
> >> +};
> >> +
> >> +/**
> >> + * struct dpu_hw_cdm_ops : Interface to the chroma down Hw driver functions
> >> + * Assumption is these functions will be called after
> >> + * clocks are enabled
> >> + * @enable: Enables the output to interface and programs the
> >> + * output packer
> >> + * @bind_pingpong_blk: enable/disable the connection with pingpong which
> >> + * will feed pixels to this cdm
> >> + */
> >> +struct dpu_hw_cdm_ops {
> >> + /**
> >> + * Enable the CDM module
> >> + * @cdm Pointer to chroma down context
> >> + */
> >> + int (*enable)(struct dpu_hw_cdm *cdm, struct dpu_hw_cdm_cfg *cfg);
> >> +
> >> + /**
> >> + * Enable/disable the connection with pingpong
> >> + * @cdm Pointer to chroma down context
> >> + * @pp pingpong block id.
> >> + */
> >> + void (*bind_pingpong_blk)(struct dpu_hw_cdm *cdm, const enum dpu_pingpong pp);
> >> +};
> >> +
> >> +/**
> >> + * struct dpu_hw_cdm - cdm description
> >> + * @base: Hardware block base structure
> >> + * @hw: Block hardware details
> >> + * @idx: CDM index
> >> + * @caps: Pointer to cdm_cfg
> >> + * @ops: handle to operations possible for this CDM
> >> + */
> >> +struct dpu_hw_cdm {
> >> + struct dpu_hw_blk base;
> >> + struct dpu_hw_blk_reg_map hw;
> >> +
> >> + /* chroma down */
> >> + const struct dpu_cdm_cfg *caps;
> >> + enum dpu_cdm idx;
> >> +
> >> + /* ops */
> >> + struct dpu_hw_cdm_ops ops;
> >> +};
> >> +
> >> +/**
> >> + * dpu_hw_cdm_init - initializes the cdm hw driver object.
> >> + * should be called once before accessing every cdm.
> >> + * @dev: DRM device handle
> >> + * @cdm: CDM catalog entry for which driver object is required
> >> + * @addr : mapped register io address of MDSS
> >> + * @mdss_rev: mdss hw core revision
> >> + */
> >> +struct dpu_hw_cdm *dpu_hw_cdm_init(struct drm_device *dev,
> >> + const struct dpu_cdm_cfg *cdm, void __iomem *addr,
> >> + const struct dpu_mdss_version *mdss_rev);
> >> +
> >> +static inline struct dpu_hw_cdm *to_dpu_hw_cdm(struct dpu_hw_blk *hw)
> >> +{
> >> + return container_of(hw, struct dpu_hw_cdm, base);
> >> +}
> >> +
> >> +#endif /*_DPU_HW_CDM_H */
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> >> index f319c8232ea5..9db4cf61bd29 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> >> @@ -466,6 +466,7 @@ struct dpu_mdss_color {
> >> #define DPU_DBG_MASK_ROT (1 << 9)
> >> #define DPU_DBG_MASK_DSPP (1 << 10)
> >> #define DPU_DBG_MASK_DSC (1 << 11)
> >> +#define DPU_DBG_MASK_CDM (1 << 12)
> >>
> >> /**
> >> * struct dpu_hw_tear_check - Struct contains parameters to configure
> >> --
> >> 2.40.1
> >>
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 01/15] drm/msm/dpu: add formats check for writeback encoder
2023-12-12 17:16 ` Abhinav Kumar
@ 2023-12-12 21:32 ` Dmitry Baryshkov
0 siblings, 0 replies; 25+ messages in thread
From: Dmitry Baryshkov @ 2023-12-12 21:32 UTC (permalink / raw
To: Abhinav Kumar
Cc: freedreno, Rob Clark, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter, dri-devel, seanpaul, quic_jesszhan, linux-arm-msm,
linux-kernel
On Tue, 12 Dec 2023 at 19:17, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/11/2023 10:40 PM, Dmitry Baryshkov wrote:
> > On Tue, 12 Dec 2023 at 02:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> In preparation for adding more formats to dpu writeback add
> >> format validation to it to fail any unsupported formats.
> >>
> >> changes in v3:
> >> - rebase on top of msm-next
> >> - replace drm_atomic_helper_check_wb_encoder_state() with
> >> drm_atomic_helper_check_wb_connector_state() due to the
> >> rebase
> >>
> >> changes in v2:
> >> - correct some grammar in the commit text
> >>
> >> Fixes: d7d0e73f7de3 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for writeback")
> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >> ---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 7 +++++++
> >> 1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >> index bb94909caa25..425415d45ec1 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >> @@ -272,6 +272,7 @@ static int dpu_encoder_phys_wb_atomic_check(
> >> {
> >> struct drm_framebuffer *fb;
> >> const struct drm_display_mode *mode = &crtc_state->mode;
> >> + int ret;
> >>
> >> DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
> >> phys_enc->hw_wb->idx, mode->name, mode->hdisplay, mode->vdisplay);
> >> @@ -308,6 +309,12 @@ static int dpu_encoder_phys_wb_atomic_check(
> >> return -EINVAL;
> >> }
> >>
> >> + ret = drm_atomic_helper_check_wb_connector_state(conn_state->connector, conn_state->state);
> >> + if (ret < 0) {
> >> + DPU_ERROR("invalid pixel format %p4cc\n", &fb->format->format);
> >> + return ret;
> >> + }
> >
> > There is no guarantee that there will be no other checks added to this
> > helper. So, I think this message is incorrect. If you wish, you can
> > promote the level of the message in the helper itself.
> > On the other hand, we rarely print such messages by default. Most of
> > the checks use drm_dbg.
> >
>
> hmm...actually drm_atomic_helper_check_wb_connector_state() already has
> a debug message to indicate invalid pixel formats.
>
> You are right, i should perhaps just say that "atomic_check failed" or
> something.
>
> I can make this a DPU_DEBUG. Actually I didnt know that we are not
> supposed to print out atomic_check() errors. Is it perhaps because its
> okay for check to fail?
There are no messages by default there, because otherwise it is so
easy for the user to overspam the dmesg and thus syslog / journal. DoS
on the plate.
>
> But then we would not know why it failed.
Think about the user of X11. They don't see the console. And by
default in the contemporary distros they won't be able to check dmesg.
So if a commit fails, they have to deduce anyway, why did it fail.
>
> >> +
> >> return 0;
> >> }
> >>
> >> --
> >> 2.40.1
> >>
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2023-12-12 21:33 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20231212002245.23715-1-quic_abhinavk@quicinc.com>
2023-12-12 0:22 ` [PATCH v3 01/15] drm/msm/dpu: add formats check for writeback encoder Abhinav Kumar
2023-12-12 6:40 ` Dmitry Baryshkov
2023-12-12 17:16 ` Abhinav Kumar
2023-12-12 21:32 ` Dmitry Baryshkov
2023-12-12 0:22 ` [PATCH v3 02/15] drm/msm/dpu: rename dpu_encoder_phys_wb_setup_cdp to match its functionality Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 03/15] drm/msm/dpu: fix writeback programming for YUV cases Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 04/15] drm/msm/dpu: move csc matrices to dpu_hw_util Abhinav Kumar
2023-12-12 6:41 ` Dmitry Baryshkov
2023-12-12 0:22 ` [PATCH v3 05/15] drm/msm/dpu: add cdm blocks to sc7280 dpu_hw_catalog Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 06/15] drm/msm/dpu: add cdm blocks to sm8250 dpu_hw_catalog Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 07/15] drm/msm/dpu: add dpu_hw_cdm abstraction for CDM block Abhinav Kumar
2023-12-12 9:40 ` Dmitry Baryshkov
2023-12-12 17:51 ` Abhinav Kumar
2023-12-12 21:29 ` Dmitry Baryshkov
2023-12-12 0:22 ` [PATCH v3 08/15] drm/msm/dpu: add cdm blocks to RM Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 09/15] drm/msm/dpu: add support to allocate CDM from RM Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 10/15] drm/msm/dpu: add CDM related logic to dpu_hw_ctl layer Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 11/15] drm/msm/dpu: add an API to setup the CDM block for writeback Abhinav Kumar
2023-12-12 7:17 ` Dmitry Baryshkov
2023-12-12 0:22 ` [PATCH v3 12/15] drm/msm/dpu: plug-in the cdm related bits to writeback setup Abhinav Kumar
2023-12-12 0:22 ` [PATCH v3 13/15] drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output Abhinav Kumar
2023-12-12 7:21 ` Dmitry Baryshkov
2023-12-12 0:22 ` [PATCH v3 14/15] drm/msm/dpu: introduce separate wb2_format arrays for rgb and yuv Abhinav Kumar
2023-12-12 7:22 ` Dmitry Baryshkov
2023-12-12 0:22 ` [PATCH v3 15/15] drm/msm/dpu: add cdm blocks to dpu snapshot Abhinav Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).