Re: [Freedreno] (subset) [PATCH v2 0/8] MDSS reg bus interconnect
On Wed, 12 Jul 2023 15:11:37 +0300, Dmitry Baryshkov wrote: > Per agreement with Konrad, picked up this patch series. > > Apart from the already handled data bus (MAS_MDP_Pn<->DDR), there's > another path that needs to be handled to ensure MDSS functions properly, > namely the "reg bus", a.k.a the CPU-MDSS interconnect. > > Gating that path may have a variety of effects. from none to otherwise > inexplicable DSI timeouts. > > [...] Applied, thanks! [8/8] arm64: dts: qcom: sm8450: provide MDSS cfg interconnect commit: 4e125191e6cb00d6c3f3a8e1b67fd242e639b3c3 Best regards, -- Bjorn Andersson
[Freedreno] [PATCH v2 2/4] drm/msm/dpu: drop the `smart_dma_priority' field from struct dpu_sspp_sub_blks
In preparation to deduplicating SSPP subblocks, drop the (unused)
`smart_dma_priority' field from struct dpu_sspp_sub_blks. If it is
needed later (e.g. for SmartDMA v1), it should be added to the SSPP
declarations themselves.
Signed-off-by: Dmitry Baryshkov
---
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 68 +--
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 2 -
2 files changed, 32 insertions(+), 38 deletions(-)
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 daec3f2758e3..63304c2ee6d9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -251,11 +251,10 @@ static const uint32_t wb2_formats[] = {
*/
/* SSPP common configuration */
-#define _VIG_SBLK(sdma_pri, qseed_ver) \
+#define _VIG_SBLK(qseed_ver) \
{ \
.maxdwnscale = MAX_DOWNSCALE_RATIO, \
.maxupscale = MAX_UPSCALE_RATIO, \
- .smart_dma_priority = sdma_pri, \
.scaler_blk = {.name = "scaler", \
.base = 0xa00, .len = 0xa0,}, \
.csc_blk = {.name = "csc", \
@@ -267,11 +266,10 @@ static const uint32_t wb2_formats[] = {
.rotation_cfg = NULL, \
}
-#define _VIG_SBLK_ROT(sdma_pri, qseed_ver, rot_cfg) \
+#define _VIG_SBLK_ROT(qseed_ver, rot_cfg) \
{ \
.maxdwnscale = MAX_DOWNSCALE_RATIO, \
.maxupscale = MAX_UPSCALE_RATIO, \
- .smart_dma_priority = sdma_pri, \
.scaler_blk = {.name = "scaler", \
.base = 0xa00, .len = 0xa0,}, \
.csc_blk = {.name = "csc", \
@@ -283,11 +281,10 @@ static const uint32_t wb2_formats[] = {
.rotation_cfg = rot_cfg, \
}
-#define _DMA_SBLK(sdma_pri) \
+#define _DMA_SBLK \
{ \
.maxdwnscale = SSPP_UNITY_SCALE, \
.maxupscale = SSPP_UNITY_SCALE, \
- .smart_dma_priority = sdma_pri, \
.format_list = plane_formats, \
.num_formats = ARRAY_SIZE(plane_formats), \
.virt_format_list = plane_formats, \
@@ -295,13 +292,13 @@ static const uint32_t wb2_formats[] = {
}
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_0 =
- _VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_1 =
- _VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_2 =
- _VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_3 =
- _VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(DPU_SSPP_SCALER_QSEED3);
static const struct dpu_rotation_cfg dpu_rot_sc7280_cfg_v2 = {
.rot_maxheight = 1088,
@@ -310,64 +307,63 @@ static const struct dpu_rotation_cfg
dpu_rot_sc7280_cfg_v2 = {
};
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_0 =
- _VIG_SBLK(5, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_1 =
- _VIG_SBLK(6, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_2 =
- _VIG_SBLK(7, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_3 =
- _VIG_SBLK(8, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(DPU_SSPP_SCALER_QSEED3);
-static const struct dpu_sspp_sub_blks sdm845_dma_sblk_0 = _DMA_SBLK(1);
-static const struct dpu_sspp_sub_blks sdm845_dma_sblk_1 = _DMA_SBLK(2);
-static const struct dpu_sspp_sub_blks sdm845_dma_sblk_2 = _DMA_SBLK(3);
-static const struct dpu_sspp_sub_blks sdm845_dma_sblk_3 = _DMA_SBLK(4);
+static const struct dpu_sspp_sub_blks sdm845_dma_sblk_0 = _DMA_SBLK;
+static const struct dpu_sspp_sub_blks sdm845_dma_sblk_1 = _DMA_SBLK;
+static const struct dpu_sspp_sub_blks sdm845_dma_sblk_2 = _DMA_SBLK;
+static const struct dpu_sspp_sub_blks sdm845_dma_sblk_3 = _DMA_SBLK;
static const struct dpu_sspp_sub_blks sc7180_vig_sblk_0 =
- _VIG_SBLK(4, DPU_SSPP_SCALER_QSEED4);
+ _VIG_SBLK(DPU_SSPP_SCALER_QSEED4);
static const struct dpu_sspp_sub_blks sc7280_vig_sblk_0 =
- _VIG_SBLK_ROT(4, DPU_SSPP_SCALER_QSEED4,
_rot_sc7280_cfg_v2);
+ _VIG_SBLK_ROT(DPU_SSPP_SCALER_QSEED4,
_rot_sc7280_cfg_v2);
static const struct
[Freedreno] [PATCH v2 1/4] drm/msm/dpu: drop the `id' field from DPU_HW_SUBBLK_INFO
The field `id' is not used for subblocks. The handling code usually
knows, which sub-block it is now looking at. Drop the field completely.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 16 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
2 files changed, 6 insertions(+), 12 deletions(-)
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 2522e06c5262..daec3f2758e3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -257,10 +257,8 @@ static const uint32_t wb2_formats[] = {
.maxupscale = MAX_UPSCALE_RATIO, \
.smart_dma_priority = sdma_pri, \
.scaler_blk = {.name = "scaler", \
- .id = qseed_ver, \
.base = 0xa00, .len = 0xa0,}, \
.csc_blk = {.name = "csc", \
- .id = DPU_SSPP_CSC_10BIT, \
.base = 0x1a00, .len = 0x100,}, \
.format_list = plane_formats_yuv, \
.num_formats = ARRAY_SIZE(plane_formats_yuv), \
@@ -275,10 +273,8 @@ static const uint32_t wb2_formats[] = {
.maxupscale = MAX_UPSCALE_RATIO, \
.smart_dma_priority = sdma_pri, \
.scaler_blk = {.name = "scaler", \
- .id = qseed_ver, \
.base = 0xa00, .len = 0xa0,}, \
.csc_blk = {.name = "csc", \
- .id = DPU_SSPP_CSC_10BIT, \
.base = 0x1a00, .len = 0x100,}, \
.format_list = plane_formats_yuv, \
.num_formats = ARRAY_SIZE(plane_formats_yuv), \
@@ -423,12 +419,12 @@ static const struct dpu_lm_sub_blks qcm2290_lm_sblk = {
* DSPP sub blocks config
*/
static const struct dpu_dspp_sub_blks msm8998_dspp_sblk = {
- .pcc = {.name = "pcc", .id = DPU_DSPP_PCC, .base = 0x1700,
+ .pcc = {.name = "pcc", .base = 0x1700,
.len = 0x90, .version = 0x10007},
};
static const struct dpu_dspp_sub_blks sdm845_dspp_sblk = {
- .pcc = {.name = "pcc", .id = DPU_DSPP_PCC, .base = 0x1700,
+ .pcc = {.name = "pcc", .base = 0x1700,
.len = 0x90, .version = 0x4},
};
@@ -436,19 +432,19 @@ static const struct dpu_dspp_sub_blks sdm845_dspp_sblk = {
* PINGPONG sub blocks config
*/
static const struct dpu_pingpong_sub_blks sdm845_pp_sblk_te = {
- .te2 = {.name = "te2", .id = DPU_PINGPONG_TE2, .base = 0x2000, .len =
0x0,
+ .te2 = {.name = "te2", .base = 0x2000, .len = 0x0,
.version = 0x1},
- .dither = {.name = "dither", .id = DPU_PINGPONG_DITHER, .base = 0x30e0,
+ .dither = {.name = "dither", .base = 0x30e0,
.len = 0x20, .version = 0x1},
};
static const struct dpu_pingpong_sub_blks sdm845_pp_sblk = {
- .dither = {.name = "dither", .id = DPU_PINGPONG_DITHER, .base = 0x30e0,
+ .dither = {.name = "dither", .base = 0x30e0,
.len = 0x20, .version = 0x1},
};
static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
- .dither = {.name = "dither", .id = DPU_PINGPONG_DITHER, .base = 0xe0,
+ .dither = {.name = "dither", .base = 0xe0,
.len = 0x20, .version = 0x2},
};
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 1d150091da9c..4e8fc3bbc240 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -274,14 +274,12 @@ enum {
/**
* MACRO DPU_HW_SUBBLK_INFO - information of HW sub-block inside DPU
* @name: string name for debug purposes
- * @id:enum identifying this sub-block
* @base: offset of this sub-block relative to the block
* offset
* @lenregister block length of this sub-block
*/
#define DPU_HW_SUBBLK_INFO \
char name[DPU_HW_BLK_NAME_LEN]; \
- u32 id; \
u32 base; \
u32 len
--
2.39.2
[Freedreno] [PATCH v2 4/4] drm/msm/dpu: drop DPU_HW_SUBBLK_INFO macro
As the subblock info is now mostly gone, inline and drop the macro
DPU_HW_SUBBLK_INFO.
Signed-off-by: Dmitry Baryshkov
---
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 40 ++-
1 file changed, 21 insertions(+), 19 deletions(-)
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 cc1800e324dd..61c6caf1b185 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -271,48 +271,50 @@ enum {
u32 len; \
unsigned long features
-/**
- * MACRO DPU_HW_SUBBLK_INFO - information of HW sub-block inside DPU
- * @name: string name for debug purposes
- * @base: offset of this sub-block relative to the block
- * offset
- * @lenregister block length of this sub-block
- */
-#define DPU_HW_SUBBLK_INFO \
- char name[DPU_HW_BLK_NAME_LEN]; \
- u32 base; \
- u32 len
-
/**
* struct dpu_scaler_blk: Scaler information
- * @info: HW register and features supported by this sub-blk
+ * @name: string name for debug purposes
+ * @base: offset of this sub-block relative to the block offset
+ * @len: register block length of this sub-block
* @version: qseed block revision
*/
struct dpu_scaler_blk {
- DPU_HW_SUBBLK_INFO;
+ char name[DPU_HW_BLK_NAME_LEN];
+ u32 base;
+ u32 len;
u32 version;
};
struct dpu_csc_blk {
- DPU_HW_SUBBLK_INFO;
+ char name[DPU_HW_BLK_NAME_LEN];
+ u32 base;
+ u32 len;
};
/**
* struct dpu_pp_blk : Pixel processing sub-blk information
- * @info: HW register and features supported by this sub-blk
+ * @name: string name for debug purposes
+ * @base: offset of this sub-block relative to the block offset
+ * @len: register block length of this sub-block
* @version: HW Algorithm version
*/
struct dpu_pp_blk {
- DPU_HW_SUBBLK_INFO;
+ char name[DPU_HW_BLK_NAME_LEN];
+ u32 base;
+ u32 len;
u32 version;
};
/**
* struct dpu_dsc_blk - DSC Encoder sub-blk information
- * @info: HW register and features supported by this sub-blk
+ * @name: string name for debug purposes
+ * @base: offset of this sub-block relative to the block offset
+ * @len: register block length of this sub-block
*/
struct dpu_dsc_blk {
- DPU_HW_SUBBLK_INFO;
+ char name[DPU_HW_BLK_NAME_LEN];
+ u32 base;
+ u32 len;
};
/**
--
2.39.2
[Freedreno] [PATCH v2 3/4] drm/msm/dpu: deduplicate some (most) of SSPP sub-blocks
As we have dropped the variadic parts of SSPP sub-blocks declarations,
deduplicate them now, reducing memory cruft.
Signed-off-by: Dmitry Baryshkov
---
.../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 16 +++---
.../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h| 16 +++---
.../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h| 16 +++---
.../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 16 +++---
.../msm/disp/dpu1/catalog/dpu_5_4_sm6125.h| 6 +-
.../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h| 16 +++---
.../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h| 8 +--
.../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h| 4 +-
.../msm/disp/dpu1/catalog/dpu_6_4_sm6350.h| 8 +--
.../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h | 4 +-
.../msm/disp/dpu1/catalog/dpu_6_9_sm6375.h| 4 +-
.../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h| 16 +++---
.../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h| 8 +--
.../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 16 +++---
.../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h| 16 +++---
.../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h| 20 +++
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 56 +++
17 files changed, 102 insertions(+), 144 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
index 7d87dc2d7b1b..7aa3738d2edd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
@@ -75,7 +75,7 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
.name = "sspp_0", .id = SSPP_VIG0,
.base = 0x4000, .len = 0x1ac,
.features = VIG_MSM8998_MASK,
- .sblk = _vig_sblk_0,
+ .sblk = _vig_sblk_qseed3,
.xin_id = 0,
.type = SSPP_TYPE_VIG,
.clk_ctrl = DPU_CLK_CTRL_VIG0,
@@ -83,7 +83,7 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
.name = "sspp_1", .id = SSPP_VIG1,
.base = 0x6000, .len = 0x1ac,
.features = VIG_MSM8998_MASK,
- .sblk = _vig_sblk_1,
+ .sblk = _vig_sblk_qseed3,
.xin_id = 4,
.type = SSPP_TYPE_VIG,
.clk_ctrl = DPU_CLK_CTRL_VIG1,
@@ -91,7 +91,7 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
.name = "sspp_2", .id = SSPP_VIG2,
.base = 0x8000, .len = 0x1ac,
.features = VIG_MSM8998_MASK,
- .sblk = _vig_sblk_2,
+ .sblk = _vig_sblk_qseed3,
.xin_id = 8,
.type = SSPP_TYPE_VIG,
.clk_ctrl = DPU_CLK_CTRL_VIG2,
@@ -99,7 +99,7 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
.name = "sspp_3", .id = SSPP_VIG3,
.base = 0xa000, .len = 0x1ac,
.features = VIG_MSM8998_MASK,
- .sblk = _vig_sblk_3,
+ .sblk = _vig_sblk_qseed3,
.xin_id = 12,
.type = SSPP_TYPE_VIG,
.clk_ctrl = DPU_CLK_CTRL_VIG3,
@@ -107,7 +107,7 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
.name = "sspp_8", .id = SSPP_DMA0,
.base = 0x24000, .len = 0x1ac,
.features = DMA_MSM8998_MASK,
- .sblk = _dma_sblk_0,
+ .sblk = _dma_sblk,
.xin_id = 1,
.type = SSPP_TYPE_DMA,
.clk_ctrl = DPU_CLK_CTRL_DMA0,
@@ -115,7 +115,7 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
.name = "sspp_9", .id = SSPP_DMA1,
.base = 0x26000, .len = 0x1ac,
.features = DMA_MSM8998_MASK,
- .sblk = _dma_sblk_1,
+ .sblk = _dma_sblk,
.xin_id = 5,
.type = SSPP_TYPE_DMA,
.clk_ctrl = DPU_CLK_CTRL_DMA1,
@@ -123,7 +123,7 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
.name = "sspp_10", .id = SSPP_DMA2,
.base = 0x28000, .len = 0x1ac,
.features = DMA_CURSOR_MSM8998_MASK,
- .sblk = _dma_sblk_2,
+ .sblk = _dma_sblk,
.xin_id = 9,
.type = SSPP_TYPE_DMA,
.clk_ctrl = DPU_CLK_CTRL_DMA2,
@@ -131,7 +131,7 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
.name = "sspp_11", .id = SSPP_DMA3,
.base = 0x2a000, .len = 0x1ac,
.features = DMA_CURSOR_MSM8998_MASK,
- .sblk = _dma_sblk_3,
+ .sblk = _dma_sblk,
.xin_id = 13,
.type = SSPP_TYPE_DMA,
.clk_ctrl = DPU_CLK_CTRL_DMA3,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
index 87459cf40895..02d33ea7e25f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
+++
[Freedreno] [PATCH v2 0/4] drm/msm/dpu: simplify DPU sub-blocks info
The handling code also usually knows, which sub-block it is now looking at. Drop unused 'id' field and arguments and merge some of sub-block declarations. Changes since v1: - Dropped the patch dropping 'name' field (Abhinav). - Deduplicate equivalent SBLK definitions. - Dropped the dpu_csc_blk and dpu_dsc_blk merge. Dmitry Baryshkov (4): drm/msm/dpu: drop the `id' field from DPU_HW_SUBBLK_INFO drm/msm/dpu: drop the `smart_dma_priority' field from struct dpu_sspp_sub_blks drm/msm/dpu: deduplicate some (most) of SSPP sub-blocks drm/msm/dpu: drop DPU_HW_SUBBLK_INFO macro .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 16 +-- .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h| 16 +-- .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h| 16 +-- .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 16 +-- .../msm/disp/dpu1/catalog/dpu_5_4_sm6125.h| 6 +- .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h| 16 +-- .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h| 8 +- .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h| 4 +- .../msm/disp/dpu1/catalog/dpu_6_4_sm6350.h| 8 +- .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h | 4 +- .../msm/disp/dpu1/catalog/dpu_6_9_sm6375.h| 4 +- .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h| 16 +-- .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h| 8 +- .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 16 +-- .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h| 16 +-- .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h| 20 ++-- .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 100 +- .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 44 18 files changed, 141 insertions(+), 193 deletions(-) -- 2.39.2
[Freedreno] [PATCH v3 0/3] Fix IS_ERR() vs NULL check for drm
v3: - Update the second patch: 1. change IS_ERR to IS_ERR_OR_NULL 2. add Dmitry's R-b in this revision: link: https://patchwork.freedesktop.org/patch/511035/?series=110745=1 Thanks! v2: - I'm sorry I missed some emails, these patches were submitted last year, now let me resend it with the following changes: 1. remove the patch: "drm/msm: Fix IS_ERR_OR_NULL() vs NULL check in msm_icc_get()" 2. remove the patch: "drm/vc4: kms: Fix IS_ERR() vs NULL check for vc4_kms" 3. add "Reviewed-by: Abhinav Kumar " to the second patch. Thanks! v1: - This series contains a few fixup patches, to fix IS_ERR() vs NULL check for drm, and avoid a potential null-ptr-defer issue, too. Thanks! Gaosheng Cui (3): drm/panel: Fix IS_ERR() vs NULL check in nt35950_probe() drm/msm: Fix IS_ERR() vs NULL check in a5xx_submit_in_rb() drm/komeda: Fix IS_ERR() vs NULL check in komeda_component_get_avail_scaler() drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +- drivers/gpu/drm/panel/panel-novatek-nt35950.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) -- 2.25.1
[Freedreno] [PATCH v3 2/3] drm/msm: Fix IS_ERR() vs NULL check in a5xx_submit_in_rb()
The msm_gem_get_vaddr() returns an ERR_PTR() on failure, we should
use IS_ERR() to check the return value.
Fixes: 6a8bd08d0465 ("drm/msm: add sudo flag to submit ioctl")
Signed-off-by: Gaosheng Cui
Reviewed-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
Reviewed-by: Akhil P Oommen
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index a99310b68793..bbb1bf33f98e 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -89,7 +89,7 @@ static void a5xx_submit_in_rb(struct msm_gpu *gpu, struct
msm_gem_submit *submit
* since we've already mapped it once in
* submit_reloc()
*/
- if (WARN_ON(!ptr))
+ if (WARN_ON(IS_ERR_OR_NULL(ptr)))
return;
for (i = 0; i < dwords; i++) {
--
2.25.1
[Freedreno] [PATCH v3 3/3] drm/komeda: Fix IS_ERR() vs NULL check in komeda_component_get_avail_scaler()
The komeda_pipeline_get_state() returns an ERR_PTR() on failure, we should
use IS_ERR() to check the return value.
Fixes: 502932a03fce ("drm/komeda: Add the initial scaler support for CORE")
Signed-off-by: Gaosheng Cui
Reviewed-by: Liviu Dudau
---
drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index 3276a3e82c62..e9c92439398d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -259,7 +259,7 @@ komeda_component_get_avail_scaler(struct komeda_component
*c,
u32 avail_scalers;
pipe_st = komeda_pipeline_get_state(c->pipeline, state);
- if (!pipe_st)
+ if (IS_ERR(pipe_st))
return NULL;
avail_scalers = (pipe_st->active_comps & KOMEDA_PIPELINE_SCALERS) ^
--
2.25.1
[Freedreno] [PATCH v3 1/3] drm/panel: Fix IS_ERR() vs NULL check in nt35950_probe()
The mipi_dsi_device_register_full() returns an ERR_PTR() on failure,
we should use IS_ERR() to check the return value.
Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC
panels")
Signed-off-by: Gaosheng Cui
---
drivers/gpu/drm/panel/panel-novatek-nt35950.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
index 8b108ac80b55..4903bbf1df55 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
@@ -571,7 +571,7 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
}
nt->dsi[1] = mipi_dsi_device_register_full(dsi_r_host, info);
- if (!nt->dsi[1]) {
+ if (IS_ERR(nt->dsi[1])) {
dev_err(dev, "Cannot get secondary DSI node\n");
return -ENODEV;
}
--
2.25.1
Re: [Freedreno] [PATCH v2] drm/msm/dsi: Enable DATABUS_WIDEN for DSI command mode
On 14/07/2023 03:21, Jessica Zhang wrote:
DSI 6G v2.5.x+ and DPU 7.x+ support a data-bus widen mode that allows DSI
to send 48 bits of compressed data per pclk instead of 24.
For all chipsets that support this mode, enable it whenever DSC is
enabled as recommended by the hardware programming guide.
Only enable this for command mode as we are currently unable to validate
it for video mode.
Signed-off-by: Jessica Zhang
---
Note: The dsi.xml.h changes were generated using the headergen2 script in
envytools [2], but the changes to the copyright and rules-ng-ng source file
paths were dropped.
Separate commit please, so that it can be replaced by headers sync with
Mesa3d.
[1] https://patchwork.freedesktop.org/series/120580/
[2] https://github.com/freedreno/envytools/
--
Changes in v2:
- Rebased on top of "drm/msm/dpu: Re-introduce dpu core revision"
- Squashed all commits to avoid breaking feature if the series is only
partially applied
No. Please unsquash it. Please design the series so that the patches
work even if it is only partially applied.
- Moved DATABUS_WIDEN bit setting to dsi_ctr_enable() (Marijn)
- Have DPU check if wide bus is requested by output driver (Dmitry)
- Introduced bytes_per_pclk variable for dsi_timing_setup() hdisplay adjustment
(Marijn)
- Link to v1:
https://lore.kernel.org/r/20230525-add-widebus-support-v1-0-c7069f2ef...@quicinc.com
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 10 ++
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 +++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 3 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h| 1 +
drivers/gpu/drm/msm/dsi/dsi.c | 5 +
drivers/gpu/drm/msm/dsi/dsi.h | 1 +
drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
drivers/gpu/drm/msm/dsi/dsi_host.c | 23 +-
drivers/gpu/drm/msm/msm_drv.h | 6 ++
9 files changed, 48 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index f0a2a1dca741..6aed63c06c1d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2411,6 +2411,7 @@ struct drm_encoder *dpu_encoder_init(struct drm_device
*dev,
struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
struct drm_encoder *drm_enc = NULL;
struct dpu_encoder_virt *dpu_enc = NULL;
+ int index = disp_info->h_tile_instance[0];
int ret = 0;
dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
@@ -2439,13 +2440,14 @@ struct drm_encoder *dpu_encoder_init(struct drm_device
*dev,
timer_setup(_enc->frame_done_timer,
dpu_encoder_frame_done_timeout, 0);
- if (disp_info->intf_type == INTF_DSI)
+ if (disp_info->intf_type == INTF_DSI) {
timer_setup(_enc->vsync_event_timer,
dpu_encoder_vsync_event_handler,
While you are touching this part, could you please drop
dpu_encoder_vsync_event_handler() and
dpu_encoder_vsync_event_work_handler(), they are useless?
0);
- else if (disp_info->intf_type == INTF_DP)
- dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
- priv->dp[disp_info->h_tile_instance[0]]);
+ dpu_enc->wide_bus_en =
msm_dsi_is_widebus_enabled(priv->dsi[index]);
+ } else if (disp_info->intf_type == INTF_DP) {
+ dpu_enc->wide_bus_en =
msm_dp_wide_bus_available(priv->dp[index]);
+ }
INIT_DELAYED_WORK(_enc->delayed_off_work,
dpu_encoder_off_work);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index df88358e7037..dace6168be2d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
phys_enc->hw_intf,
phys_enc->hw_pp->idx);
- if (intf_cfg.dsc != 0)
+ if (intf_cfg.dsc != 0) {
cmd_mode_cfg.data_compress = true;
+ cmd_mode_cfg.wide_bus_en =
dpu_encoder_is_widebus_enabled(phys_enc->parent);
+ }
if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf,
_mode_cfg);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 8ec6505d9e78..dc6f3febb574 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct
dpu_hw_intf *ctx,
if (cmd_mode_cfg->data_compress)
intf_cfg2
[Freedreno] [PATCH v2] drm/msm/dsi: Enable DATABUS_WIDEN for DSI command mode
DSI 6G v2.5.x+ and DPU 7.x+ support a data-bus widen mode that allows DSI
to send 48 bits of compressed data per pclk instead of 24.
For all chipsets that support this mode, enable it whenever DSC is
enabled as recommended by the hardware programming guide.
Only enable this for command mode as we are currently unable to validate
it for video mode.
Signed-off-by: Jessica Zhang
---
Note: The dsi.xml.h changes were generated using the headergen2 script in
envytools [2], but the changes to the copyright and rules-ng-ng source file
paths were dropped.
[1] https://patchwork.freedesktop.org/series/120580/
[2] https://github.com/freedreno/envytools/
--
Changes in v2:
- Rebased on top of "drm/msm/dpu: Re-introduce dpu core revision"
- Squashed all commits to avoid breaking feature if the series is only
partially applied
- Moved DATABUS_WIDEN bit setting to dsi_ctr_enable() (Marijn)
- Have DPU check if wide bus is requested by output driver (Dmitry)
- Introduced bytes_per_pclk variable for dsi_timing_setup() hdisplay adjustment
(Marijn)
- Link to v1:
https://lore.kernel.org/r/20230525-add-widebus-support-v1-0-c7069f2ef...@quicinc.com
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 10 ++
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 +++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 3 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h| 1 +
drivers/gpu/drm/msm/dsi/dsi.c | 5 +
drivers/gpu/drm/msm/dsi/dsi.h | 1 +
drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
drivers/gpu/drm/msm/dsi/dsi_host.c | 23 +-
drivers/gpu/drm/msm/msm_drv.h | 6 ++
9 files changed, 48 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index f0a2a1dca741..6aed63c06c1d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2411,6 +2411,7 @@ struct drm_encoder *dpu_encoder_init(struct drm_device
*dev,
struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
struct drm_encoder *drm_enc = NULL;
struct dpu_encoder_virt *dpu_enc = NULL;
+ int index = disp_info->h_tile_instance[0];
int ret = 0;
dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
@@ -2439,13 +2440,14 @@ struct drm_encoder *dpu_encoder_init(struct drm_device
*dev,
timer_setup(_enc->frame_done_timer,
dpu_encoder_frame_done_timeout, 0);
- if (disp_info->intf_type == INTF_DSI)
+ if (disp_info->intf_type == INTF_DSI) {
timer_setup(_enc->vsync_event_timer,
dpu_encoder_vsync_event_handler,
0);
- else if (disp_info->intf_type == INTF_DP)
- dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
- priv->dp[disp_info->h_tile_instance[0]]);
+ dpu_enc->wide_bus_en =
msm_dsi_is_widebus_enabled(priv->dsi[index]);
+ } else if (disp_info->intf_type == INTF_DP) {
+ dpu_enc->wide_bus_en =
msm_dp_wide_bus_available(priv->dp[index]);
+ }
INIT_DELAYED_WORK(_enc->delayed_off_work,
dpu_encoder_off_work);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index df88358e7037..dace6168be2d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
phys_enc->hw_intf,
phys_enc->hw_pp->idx);
- if (intf_cfg.dsc != 0)
+ if (intf_cfg.dsc != 0) {
cmd_mode_cfg.data_compress = true;
+ cmd_mode_cfg.wide_bus_en =
dpu_encoder_is_widebus_enabled(phys_enc->parent);
+ }
if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf,
_mode_cfg);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 8ec6505d9e78..dc6f3febb574 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct
dpu_hw_intf *ctx,
if (cmd_mode_cfg->data_compress)
intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
+ if (cmd_mode_cfg->wide_bus_en)
+ intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
+
DPU_REG_WRITE(>hw, INTF_CONFIG2, intf_cfg2);
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index 77f80531782b..c539025c418b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++
Re: [Freedreno] [PATCH 12/12] drm/msm/adreno: Switch to chip-id for identifying GPU
On Fri, 14 Jul 2023 at 01:06, Rob Clark wrote: > > On Thu, Jul 13, 2023 at 2:39 PM Akhil P Oommen > wrote: > > > > On Fri, Jul 07, 2023 at 06:45:42AM +0300, Dmitry Baryshkov wrote: > > > > > > On 07/07/2023 00:10, Rob Clark wrote: > > > > From: Rob Clark > > > > > > > > Since the revision becomes an opaque identifier with future GPUs, move > > > > away from treating different ranges of bits as having a given meaning. > > > > This means that we need to explicitly list different patch revisions in > > > > the device table. > > > > > > > > Signed-off-by: Rob Clark > > > > --- > > > > drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 2 +- > > > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +- > > > > drivers/gpu/drm/msm/adreno/a5xx_power.c| 2 +- > > > > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 13 ++- > > > > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 9 +- > > > > drivers/gpu/drm/msm/adreno/adreno_device.c | 128 ++--- > > > > drivers/gpu/drm/msm/adreno/adreno_gpu.c| 16 +-- > > > > drivers/gpu/drm/msm/adreno/adreno_gpu.h| 51 > > > > 8 files changed, 122 insertions(+), 110 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > > > > b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > > > > index 715436cb3996..8b4cdf95f445 100644 > > > > --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > > > > +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > > > > @@ -145,7 +145,7 @@ static void a4xx_enable_hwcg(struct msm_gpu *gpu) > > > > gpu_write(gpu, REG_A4XX_RBBM_CLOCK_DELAY_HLSQ, 0x0022); > > > > /* Early A430's have a timing issue with SP/TP power collapse; > > > >disabling HW clock gating prevents it. */ > > > > - if (adreno_is_a430(adreno_gpu) && adreno_gpu->rev.patchid < 2) > > > > + if (adreno_is_a430(adreno_gpu) && adreno_patchid(adreno_gpu) < 2) > > > > gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0); > > > > else > > > > gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0x); > > > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > > > b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > > > index f0803e94ebe5..70d2b5342cd9 100644 > > > > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > > > @@ -1744,6 +1744,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device > > > > *dev) > > > > struct msm_drm_private *priv = dev->dev_private; > > > > struct platform_device *pdev = priv->gpu_pdev; > > > > struct adreno_platform_config *config = pdev->dev.platform_data; > > > > + const struct adreno_info *info; > > > > struct a5xx_gpu *a5xx_gpu = NULL; > > > > struct adreno_gpu *adreno_gpu; > > > > struct msm_gpu *gpu; > > > > @@ -1770,7 +1771,15 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device > > > > *dev) > > > > nr_rings = 4; > > > > - if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev)) > > > > + /* > > > > +* Note that we wouldn't have been able to get this far if there is > > > > not > > > > +* a device table entry for this chip_id > > > > +*/ > > > > + info = adreno_find_info(config->chip_id); > > > > + if (WARN_ON(!info)) > > > > + return ERR_PTR(-EINVAL); > > > > + > > > > + if (info->revn == 510) > > > > nr_rings = 1; > > > > ret = adreno_gpu_init(dev, pdev, adreno_gpu, , nr_rings); > > > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c > > > > b/drivers/gpu/drm/msm/adreno/a5xx_power.c > > > > index 0e63a1429189..7705f8010484 100644 > > > > --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c > > > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c > > > > @@ -179,7 +179,7 @@ static void a540_lm_setup(struct msm_gpu *gpu) > > > > /* The battery current limiter isn't enabled for A540 */ > > > > config = AGC_LM_CONFIG_BCL_DISABLED; > > > > - config |= adreno_gpu->rev.patchid << > > > > AGC_LM_CONFIG_GPU_VERSION_SHIFT; > > > > + config |= adreno_patchid(adreno_gpu) << > > > > AGC_LM_CONFIG_GPU_VERSION_SHIFT; > > > > /* For now disable GPMU side throttling */ > > > > config |= AGC_LM_CONFIG_THROTTLE_DISABLE; > > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > > > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > > > index f1bb20574018..a9ba547a120c 100644 > > > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > > > @@ -790,10 +790,15 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu > > > > *gmu, unsigned int state) > > > > gmu_write(gmu, REG_A6XX_GMU_AHB_FENCE_RANGE_0, > > > > (1 << 31) | (0xa << 18) | (0xa0)); > > > > - chipid = adreno_gpu->rev.core << 24; > > > > - chipid |= adreno_gpu->rev.major << 16; > > > > - chipid |= adreno_gpu->rev.minor << 12; > > > > - chipid |= adreno_gpu->rev.patchid << 8; > > > > + /* Note that the GMU has a slightly different layout for > > > > +* chip_id, for whatever reason, so a bit of massaging > > > > +* is needed.
Re: [Freedreno] [PATCH 05/12] drm/msm/adreno: Use quirk to identify cached-coherent support
On Thu, Jul 13, 2023 at 1:06 PM Akhil P Oommen wrote:
>
> On Thu, Jul 06, 2023 at 02:10:38PM -0700, Rob Clark wrote:
> >
> > From: Rob Clark
> >
> > It is better to explicitly list it. With the move to opaque chip-id's
> > for future devices, we should avoid trying to infer things like
> > generation from the numerical value.
> >
> > Signed-off-by: Rob Clark
> > ---
> > drivers/gpu/drm/msm/adreno/adreno_device.c | 23 +++---
> > drivers/gpu/drm/msm/adreno/adreno_gpu.h| 1 +
> > 2 files changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index f469f951a907..3c531da417b9 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -256,6 +256,7 @@ static const struct adreno_info gpulist[] = {
> > },
> > .gmem = SZ_512K,
> > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > .init = a6xx_gpu_init,
> > }, {
> > .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > @@ -266,6 +267,7 @@ static const struct adreno_info gpulist[] = {
> > },
> > .gmem = SZ_512K,
> > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > .init = a6xx_gpu_init,
> > .zapfw = "a615_zap.mdt",
> > .hwcg = a615_hwcg,
> > @@ -278,6 +280,7 @@ static const struct adreno_info gpulist[] = {
> > },
> > .gmem = SZ_1M,
> > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > .init = a6xx_gpu_init,
> > .zapfw = "a630_zap.mdt",
> > .hwcg = a630_hwcg,
> > @@ -290,6 +293,7 @@ static const struct adreno_info gpulist[] = {
> > },
> > .gmem = SZ_1M,
> > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > .init = a6xx_gpu_init,
> > .zapfw = "a640_zap.mdt",
> > .hwcg = a640_hwcg,
> > @@ -302,7 +306,8 @@ static const struct adreno_info gpulist[] = {
> > },
> > .gmem = SZ_1M + SZ_128K,
> > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > - .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
> > + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
> > + ADRENO_QUIRK_HAS_HW_APRIV,
> > .init = a6xx_gpu_init,
> > .zapfw = "a650_zap.mdt",
> > .hwcg = a650_hwcg,
> > @@ -316,7 +321,8 @@ static const struct adreno_info gpulist[] = {
> > },
> > .gmem = SZ_1M + SZ_512K,
> > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > - .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
> > + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
> > + ADRENO_QUIRK_HAS_HW_APRIV,
> > .init = a6xx_gpu_init,
> > .zapfw = "a660_zap.mdt",
> > .hwcg = a660_hwcg,
> > @@ -329,7 +335,8 @@ static const struct adreno_info gpulist[] = {
> > },
> > .gmem = SZ_512K,
> > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > - .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
> > + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
> > + ADRENO_QUIRK_HAS_HW_APRIV,
> > .init = a6xx_gpu_init,
> > .hwcg = a660_hwcg,
> > .address_space_size = SZ_16G,
> > @@ -342,6 +349,7 @@ static const struct adreno_info gpulist[] = {
> > },
> > .gmem = SZ_2M,
> > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > .init = a6xx_gpu_init,
> > .zapfw = "a640_zap.mdt",
> > .hwcg = a640_hwcg,
> > @@ -353,7 +361,8 @@ static const struct adreno_info gpulist[] = {
> > },
> > .gmem = SZ_4M,
> > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > - .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
> > + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
> > + ADRENO_QUIRK_HAS_HW_APRIV,
> > .init = a6xx_gpu_init,
> > .zapfw = "a690_zap.mdt",
> > .hwcg = a690_hwcg,
> > @@ -565,9 +574,9 @@ static int adreno_bind(struct device *dev, struct
> > device *master, void *data)
> > if (ret)
> > return ret;
> >
> > - if (config.rev.core >= 6)
> > - if (!adreno_has_gmu_wrapper(to_adreno_gpu(gpu)))
> > - priv->has_cached_coherent = true;
> > + priv->has_cached_coherent =
> > + !!(info->quirks &
Re: [Freedreno] [PATCH 06/12] drm/msm/adreno: Allow SoC specific gpu device table entries
On Fri, Jul 07, 2023 at 02:40:47AM +0200, Konrad Dybcio wrote:
>
> On 6.07.2023 23:10, Rob Clark wrote:
> > From: Rob Clark
> >
> > There are cases where there are differences due to SoC integration.
> > Such as cache-coherency support, and (in the next patch) e-fuse to
> > speedbin mappings.
> >
> > Signed-off-by: Rob Clark
> > ---
> of_machine_is_compatible is rather used in extremely desperate
> situations :/ I'm not sure this is the correct way to do this..
>
> Especially since there's a direct correlation between GMU presence
> and ability to do cached coherent.
>
> The GMU mandates presence of RPMh (as most of what the GMU does is
> talk to AOSS through its RSC).
>
> To achieve I/O coherency, there must be some memory that both the
> CPU and GPU (and possibly others) can access through some sort of
> a negotiator/manager.
>
> In our case, I believe that's LLC. And guess what that implies.
> MEMNOC instead of BIMC. And guess what that implies. RPMh!
>
> Now, we know GMU => RPMh, but does it work the other way around?
I don't think we should tie gpu io-coherency with rpmh or llc. These
features are more dependent on SoC architecture than GPU arch.
-Akhil
>
> Yes. GMU wrapper was a hack because probably nobody in the Adreno team
> would have imagined that somebody would be crazy enough to fork
> multiple year old designs multiple times and release them as new
> SoCs with updated arm cores and 5G..
>
> (Except for A612 which has a "Reduced GMU" but that zombie still talks
> to RPMh. And A612 is IO-coherent. So I guess it works anyway.)
>
> Konrad
>
> > drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++---
> > drivers/gpu/drm/msm/adreno/adreno_gpu.h| 1 +
> > 2 files changed, 31 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index 3c531da417b9..e62bc895a31f 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > .init = a6xx_gpu_init,
> > + }, {
> > + .machine = "qcom,sm4350",
> > + .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > + .revn = 619,
> > + .fw = {
> > + [ADRENO_FW_SQE] = "a630_sqe.fw",
> > + [ADRENO_FW_GMU] = "a619_gmu.bin",
> > + },
> > + .gmem = SZ_512K,
> > + .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > + .init = a6xx_gpu_init,
> > + .zapfw = "a615_zap.mdt",
> > + .hwcg = a615_hwcg,
> > + }, {
> > + .machine = "qcom,sm6375",
> > + .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > + .revn = 619,
> > + .fw = {
> > + [ADRENO_FW_SQE] = "a630_sqe.fw",
> > + [ADRENO_FW_GMU] = "a619_gmu.bin",
> > + },
> > + .gmem = SZ_512K,
> > + .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > + .init = a6xx_gpu_init,
> > + .zapfw = "a615_zap.mdt",
> > + .hwcg = a615_hwcg,
> > }, {
> > .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > .revn = 619,
> > @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev
> > rev)
> > /* identify gpu: */
> > for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > const struct adreno_info *info = [i];
> > + if (info->machine && !of_machine_is_compatible(info->machine))
> > + continue;
> > if (adreno_cmp_rev(info->rev, rev))
> > return info;
> > }
> > @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct
> > device *master, void *data)
> > config.rev.minor, config.rev.patchid);
> >
> > priv->is_a2xx = config.rev.core == 2;
> > + priv->has_cached_coherent =
> > + !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> >
> > gpu = info->init(drm);
> > if (IS_ERR(gpu)) {
> > @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct
> > device *master, void *data)
> > if (ret)
> > return ret;
> >
> > - priv->has_cached_coherent =
> > - !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > - !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > -
> > return 0;
> > }
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > index e08d41337169..d5335b99c64c 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[],
> > a615_hwcg[], a630_hwcg[], a640_h
> > extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> >
> > struct
Re: [Freedreno] [PATCH 12/12] drm/msm/adreno: Switch to chip-id for identifying GPU
On Thu, Jul 13, 2023 at 2:39 PM Akhil P Oommen wrote: > > On Fri, Jul 07, 2023 at 06:45:42AM +0300, Dmitry Baryshkov wrote: > > > > On 07/07/2023 00:10, Rob Clark wrote: > > > From: Rob Clark > > > > > > Since the revision becomes an opaque identifier with future GPUs, move > > > away from treating different ranges of bits as having a given meaning. > > > This means that we need to explicitly list different patch revisions in > > > the device table. > > > > > > Signed-off-by: Rob Clark > > > --- > > > drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 2 +- > > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +- > > > drivers/gpu/drm/msm/adreno/a5xx_power.c| 2 +- > > > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 13 ++- > > > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 9 +- > > > drivers/gpu/drm/msm/adreno/adreno_device.c | 128 ++--- > > > drivers/gpu/drm/msm/adreno/adreno_gpu.c| 16 +-- > > > drivers/gpu/drm/msm/adreno/adreno_gpu.h| 51 > > > 8 files changed, 122 insertions(+), 110 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > > > b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > > > index 715436cb3996..8b4cdf95f445 100644 > > > --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > > > +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > > > @@ -145,7 +145,7 @@ static void a4xx_enable_hwcg(struct msm_gpu *gpu) > > > gpu_write(gpu, REG_A4XX_RBBM_CLOCK_DELAY_HLSQ, 0x0022); > > > /* Early A430's have a timing issue with SP/TP power collapse; > > >disabling HW clock gating prevents it. */ > > > - if (adreno_is_a430(adreno_gpu) && adreno_gpu->rev.patchid < 2) > > > + if (adreno_is_a430(adreno_gpu) && adreno_patchid(adreno_gpu) < 2) > > > gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0); > > > else > > > gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0x); > > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > > b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > > index f0803e94ebe5..70d2b5342cd9 100644 > > > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > > @@ -1744,6 +1744,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device > > > *dev) > > > struct msm_drm_private *priv = dev->dev_private; > > > struct platform_device *pdev = priv->gpu_pdev; > > > struct adreno_platform_config *config = pdev->dev.platform_data; > > > + const struct adreno_info *info; > > > struct a5xx_gpu *a5xx_gpu = NULL; > > > struct adreno_gpu *adreno_gpu; > > > struct msm_gpu *gpu; > > > @@ -1770,7 +1771,15 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device > > > *dev) > > > nr_rings = 4; > > > - if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev)) > > > + /* > > > +* Note that we wouldn't have been able to get this far if there is > > > not > > > +* a device table entry for this chip_id > > > +*/ > > > + info = adreno_find_info(config->chip_id); > > > + if (WARN_ON(!info)) > > > + return ERR_PTR(-EINVAL); > > > + > > > + if (info->revn == 510) > > > nr_rings = 1; > > > ret = adreno_gpu_init(dev, pdev, adreno_gpu, , nr_rings); > > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c > > > b/drivers/gpu/drm/msm/adreno/a5xx_power.c > > > index 0e63a1429189..7705f8010484 100644 > > > --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c > > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c > > > @@ -179,7 +179,7 @@ static void a540_lm_setup(struct msm_gpu *gpu) > > > /* The battery current limiter isn't enabled for A540 */ > > > config = AGC_LM_CONFIG_BCL_DISABLED; > > > - config |= adreno_gpu->rev.patchid << AGC_LM_CONFIG_GPU_VERSION_SHIFT; > > > + config |= adreno_patchid(adreno_gpu) << > > > AGC_LM_CONFIG_GPU_VERSION_SHIFT; > > > /* For now disable GPMU side throttling */ > > > config |= AGC_LM_CONFIG_THROTTLE_DISABLE; > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > > index f1bb20574018..a9ba547a120c 100644 > > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > > @@ -790,10 +790,15 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu, > > > unsigned int state) > > > gmu_write(gmu, REG_A6XX_GMU_AHB_FENCE_RANGE_0, > > > (1 << 31) | (0xa << 18) | (0xa0)); > > > - chipid = adreno_gpu->rev.core << 24; > > > - chipid |= adreno_gpu->rev.major << 16; > > > - chipid |= adreno_gpu->rev.minor << 12; > > > - chipid |= adreno_gpu->rev.patchid << 8; > > > + /* Note that the GMU has a slightly different layout for > > > +* chip_id, for whatever reason, so a bit of massaging > > > +* is needed. The upper 16b are the same, but minor and > > > +* patchid are packed in four bits each with the lower > > > +* 8b unused: > > > +*/ > > > + chipid = adreno_gpu->chip_id & 0x; > > > + chipid |= (adreno_gpu->chip_id << 4) &
Re: [Freedreno] [PATCH 12/12] drm/msm/adreno: Switch to chip-id for identifying GPU
On Fri, Jul 07, 2023 at 06:45:42AM +0300, Dmitry Baryshkov wrote: > > On 07/07/2023 00:10, Rob Clark wrote: > > From: Rob Clark > > > > Since the revision becomes an opaque identifier with future GPUs, move > > away from treating different ranges of bits as having a given meaning. > > This means that we need to explicitly list different patch revisions in > > the device table. > > > > Signed-off-by: Rob Clark > > --- > > drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 2 +- > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +- > > drivers/gpu/drm/msm/adreno/a5xx_power.c| 2 +- > > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 13 ++- > > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 9 +- > > drivers/gpu/drm/msm/adreno/adreno_device.c | 128 ++--- > > drivers/gpu/drm/msm/adreno/adreno_gpu.c| 16 +-- > > drivers/gpu/drm/msm/adreno/adreno_gpu.h| 51 > > 8 files changed, 122 insertions(+), 110 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > > b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > > index 715436cb3996..8b4cdf95f445 100644 > > --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > > +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c > > @@ -145,7 +145,7 @@ static void a4xx_enable_hwcg(struct msm_gpu *gpu) > > gpu_write(gpu, REG_A4XX_RBBM_CLOCK_DELAY_HLSQ, 0x0022); > > /* Early A430's have a timing issue with SP/TP power collapse; > >disabling HW clock gating prevents it. */ > > - if (adreno_is_a430(adreno_gpu) && adreno_gpu->rev.patchid < 2) > > + if (adreno_is_a430(adreno_gpu) && adreno_patchid(adreno_gpu) < 2) > > gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0); > > else > > gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0x); > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > index f0803e94ebe5..70d2b5342cd9 100644 > > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > > @@ -1744,6 +1744,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) > > struct msm_drm_private *priv = dev->dev_private; > > struct platform_device *pdev = priv->gpu_pdev; > > struct adreno_platform_config *config = pdev->dev.platform_data; > > + const struct adreno_info *info; > > struct a5xx_gpu *a5xx_gpu = NULL; > > struct adreno_gpu *adreno_gpu; > > struct msm_gpu *gpu; > > @@ -1770,7 +1771,15 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev) > > nr_rings = 4; > > - if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev)) > > + /* > > +* Note that we wouldn't have been able to get this far if there is not > > +* a device table entry for this chip_id > > +*/ > > + info = adreno_find_info(config->chip_id); > > + if (WARN_ON(!info)) > > + return ERR_PTR(-EINVAL); > > + > > + if (info->revn == 510) > > nr_rings = 1; > > ret = adreno_gpu_init(dev, pdev, adreno_gpu, , nr_rings); > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c > > b/drivers/gpu/drm/msm/adreno/a5xx_power.c > > index 0e63a1429189..7705f8010484 100644 > > --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c > > @@ -179,7 +179,7 @@ static void a540_lm_setup(struct msm_gpu *gpu) > > /* The battery current limiter isn't enabled for A540 */ > > config = AGC_LM_CONFIG_BCL_DISABLED; > > - config |= adreno_gpu->rev.patchid << AGC_LM_CONFIG_GPU_VERSION_SHIFT; > > + config |= adreno_patchid(adreno_gpu) << AGC_LM_CONFIG_GPU_VERSION_SHIFT; > > /* For now disable GPMU side throttling */ > > config |= AGC_LM_CONFIG_THROTTLE_DISABLE; > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > index f1bb20574018..a9ba547a120c 100644 > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > @@ -790,10 +790,15 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu, > > unsigned int state) > > gmu_write(gmu, REG_A6XX_GMU_AHB_FENCE_RANGE_0, > > (1 << 31) | (0xa << 18) | (0xa0)); > > - chipid = adreno_gpu->rev.core << 24; > > - chipid |= adreno_gpu->rev.major << 16; > > - chipid |= adreno_gpu->rev.minor << 12; > > - chipid |= adreno_gpu->rev.patchid << 8; > > + /* Note that the GMU has a slightly different layout for > > +* chip_id, for whatever reason, so a bit of massaging > > +* is needed. The upper 16b are the same, but minor and > > +* patchid are packed in four bits each with the lower > > +* 8b unused: > > +*/ > > + chipid = adreno_gpu->chip_id & 0x; > > + chipid |= (adreno_gpu->chip_id << 4) & 0xf000; /* minor */ > > + chipid |= (adreno_gpu->chip_id << 8) & 0x0f00; /* patchid */ > > I'd beg for explicit FIELD_GET and FIELD_PREP here. > > > gmu_write(gmu, REG_A6XX_GMU_HFI_SFR_ADDR, chipid); > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >
Re: [Freedreno] [PATCH] drm/msm: Fix hw_fence error path cleanup
On Thu, Jul 13, 2023 at 1:03 PM Dmitry Baryshkov
wrote:
>
> On 13/07/2023 01:25, Rob Clark wrote:
> > From: Rob Clark
> >
> > In an error path where the submit is free'd without the job being run,
> > the hw_fence pointer is simply a kzalloc'd block of memory. In this
> > case we should just kfree() it, rather than trying to decrement it's
> > reference count. Fortunately we can tell that this is the case by
> > checking for a zero refcount, since if the job was run, the submit would
> > be holding a reference to the hw_fence.
> >
> > Fixes: f94e6a51e17c ("drm/msm: Pre-allocate hw_fence")
> > Signed-off-by: Rob Clark
> > ---
> > drivers/gpu/drm/msm/msm_fence.c | 6 ++
> > drivers/gpu/drm/msm/msm_gem_submit.c | 14 +-
> > 2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_fence.c
> > b/drivers/gpu/drm/msm/msm_fence.c
> > index 96599ec3eb78..1a5d4f1c8b42 100644
> > --- a/drivers/gpu/drm/msm/msm_fence.c
> > +++ b/drivers/gpu/drm/msm/msm_fence.c
> > @@ -191,6 +191,12 @@ msm_fence_init(struct dma_fence *fence, struct
> > msm_fence_context *fctx)
> >
> > f->fctx = fctx;
> >
> > + /*
> > + * Until this point, the fence was just some pre-allocated memory,
> > + * no-one should have taken a reference to it yet.
> > + */
> > + WARN_ON(kref_read(>refcount));
>
> It this really correct to return a refcounted object with 0 refcount
> (I'm looking at submit_create() / msm_fence_alloc() )? Maybe it would be
> better to move dma_fence_get() to msm_fence_alloc() ? But don't
> immediately see, which one should be moved.
The issue is that we can't really initialize the fence until
msm_job_run(), when it is known what order the fence would be
signaled. But we don't want to do any allocations in msm_job_run()
because that could trigger the shrinker, which could need to wait
until jobs complete to release memory, forming a deadlock.
BR,
-R
> > +
> > dma_fence_init(>base, _fence_ops, >spinlock,
> > fctx->context, ++fctx->last_fence);
> > }
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> > b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index 3f1aa4de3b87..9d66498cdc04 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -86,7 +86,19 @@ void __msm_gem_submit_destroy(struct kref *kref)
> > }
> >
> > dma_fence_put(submit->user_fence);
> > - dma_fence_put(submit->hw_fence);
> > +
> > + /*
> > + * If the submit is freed before msm_job_run(), then hw_fence is
> > + * just some pre-allocated memory, not a reference counted fence.
> > + * Once the job runs and the hw_fence is initialized, it will
> > + * have a refcount of at least one, since the submit holds a ref
> > + * to the hw_fence.
> > + */
> > + if (kref_read(>hw_fence->refcount) == 0) {
> > + kfree(submit->hw_fence);
> > + } else {
> > + dma_fence_put(submit->hw_fence);
> > + }
> >
> > put_pid(submit->pid);
> > msm_submitqueue_put(submit->queue);
>
> --
> With best wishes
> Dmitry
>
Re: [Freedreno] [PATCH 06/12] drm/msm/adreno: Allow SoC specific gpu device table entries
On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
>
> On 07/07/2023 00:10, Rob Clark wrote:
> > From: Rob Clark
> >
> > There are cases where there are differences due to SoC integration.
> > Such as cache-coherency support, and (in the next patch) e-fuse to
> > speedbin mappings.
>
> I have the feeling that we are trying to circumvent the way DT works. I'd
> suggest adding explicit SoC-compatible strings to Adreno bindings and then
> using of_device_id::data and then of_device_get_match_data().
>
Just thinking, then how about a unique compatible string which we match
to identify gpu->info and drop chip-id check completely here?
-Akhil
> >
> > Signed-off-by: Rob Clark
> > ---
> > drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++---
> > drivers/gpu/drm/msm/adreno/adreno_gpu.h| 1 +
> > 2 files changed, 31 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index 3c531da417b9..e62bc895a31f 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > .init = a6xx_gpu_init,
> > + }, {
> > + .machine = "qcom,sm4350",
> > + .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > + .revn = 619,
> > + .fw = {
> > + [ADRENO_FW_SQE] = "a630_sqe.fw",
> > + [ADRENO_FW_GMU] = "a619_gmu.bin",
> > + },
> > + .gmem = SZ_512K,
> > + .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > + .init = a6xx_gpu_init,
> > + .zapfw = "a615_zap.mdt",
> > + .hwcg = a615_hwcg,
> > + }, {
> > + .machine = "qcom,sm6375",
> > + .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > + .revn = 619,
> > + .fw = {
> > + [ADRENO_FW_SQE] = "a630_sqe.fw",
> > + [ADRENO_FW_GMU] = "a619_gmu.bin",
> > + },
> > + .gmem = SZ_512K,
> > + .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > + .init = a6xx_gpu_init,
> > + .zapfw = "a615_zap.mdt",
> > + .hwcg = a615_hwcg,
> > }, {
> > .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > .revn = 619,
> > @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev
> > rev)
> > /* identify gpu: */
> > for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > const struct adreno_info *info = [i];
> > + if (info->machine && !of_machine_is_compatible(info->machine))
> > + continue;
> > if (adreno_cmp_rev(info->rev, rev))
> > return info;
> > }
> > @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct
> > device *master, void *data)
> > config.rev.minor, config.rev.patchid);
> > priv->is_a2xx = config.rev.core == 2;
> > + priv->has_cached_coherent =
> > + !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > gpu = info->init(drm);
> > if (IS_ERR(gpu)) {
> > @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct
> > device *master, void *data)
> > if (ret)
> > return ret;
> > - priv->has_cached_coherent =
> > - !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > - !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > -
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > index e08d41337169..d5335b99c64c 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[],
> > a615_hwcg[], a630_hwcg[], a640_h
> > extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > struct adreno_info {
> > + const char *machine;
> > struct adreno_rev rev;
> > uint32_t revn;
> > const char *fw[ADRENO_FW_MAX];
>
> --
> With best wishes
> Dmitry
>
Re: [Freedreno] [PATCH 05/12] drm/msm/adreno: Use quirk to identify cached-coherent support
On Thu, Jul 06, 2023 at 02:10:38PM -0700, Rob Clark wrote:
>
> From: Rob Clark
>
> It is better to explicitly list it. With the move to opaque chip-id's
> for future devices, we should avoid trying to infer things like
> generation from the numerical value.
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/adreno/adreno_device.c | 23 +++---
> drivers/gpu/drm/msm/adreno/adreno_gpu.h| 1 +
> 2 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index f469f951a907..3c531da417b9 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -256,6 +256,7 @@ static const struct adreno_info gpulist[] = {
> },
> .gmem = SZ_512K,
> .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> .init = a6xx_gpu_init,
> }, {
> .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> @@ -266,6 +267,7 @@ static const struct adreno_info gpulist[] = {
> },
> .gmem = SZ_512K,
> .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> .init = a6xx_gpu_init,
> .zapfw = "a615_zap.mdt",
> .hwcg = a615_hwcg,
> @@ -278,6 +280,7 @@ static const struct adreno_info gpulist[] = {
> },
> .gmem = SZ_1M,
> .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> .init = a6xx_gpu_init,
> .zapfw = "a630_zap.mdt",
> .hwcg = a630_hwcg,
> @@ -290,6 +293,7 @@ static const struct adreno_info gpulist[] = {
> },
> .gmem = SZ_1M,
> .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> .init = a6xx_gpu_init,
> .zapfw = "a640_zap.mdt",
> .hwcg = a640_hwcg,
> @@ -302,7 +306,8 @@ static const struct adreno_info gpulist[] = {
> },
> .gmem = SZ_1M + SZ_128K,
> .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> - .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
> + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
> + ADRENO_QUIRK_HAS_HW_APRIV,
> .init = a6xx_gpu_init,
> .zapfw = "a650_zap.mdt",
> .hwcg = a650_hwcg,
> @@ -316,7 +321,8 @@ static const struct adreno_info gpulist[] = {
> },
> .gmem = SZ_1M + SZ_512K,
> .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> - .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
> + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
> + ADRENO_QUIRK_HAS_HW_APRIV,
> .init = a6xx_gpu_init,
> .zapfw = "a660_zap.mdt",
> .hwcg = a660_hwcg,
> @@ -329,7 +335,8 @@ static const struct adreno_info gpulist[] = {
> },
> .gmem = SZ_512K,
> .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> - .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
> + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
> + ADRENO_QUIRK_HAS_HW_APRIV,
> .init = a6xx_gpu_init,
> .hwcg = a660_hwcg,
> .address_space_size = SZ_16G,
> @@ -342,6 +349,7 @@ static const struct adreno_info gpulist[] = {
> },
> .gmem = SZ_2M,
> .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> .init = a6xx_gpu_init,
> .zapfw = "a640_zap.mdt",
> .hwcg = a640_hwcg,
> @@ -353,7 +361,8 @@ static const struct adreno_info gpulist[] = {
> },
> .gmem = SZ_4M,
> .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> - .quirks = ADRENO_QUIRK_HAS_HW_APRIV,
> + .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT |
> + ADRENO_QUIRK_HAS_HW_APRIV,
> .init = a6xx_gpu_init,
> .zapfw = "a690_zap.mdt",
> .hwcg = a690_hwcg,
> @@ -565,9 +574,9 @@ static int adreno_bind(struct device *dev, struct device
> *master, void *data)
> if (ret)
> return ret;
>
> - if (config.rev.core >= 6)
> - if (!adreno_has_gmu_wrapper(to_adreno_gpu(gpu)))
> - priv->has_cached_coherent = true;
> + priv->has_cached_coherent =
> + !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> + !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index a7c4a2c536e3..e08d41337169 100644
>
Re: [Freedreno] [PATCH] drm/msm: Fix hw_fence error path cleanup
On 13/07/2023 01:25, Rob Clark wrote:
From: Rob Clark
In an error path where the submit is free'd without the job being run,
the hw_fence pointer is simply a kzalloc'd block of memory. In this
case we should just kfree() it, rather than trying to decrement it's
reference count. Fortunately we can tell that this is the case by
checking for a zero refcount, since if the job was run, the submit would
be holding a reference to the hw_fence.
Fixes: f94e6a51e17c ("drm/msm: Pre-allocate hw_fence")
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_fence.c | 6 ++
drivers/gpu/drm/msm/msm_gem_submit.c | 14 +-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
index 96599ec3eb78..1a5d4f1c8b42 100644
--- a/drivers/gpu/drm/msm/msm_fence.c
+++ b/drivers/gpu/drm/msm/msm_fence.c
@@ -191,6 +191,12 @@ msm_fence_init(struct dma_fence *fence, struct
msm_fence_context *fctx)
f->fctx = fctx;
+ /*
+* Until this point, the fence was just some pre-allocated memory,
+* no-one should have taken a reference to it yet.
+*/
+ WARN_ON(kref_read(>refcount));
It this really correct to return a refcounted object with 0 refcount
(I'm looking at submit_create() / msm_fence_alloc() )? Maybe it would be
better to move dma_fence_get() to msm_fence_alloc() ? But don't
immediately see, which one should be moved.
+
dma_fence_init(>base, _fence_ops, >spinlock,
fctx->context, ++fctx->last_fence);
}
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 3f1aa4de3b87..9d66498cdc04 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -86,7 +86,19 @@ void __msm_gem_submit_destroy(struct kref *kref)
}
dma_fence_put(submit->user_fence);
- dma_fence_put(submit->hw_fence);
+
+ /*
+* If the submit is freed before msm_job_run(), then hw_fence is
+* just some pre-allocated memory, not a reference counted fence.
+* Once the job runs and the hw_fence is initialized, it will
+* have a refcount of at least one, since the submit holds a ref
+* to the hw_fence.
+*/
+ if (kref_read(>hw_fence->refcount) == 0) {
+ kfree(submit->hw_fence);
+ } else {
+ dma_fence_put(submit->hw_fence);
+ }
put_pid(submit->pid);
msm_submitqueue_put(submit->queue);
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH 2/2] drm/msm/a690: Remove revn and name
On 04/07/2023 19:36, Rob Clark wrote: From: Rob Clark These fields are deprecated. But any userspace new enough to support a690 also knows how to identify the GPU based on chip-id. Signed-off-by: Rob Clark --- drivers/gpu/drm/msm/adreno/adreno_device.c | 2 -- drivers/gpu/drm/msm/adreno/adreno_gpu.h| 3 ++- 2 files changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [Freedreno] [PATCH 1/2] drm/msm/adreno: Fix warn splat for devices without revn
On 05/07/2023 17:42, Rob Clark wrote:
On Tue, Jul 4, 2023 at 10:20 AM Dmitry Baryshkov
wrote:
On Tue, 4 Jul 2023 at 19:36, Rob Clark wrote:
From: Rob Clark
Recently, a WARN_ON() was introduced to ensure that revn is filled before
adreno_is_aXYZ is called. This however doesn't work very well when revn is
0 by design (such as for A635).
Cc: Konrad Dybcio
Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before being
set")
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/adreno/adreno_gpu.h | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 65379e4824d9..ef1bcb6b624e 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -149,7 +149,8 @@ bool adreno_cmp_rev(struct adreno_rev rev1, struct
adreno_rev rev2);
static inline bool adreno_is_revn(const struct adreno_gpu *gpu, uint32_t revn)
{
- WARN_ON_ONCE(!gpu->revn);
+ /* revn can be zero, but if not is set at same time as info */
+ WARN_ON_ONCE(!gpu->info);
return gpu->revn == revn;
}
@@ -161,14 +162,16 @@ static inline bool adreno_has_gmu_wrapper(const struct
adreno_gpu *gpu)
static inline bool adreno_is_a2xx(const struct adreno_gpu *gpu)
{
- WARN_ON_ONCE(!gpu->revn);
+ /* revn can be zero, but if not is set at same time as info */
+ WARN_ON_ONCE(!gpu->info);
return (gpu->revn < 300);
This is then incorrect for a635 / a690 if they have revn at 0.
Fortunately not any more broken that it has ever been. But as long as
sc7280/sc8280 have GPU OPP tables, you'd never hit this. I'm working
on a better solution for next merge window.
Sounds fine with me then.
Reviewed-by: Dmitry Baryshkov
BR,
-R
}
static inline bool adreno_is_a20x(const struct adreno_gpu *gpu)
{
- WARN_ON_ONCE(!gpu->revn);
+ /* revn can be zero, but if not is set at same time as info */
+ WARN_ON_ONCE(!gpu->info);
return (gpu->revn < 210);
And this too.
}
--
2.41.0
--
With best wishes
Dmitry
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] drm/msm/adreno: Fix snapshot BINDLESS_DATA size
On 11/07/2023 20:54, Rob Clark wrote:
From: Rob Clark
The incorrect size was causing "CP | AHB bus error" when snapshotting
the GPU state on a6xx gen4 (a660 family).
Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/26
Signed-off-by: Rob Clark
What about:
Fixes: 1707add81551 ("drm/msm/a6xx: Add a6xx gpu state")
?
---
drivers/gpu/drm/msm/adreno/a6xx_gpu_state.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.h
b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.h
index 790f55e24533..e788ed72eb0d 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.h
@@ -206,7 +206,7 @@ static const struct a6xx_shader_block {
SHADER(A6XX_SP_LB_3_DATA, 0x800),
SHADER(A6XX_SP_LB_4_DATA, 0x800),
SHADER(A6XX_SP_LB_5_DATA, 0x200),
- SHADER(A6XX_SP_CB_BINDLESS_DATA, 0x2000),
+ SHADER(A6XX_SP_CB_BINDLESS_DATA, 0x800),
SHADER(A6XX_SP_CB_LEGACY_DATA, 0x280),
SHADER(A6XX_SP_UAV_DATA, 0x80),
SHADER(A6XX_SP_INST_TAG, 0x80),
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH 02/12] drm/msm/adreno: Remove redundant gmem size param
On Fri, Jul 07, 2023 at 01:22:56AM +0200, Konrad Dybcio wrote:
>
> On 6.07.2023 23:10, Rob Clark wrote:
> > From: Rob Clark
> >
> > Even in the ocmem case, the allocated ocmem buffer size should match the
> > requested size.
> >
> > Signed-off-by: Rob Clark
> > ---
> [...]
>
> > +
> > + WARN_ON(ocmem_hdl->len != adreno_gpu->info->gmem);
> I believe this should be an error condition. If the sizes are mismatched,
> best case scenario you get suboptimal perf and worst case scenario your
> system explodes.
No, the worst case scenarios are subtle bugs like random corruptions,
pagefaults etc which you debug for months. ;)
-Akhil.
>
> Very nice cleanup though!
>
> Konrad
> >
> > return 0;
> > }
> > @@ -1097,7 +1098,6 @@ int adreno_gpu_init(struct drm_device *drm, struct
> > platform_device *pdev,
> >
> > adreno_gpu->funcs = funcs;
> > adreno_gpu->info = adreno_info(config->rev);
> > - adreno_gpu->gmem = adreno_gpu->info->gmem;
> > adreno_gpu->revn = adreno_gpu->info->revn;
> > adreno_gpu->rev = *rev;
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > index 6830c3776c2d..aaf09c642dc6 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -77,7 +77,6 @@ struct adreno_gpu {
> > struct msm_gpu base;
> > struct adreno_rev rev;
> > const struct adreno_info *info;
> > - uint32_t gmem; /* actual gmem size */
> > uint32_t revn; /* numeric revision name */
> > uint16_t speedbin;
> > const struct adreno_gpu_funcs *funcs;
Re: [Freedreno] [PATCH v2 2/3] drm/msm: Fix IS_ERR() vs NULL check in a5xx_submit_in_rb()
On Thu, 13 Jul 2023 at 22:08, Akhil P Oommen wrote:
>
> On Thu, Jul 13, 2023 at 10:05:55AM +0800, Gaosheng Cui wrote:
> >
> > The msm_gem_get_vaddr() returns an ERR_PTR() on failure, we should
> > use IS_ERR() to check the return value.
> >
> > Fixes: 6a8bd08d0465 ("drm/msm: add sudo flag to submit ioctl")
> > Signed-off-by: Gaosheng Cui
> > Reviewed-by: Abhinav Kumar
> > ---
> > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > index a99310b68793..a499e3b350fc 100644
> > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > @@ -89,7 +89,7 @@ static void a5xx_submit_in_rb(struct msm_gpu *gpu, struct
> > msm_gem_submit *submit
> >* since we've already mapped it once in
> >* submit_reloc()
> >*/
> > - if (WARN_ON(!ptr))
> > + if (WARN_ON(IS_ERR(ptr)))
> nit: can we make this IS_ERR_OR_NULL() check to retain the current
> validation? A null is catastrophic here. Yeah, I see that the current
> implementation of ...get_vaddr() doesn't return a NULL.
I generally dislike IS_ERR_OR_NULL, as it is always (incorrectly)
paired with PTR_ERR. But in the case of void return it would be a
perfect fit.
>
> Reviewed-by: Akhil P Oommen
>
> -Akhil
>
> > return;
> >
> > for (i = 0; i < dwords; i++) {
> > --
> > 2.25.1
> >
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] drm/msm/a6xx: Fix misleading comment
On Fri, Jun 30, 2023 at 09:20:43AM -0700, Rob Clark wrote:
>
> From: Rob Clark
>
> The range is actually len+1.
>
> Signed-off-by: Rob Clark
Reviewed-by: Akhil P Oommen
-Akhil
> ---
> drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> index eea2e60ce3b7..edf76a4b16bd 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> @@ -39,8 +39,8 @@ struct a6xx_gpu {
>
> /*
> * Given a register and a count, return a value to program into
> - * REG_CP_PROTECT_REG(n) - this will block both reads and writes for _len
> - * registers starting at _reg.
> + * REG_CP_PROTECT_REG(n) - this will block both reads and writes for
> + * _len + 1 registers starting at _reg.
> */
> #define A6XX_PROTECT_NORDWR(_reg, _len) \
> ((1 << 31) | \
> --
> 2.41.0
>
Re: [Freedreno] [PATCH v2 2/3] drm/msm: Fix IS_ERR() vs NULL check in a5xx_submit_in_rb()
On Thu, Jul 13, 2023 at 10:05:55AM +0800, Gaosheng Cui wrote:
>
> The msm_gem_get_vaddr() returns an ERR_PTR() on failure, we should
> use IS_ERR() to check the return value.
>
> Fixes: 6a8bd08d0465 ("drm/msm: add sudo flag to submit ioctl")
> Signed-off-by: Gaosheng Cui
> Reviewed-by: Abhinav Kumar
> ---
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index a99310b68793..a499e3b350fc 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -89,7 +89,7 @@ static void a5xx_submit_in_rb(struct msm_gpu *gpu, struct
> msm_gem_submit *submit
>* since we've already mapped it once in
>* submit_reloc()
>*/
> - if (WARN_ON(!ptr))
> + if (WARN_ON(IS_ERR(ptr)))
nit: can we make this IS_ERR_OR_NULL() check to retain the current
validation? A null is catastrophic here. Yeah, I see that the current
implementation of ...get_vaddr() doesn't return a NULL.
Reviewed-by: Akhil P Oommen
-Akhil
> return;
>
> for (i = 0; i < dwords; i++) {
> --
> 2.25.1
>
Re: [Freedreno] [PATCH] drm/msm/adreno: Fix snapshot BINDLESS_DATA size
On Tue, Jul 11, 2023 at 10:54:07AM -0700, Rob Clark wrote:
>
> From: Rob Clark
>
> The incorrect size was causing "CP | AHB bus error" when snapshotting
> the GPU state on a6xx gen4 (a660 family).
>
> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/26
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/adreno/a6xx_gpu_state.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.h
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.h
> index 790f55e24533..e788ed72eb0d 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.h
> @@ -206,7 +206,7 @@ static const struct a6xx_shader_block {
> SHADER(A6XX_SP_LB_3_DATA, 0x800),
> SHADER(A6XX_SP_LB_4_DATA, 0x800),
> SHADER(A6XX_SP_LB_5_DATA, 0x200),
> - SHADER(A6XX_SP_CB_BINDLESS_DATA, 0x2000),
> + SHADER(A6XX_SP_CB_BINDLESS_DATA, 0x800),
> SHADER(A6XX_SP_CB_LEGACY_DATA, 0x280),
> SHADER(A6XX_SP_UAV_DATA, 0x80),
> SHADER(A6XX_SP_INST_TAG, 0x80),
> --
> 2.41.0
>
Reviewed-by: Akhil P Oommen
-Akhil
Re: [Freedreno] [PATCH 2/2] drm/bridge: lt9611: Do not generate HFP/HBP/HSA and EOT packet
On Thu, Jul 13, 2023 at 11:39 PM Abhinav Kumar wrote: > > > > On 7/12/2023 10:41 AM, Marek Vasut wrote: > > On 7/9/23 03:03, Abhinav Kumar wrote: > >> > >> > >> On 7/7/2023 1:47 AM, Neil Armstrong wrote: > >>> On 07/07/2023 09:18, Neil Armstrong wrote: > Hi, > > On 06/07/2023 11:20, Amit Pundir wrote: > > On Wed, 5 Jul 2023 at 11:09, Dmitry Baryshkov > > wrote: > >> > >> [Adding freedreno@ to cc list] > >> > >> On Wed, 5 Jul 2023 at 08:31, Jagan Teki > >> wrote: > >>> > >>> Hi Amit, > >>> > >>> On Wed, Jul 5, 2023 at 10:15 AM Amit Pundir > >>> wrote: > > Hi Marek, > > On Wed, 5 Jul 2023 at 01:48, Marek Vasut wrote: > > > > Do not generate the HS front and back porch gaps, the HSA gap and > > EOT packet, as these packets are not required. This makes the > > bridge > > work with Samsung DSIM on i.MX8MM and i.MX8MP. > > This patch broke display on Dragonboard 845c (SDM845) devboard > running > AOSP. This is what I see > https://people.linaro.org/~amit.pundir/db845c-userdebug/v6.5-broken-display/PXL_20230704_150156326.jpg. > Reverting this patch fixes this regression for me. > >>> > >>> Might be msm dsi host require proper handling on these updated > >>> mode_flags? did they? > >> > >> The msm DSI host supports those flags. Also, I'd like to point out > >> that the patch didn't change the rest of the driver code. So even if > >> drm/msm ignored some of the flags, it should not have caused the > >> issue. Most likely the issue is on the lt9611 side. I's suspect that > >> additional programming is required to make it work with these flags. > > > > I spent some time today on smoke testing these flags (individually and > > in limited combination) on DB845c, to narrow down this breakage to one > > or more flag(s) triggering it. Here are my observations in limited > > testing done so far. > > > > There is no regression with MIPI_DSI_MODE_NO_EOT_PACKET when enabled > > alone and system boots to UI as usual. > > > > MIPI_DSI_MODE_VIDEO_NO_HFP always trigger the broken display as in the > > screenshot[1] shared earlier as well. > > > > Adding either of MIPI_DSI_MODE_VIDEO_NO_HSA and > > MIPI_DSI_MODE_VIDEO_NO_HBP always result in no display, unless paired > > with MIPI_DSI_MODE_VIDEO_NO_HFP and in that case we get the broken > > display as reported. > > > > In short other than MIPI_DSI_MODE_NO_EOT_PACKET flag, all other flags > > added in this commit break the display on DB845c one way or another. > > I think the investigation would be to understand why samsung-dsim > requires > such flags and/or what are the difference in behavior between MSM > DSI and samsung DSIM > for those flags ? > > If someone has access to the lt9611 datasheet, so it requires > HSA/HFP/HBP to be > skipped ? and does MSM DSI and samsung DSIM skip them in the same way ? > >>> > >>> I think there's a mismatch, where on one side this flags sets the > >>> link in LP-11 while > >>> in HSA/HFP/HPB while on the other it completely removes those > >>> blanking packets. > >>> > >>> The name MIPI_DSI_MODE_VIDEO_NO_HBP suggests removal of HPB, not > >>> LP-11 while HPB. > >>> the registers used in both controllers are different: > >>> - samsung-dsim: DSIM_HBP_DISABLE_MODE > >>> - msm dsi: DSI_VID_CFG0_HBP_POWER_STOP > >>> > >>> The first one suggest removing the packet, while the second one > >>> suggests powering > >>> off the line while in the blanking packet period. > >>> > >>> @Abhinav, can you comment on that ? > >>> > >> > >> I dont get what it means by completely removes blanking packets in DSIM. > > > > MIPI_DSI_MODE_VIDEO_NO_HFP means the HBP period is just skipped by DSIM. > > > > Maybe there is a need for new set of flags which differentiate between > > HBP skipped (i.e. NO HBP) and HBP LP11 ? > > > > No, the section of the MIPI DSI spec I posted below clearly states there > are two options: > > 1) send blanking packets during those periods > 2) transition to LP11 during those periods > > There is no 3rd option in the spec of not doing both like what you are > suggesting. So DSIM should also be only transitioning to LP11 during > those periods if its not sending the blanking packets with those flags set. > > So, there is no need for any new set of flags to differentiate. > > The flags and their interpretation is correct in MSM driver. I cannot > comment on what exactly DSIM does with those flags. As many of them know all these flags are generic across controllers I'm trying to add these flag notations from DSIM controller and how they handle the driver. HBP/HFP/HSA mode bits in i.MX8M Mini/Nano/Plus Processor Reference Manuals specify a naming conversion as 'disable mode bit' due to its
Re: [Freedreno] [PATCH 2/2] drm/bridge: lt9611: Do not generate HFP/HBP/HSA and EOT packet
On 7/13/2023 11:28 AM, Marek Vasut wrote: On 7/13/23 20:09, Abhinav Kumar wrote: On 7/12/2023 10:41 AM, Marek Vasut wrote: On 7/9/23 03:03, Abhinav Kumar wrote: On 7/7/2023 1:47 AM, Neil Armstrong wrote: On 07/07/2023 09:18, Neil Armstrong wrote: Hi, On 06/07/2023 11:20, Amit Pundir wrote: On Wed, 5 Jul 2023 at 11:09, Dmitry Baryshkov wrote: [Adding freedreno@ to cc list] On Wed, 5 Jul 2023 at 08:31, Jagan Teki wrote: Hi Amit, On Wed, Jul 5, 2023 at 10:15 AM Amit Pundir wrote: Hi Marek, On Wed, 5 Jul 2023 at 01:48, Marek Vasut wrote: Do not generate the HS front and back porch gaps, the HSA gap and EOT packet, as these packets are not required. This makes the bridge work with Samsung DSIM on i.MX8MM and i.MX8MP. This patch broke display on Dragonboard 845c (SDM845) devboard running AOSP. This is what I see https://people.linaro.org/~amit.pundir/db845c-userdebug/v6.5-broken-display/PXL_20230704_150156326.jpg. Reverting this patch fixes this regression for me. Might be msm dsi host require proper handling on these updated mode_flags? did they? The msm DSI host supports those flags. Also, I'd like to point out that the patch didn't change the rest of the driver code. So even if drm/msm ignored some of the flags, it should not have caused the issue. Most likely the issue is on the lt9611 side. I's suspect that additional programming is required to make it work with these flags. I spent some time today on smoke testing these flags (individually and in limited combination) on DB845c, to narrow down this breakage to one or more flag(s) triggering it. Here are my observations in limited testing done so far. There is no regression with MIPI_DSI_MODE_NO_EOT_PACKET when enabled alone and system boots to UI as usual. MIPI_DSI_MODE_VIDEO_NO_HFP always trigger the broken display as in the screenshot[1] shared earlier as well. Adding either of MIPI_DSI_MODE_VIDEO_NO_HSA and MIPI_DSI_MODE_VIDEO_NO_HBP always result in no display, unless paired with MIPI_DSI_MODE_VIDEO_NO_HFP and in that case we get the broken display as reported. In short other than MIPI_DSI_MODE_NO_EOT_PACKET flag, all other flags added in this commit break the display on DB845c one way or another. I think the investigation would be to understand why samsung-dsim requires such flags and/or what are the difference in behavior between MSM DSI and samsung DSIM for those flags ? If someone has access to the lt9611 datasheet, so it requires HSA/HFP/HBP to be skipped ? and does MSM DSI and samsung DSIM skip them in the same way ? I think there's a mismatch, where on one side this flags sets the link in LP-11 while in HSA/HFP/HPB while on the other it completely removes those blanking packets. The name MIPI_DSI_MODE_VIDEO_NO_HBP suggests removal of HPB, not LP-11 while HPB. the registers used in both controllers are different: - samsung-dsim: DSIM_HBP_DISABLE_MODE - msm dsi: DSI_VID_CFG0_HBP_POWER_STOP The first one suggest removing the packet, while the second one suggests powering off the line while in the blanking packet period. @Abhinav, can you comment on that ? I dont get what it means by completely removes blanking packets in DSIM. MIPI_DSI_MODE_VIDEO_NO_HFP means the HBP period is just skipped by DSIM. Maybe there is a need for new set of flags which differentiate between HBP skipped (i.e. NO HBP) and HBP LP11 ? No, the section of the MIPI DSI spec I posted below clearly states there are two options: 1) send blanking packets during those periods 2) transition to LP11 during those periods There is no 3rd option in the spec of not doing both like what you are suggesting. So DSIM should also be only transitioning to LP11 during those periods if its not sending the blanking packets with those flags set. So, there is no need for any new set of flags to differentiate. The flags and their interpretation is correct in MSM driver. I cannot comment on what exactly DSIM does with those flags. How do you explain the comment in include/drm/drm_mipi_dsi.h: 128 /* disable hback-porch area */ 129 #define MIPI_DSI_MODE_VIDEO_NO_HBP BIT(6) Esp. the "disable" part. That to me reads as "don't send HBP packet". Yes the LP11 option doesnt send HBP packet. That comment should have said "Dont send HBP packet and use LP11 instead" to match the spec. Like I said there are two options: 1) Send the blanking (HBP) packet 2) Dont send the packet and transition to LP11 Thats what those flags are controlling and thats what MSM driver does too. Where do you see that quote above in the DSI spec (which chapter and which version do you read) ? I am referring "8.11.2 Non-Burst Mode with Sync Pulses" of MIPI DSI 1.2 spec ( its slightly old ) but this part doenst change across revisions. It should be replacing those periods with LP11 too. The traffic mode being used on this bridge is MIPI_DSI_MODE_VIDEO_SYNC_PULSE which is "Non-Burst Mode with Sync
Re: [Freedreno] [PATCH 2/2] drm/bridge: lt9611: Do not generate HFP/HBP/HSA and EOT packet
On 7/13/23 20:09, Abhinav Kumar wrote: On 7/12/2023 10:41 AM, Marek Vasut wrote: On 7/9/23 03:03, Abhinav Kumar wrote: On 7/7/2023 1:47 AM, Neil Armstrong wrote: On 07/07/2023 09:18, Neil Armstrong wrote: Hi, On 06/07/2023 11:20, Amit Pundir wrote: On Wed, 5 Jul 2023 at 11:09, Dmitry Baryshkov wrote: [Adding freedreno@ to cc list] On Wed, 5 Jul 2023 at 08:31, Jagan Teki wrote: Hi Amit, On Wed, Jul 5, 2023 at 10:15 AM Amit Pundir wrote: Hi Marek, On Wed, 5 Jul 2023 at 01:48, Marek Vasut wrote: Do not generate the HS front and back porch gaps, the HSA gap and EOT packet, as these packets are not required. This makes the bridge work with Samsung DSIM on i.MX8MM and i.MX8MP. This patch broke display on Dragonboard 845c (SDM845) devboard running AOSP. This is what I see https://people.linaro.org/~amit.pundir/db845c-userdebug/v6.5-broken-display/PXL_20230704_150156326.jpg. Reverting this patch fixes this regression for me. Might be msm dsi host require proper handling on these updated mode_flags? did they? The msm DSI host supports those flags. Also, I'd like to point out that the patch didn't change the rest of the driver code. So even if drm/msm ignored some of the flags, it should not have caused the issue. Most likely the issue is on the lt9611 side. I's suspect that additional programming is required to make it work with these flags. I spent some time today on smoke testing these flags (individually and in limited combination) on DB845c, to narrow down this breakage to one or more flag(s) triggering it. Here are my observations in limited testing done so far. There is no regression with MIPI_DSI_MODE_NO_EOT_PACKET when enabled alone and system boots to UI as usual. MIPI_DSI_MODE_VIDEO_NO_HFP always trigger the broken display as in the screenshot[1] shared earlier as well. Adding either of MIPI_DSI_MODE_VIDEO_NO_HSA and MIPI_DSI_MODE_VIDEO_NO_HBP always result in no display, unless paired with MIPI_DSI_MODE_VIDEO_NO_HFP and in that case we get the broken display as reported. In short other than MIPI_DSI_MODE_NO_EOT_PACKET flag, all other flags added in this commit break the display on DB845c one way or another. I think the investigation would be to understand why samsung-dsim requires such flags and/or what are the difference in behavior between MSM DSI and samsung DSIM for those flags ? If someone has access to the lt9611 datasheet, so it requires HSA/HFP/HBP to be skipped ? and does MSM DSI and samsung DSIM skip them in the same way ? I think there's a mismatch, where on one side this flags sets the link in LP-11 while in HSA/HFP/HPB while on the other it completely removes those blanking packets. The name MIPI_DSI_MODE_VIDEO_NO_HBP suggests removal of HPB, not LP-11 while HPB. the registers used in both controllers are different: - samsung-dsim: DSIM_HBP_DISABLE_MODE - msm dsi: DSI_VID_CFG0_HBP_POWER_STOP The first one suggest removing the packet, while the second one suggests powering off the line while in the blanking packet period. @Abhinav, can you comment on that ? I dont get what it means by completely removes blanking packets in DSIM. MIPI_DSI_MODE_VIDEO_NO_HFP means the HBP period is just skipped by DSIM. Maybe there is a need for new set of flags which differentiate between HBP skipped (i.e. NO HBP) and HBP LP11 ? No, the section of the MIPI DSI spec I posted below clearly states there are two options: 1) send blanking packets during those periods 2) transition to LP11 during those periods There is no 3rd option in the spec of not doing both like what you are suggesting. So DSIM should also be only transitioning to LP11 during those periods if its not sending the blanking packets with those flags set. So, there is no need for any new set of flags to differentiate. The flags and their interpretation is correct in MSM driver. I cannot comment on what exactly DSIM does with those flags. How do you explain the comment in include/drm/drm_mipi_dsi.h: 128 /* disable hback-porch area */ 129 #define MIPI_DSI_MODE_VIDEO_NO_HBP BIT(6) Esp. the "disable" part. That to me reads as "don't send HBP packet". Where do you see that quote above in the DSI spec (which chapter and which version do you read) ? It should be replacing those periods with LP11 too. The traffic mode being used on this bridge is MIPI_DSI_MODE_VIDEO_SYNC_PULSE which is "Non-Burst Mode with Sync Pulses". As per this traffic mode in the DSI spec, "Normally, periods shown as HSA (Horizontal Sync Active), HBP (Horizontal Back Porch) and HFP (Horizontal Front Porch) are filled by Blanking Packets, with lengths (including packet overhead) calculated to match the period specified by the peripheral’s data sheet. Alternatively, if there is sufficient time to transition from HS to LP mode and back again, a timed interval in LP mode may substitute for a Blanking Packet, thus saving power. During HSA, HBP and HFP periods, the bus
Re: [Freedreno] [PATCH 2/2] drm/bridge: lt9611: Do not generate HFP/HBP/HSA and EOT packet
On 7/12/2023 10:41 AM, Marek Vasut wrote: On 7/9/23 03:03, Abhinav Kumar wrote: On 7/7/2023 1:47 AM, Neil Armstrong wrote: On 07/07/2023 09:18, Neil Armstrong wrote: Hi, On 06/07/2023 11:20, Amit Pundir wrote: On Wed, 5 Jul 2023 at 11:09, Dmitry Baryshkov wrote: [Adding freedreno@ to cc list] On Wed, 5 Jul 2023 at 08:31, Jagan Teki wrote: Hi Amit, On Wed, Jul 5, 2023 at 10:15 AM Amit Pundir wrote: Hi Marek, On Wed, 5 Jul 2023 at 01:48, Marek Vasut wrote: Do not generate the HS front and back porch gaps, the HSA gap and EOT packet, as these packets are not required. This makes the bridge work with Samsung DSIM on i.MX8MM and i.MX8MP. This patch broke display on Dragonboard 845c (SDM845) devboard running AOSP. This is what I see https://people.linaro.org/~amit.pundir/db845c-userdebug/v6.5-broken-display/PXL_20230704_150156326.jpg. Reverting this patch fixes this regression for me. Might be msm dsi host require proper handling on these updated mode_flags? did they? The msm DSI host supports those flags. Also, I'd like to point out that the patch didn't change the rest of the driver code. So even if drm/msm ignored some of the flags, it should not have caused the issue. Most likely the issue is on the lt9611 side. I's suspect that additional programming is required to make it work with these flags. I spent some time today on smoke testing these flags (individually and in limited combination) on DB845c, to narrow down this breakage to one or more flag(s) triggering it. Here are my observations in limited testing done so far. There is no regression with MIPI_DSI_MODE_NO_EOT_PACKET when enabled alone and system boots to UI as usual. MIPI_DSI_MODE_VIDEO_NO_HFP always trigger the broken display as in the screenshot[1] shared earlier as well. Adding either of MIPI_DSI_MODE_VIDEO_NO_HSA and MIPI_DSI_MODE_VIDEO_NO_HBP always result in no display, unless paired with MIPI_DSI_MODE_VIDEO_NO_HFP and in that case we get the broken display as reported. In short other than MIPI_DSI_MODE_NO_EOT_PACKET flag, all other flags added in this commit break the display on DB845c one way or another. I think the investigation would be to understand why samsung-dsim requires such flags and/or what are the difference in behavior between MSM DSI and samsung DSIM for those flags ? If someone has access to the lt9611 datasheet, so it requires HSA/HFP/HBP to be skipped ? and does MSM DSI and samsung DSIM skip them in the same way ? I think there's a mismatch, where on one side this flags sets the link in LP-11 while in HSA/HFP/HPB while on the other it completely removes those blanking packets. The name MIPI_DSI_MODE_VIDEO_NO_HBP suggests removal of HPB, not LP-11 while HPB. the registers used in both controllers are different: - samsung-dsim: DSIM_HBP_DISABLE_MODE - msm dsi: DSI_VID_CFG0_HBP_POWER_STOP The first one suggest removing the packet, while the second one suggests powering off the line while in the blanking packet period. @Abhinav, can you comment on that ? I dont get what it means by completely removes blanking packets in DSIM. MIPI_DSI_MODE_VIDEO_NO_HFP means the HBP period is just skipped by DSIM. Maybe there is a need for new set of flags which differentiate between HBP skipped (i.e. NO HBP) and HBP LP11 ? No, the section of the MIPI DSI spec I posted below clearly states there are two options: 1) send blanking packets during those periods 2) transition to LP11 during those periods There is no 3rd option in the spec of not doing both like what you are suggesting. So DSIM should also be only transitioning to LP11 during those periods if its not sending the blanking packets with those flags set. So, there is no need for any new set of flags to differentiate. The flags and their interpretation is correct in MSM driver. I cannot comment on what exactly DSIM does with those flags. It should be replacing those periods with LP11 too. The traffic mode being used on this bridge is MIPI_DSI_MODE_VIDEO_SYNC_PULSE which is "Non-Burst Mode with Sync Pulses". As per this traffic mode in the DSI spec, "Normally, periods shown as HSA (Horizontal Sync Active), HBP (Horizontal Back Porch) and HFP (Horizontal Front Porch) are filled by Blanking Packets, with lengths (including packet overhead) calculated to match the period specified by the peripheral’s data sheet. Alternatively, if there is sufficient time to transition from HS to LP mode and back again, a timed interval in LP mode may substitute for a Blanking Packet, thus saving power. During HSA, HBP and HFP periods, the bus should stay in the LP-11 state." So we can either send the blanking packets or transition to LP state and those 3 flags are controlling exactly that during those periods for MSM driver. If you stop sending the blanking packets, you need to replace that gap with LP. I don't think that's what MIPI_DSI_MODE_VIDEO_NO_HBP means, the way I understand
Re: [Freedreno] [PATCH v2 2/3] drm/msm: Fix IS_ERR() vs NULL check in a5xx_submit_in_rb()
On 7/12/2023 7:05 PM, Gaosheng Cui wrote:
The msm_gem_get_vaddr() returns an ERR_PTR() on failure, we should
use IS_ERR() to check the return value.
Fixes: 6a8bd08d0465 ("drm/msm: add sudo flag to submit ioctl")
Signed-off-by: Gaosheng Cui
Reviewed-by: Abhinav Kumar
---
You dropped Dmitry's R-b in this revision.
https://patchwork.freedesktop.org/patch/511035/?series=110745=1
If you are going to spin a v3, pls add it back. Otherwise I will while
applying.
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index a99310b68793..a499e3b350fc 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -89,7 +89,7 @@ static void a5xx_submit_in_rb(struct msm_gpu *gpu, struct
msm_gem_submit *submit
* since we've already mapped it once in
* submit_reloc()
*/
- if (WARN_ON(!ptr))
+ if (WARN_ON(IS_ERR(ptr)))
return;
for (i = 0; i < dwords; i++) {
[Freedreno] [PATCH v5 2/2] arm64: dts: qcom: sdm845-db845c: Mark cont splash memory region as reserved
Adding a reserved memory region for the framebuffer memory
(the splash memory region set up by the bootloader).
Signed-off-by: Amit Pundir
---
v5: Re-sending with updated dt-bindings patch in mdss-common
schema.
v4: Re-sending this along with a new dt-bindings patch to
document memory-region property in qcom,sdm845-mdss
schema and keep dtbs_check happy.
v3: Point this reserved region to MDSS.
v2: Updated commit message.
There was some dicussion on v1 but it didn't go anywhere,
https://lore.kernel.org/linux-kernel/20230124182857.1524912-1-amit.pun...@linaro.org/T/#u.
The general consensus is that this memory should be freed and be
made resuable but that (releasing this piece of memory) has been
tried before and it is not trivial to return the reserved memory
node to the system RAM pool in this case.
arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 9 +
1 file changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
index d6b464cb61d6..f546f6f57c1e 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
@@ -101,6 +101,14 @@ hdmi_con: endpoint {
};
};
+ reserved-memory {
+ /* Cont splash region set up by the bootloader */
+ cont_splash_mem: framebuffer@9d40 {
+ reg = <0x0 0x9d40 0x0 0x240>;
+ no-map;
+ };
+ };
+
lt9611_1v8: lt9611-vdd18-regulator {
compatible = "regulator-fixed";
regulator-name = "LT9611_1V8";
@@ -506,6 +514,7 @@ {
};
{
+ memory-region = <_splash_mem>;
status = "okay";
};
--
2.25.1
[Freedreno] [PATCH v5 1/2] dt-bindings: display/msm: mdss-common: add memory-region property
Add and document the reserved memory region property in the mdss-common schema. For now (sdm845-db845c), it points to a framebuffer memory region reserved by the bootloader for splash screen. Signed-off-by: Amit Pundir --- v5: Moving the dt-binding to mdss-common schema with updated commit message and property description. v4: Adding this new dt-binding patch, in qcom,sdm845-mdss schema, in the v4 of the follow-up patch for sdm845-db845c. https://lore.kernel.org/lkml/20230712130215.666924-2-amit.pun...@linaro.org/ .../devicetree/bindings/display/msm/mdss-common.yaml | 5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml index ccd7d6417523..84ed2757ded5 100644 --- a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml +++ b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml @@ -77,6 +77,12 @@ properties: items: - description: MDSS_CORE reset + memory-region: +maxItems: 1 +description: + Phandle to a node describing a reserved framebuffer memory region. + For example, the splash memory region set up by the bootloader. + required: - reg - reg-names -- 2.25.1
Re: [Freedreno] [PATCH 1/2] dt-bindings: display/msm: qcom, sdm845-mdss: add memory-region property
On Thu, 13 Jul 2023 at 21:13, Dmitry Baryshkov wrote: > > On Thu, 13 Jul 2023 at 18:34, Amit Pundir wrote: > > > > On Wed, 12 Jul 2023 at 18:45, Dmitry Baryshkov > > wrote: > > > > > > On 12/07/2023 16:02, Amit Pundir wrote: > > > > Add and document the reserved memory region property > > > > in the qcom,sdm845-mdss schema. > > > > > > > > Signed-off-by: Amit Pundir > > > > --- > > > > .../devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml| 5 + > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > > > b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > > > index 6ecb00920d7f..3ea1dbd7e317 100644 > > > > --- > > > > a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > > > +++ > > > > b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > > > @@ -39,6 +39,11 @@ properties: > > > > interconnect-names: > > > > maxItems: 2 > > > > > > > > + memory-region: > > > > +maxItems: 1 > > > > +description: > > > > + Phandle to a node describing a reserved memory region. > > > > + > > > > > > Please add it to mdss-common.yaml instead > > > > mdss-common.yaml didn't like this property at all and > > I ran into a lot of new dtbs_check warnings: > > https://www.irccloud.com/pastebin/raw/pEYAeaB1 > > > > I need some help in decoding these please. > > I'm not sure what happened there (and it's hard to understand without > seeing your patch). Yup.. It was my broken patch. I used "For example:" in the property description and it tripped off the checks. Didn't realise that casually used ":" can break yaml parsing until now. Sorry for all the noise. Regards, Amit Pundir > But after applying your patch to mdss-common.yaml, > `make dt_binding_check' passes: > > diff --git a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml > b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml > index ccd7d6417523..924fe383e4a1 100644 > --- a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml > +++ b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml > @@ -77,6 +77,11 @@ properties: > items: >- description: MDSS_CORE reset > > + memory-region: > +maxItems: 1 > +description: > + Phandle to a node describing a reserved memory region. > + > required: >- reg >- reg-names > > > > > > Regards, > > Amit Pundir > > > > > > > > > patternProperties: > > > > "^display-controller@[0-9a-f]+$": > > > > type: object > > > > > > -- > > > With best wishes > > > Dmitry > > > > > > > -- > With best wishes > Dmitry
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Thu, Jul 13, 2023 at 10:41:45AM -0400, Sean Paul wrote: > On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König > > But even with the one-patch-per-rename approach I'd consider the > > renaming a net win, because ease of understanding code has a big value. > > It's value is not so easy measurable as "conflicts when backporting", > > but it also matters in say two years from now, while backporting > > shouldn't be an issue then any more. > > You've rightly identified the conjecture in your statement. I've been > on both sides of the argument, having written/maintained drm code > upstream and cherry-picked changes to a downstream kernel. Perhaps > it's because drm's definition of dev is ingrained in my muscle memory, > or maybe it's because I don't do a lot of upstream development these > days, but I just have a hard time seeing the benefit here. I see that my change doesn't immediately benefit those who are already at home in drivers/gpu/drm. So it's quite understandable that someone in a senior role (long time contributor or maintainer) doesn't see a big upside. In contrast I think my change (with its indisputable cost) lowers the bar for new contributors to drm. IMHO that's something a maintainer has to have on their radar, too, and that's easily undervalued in my experience. Of course in the end it's about weighting the ups and downs. Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Am 13.07.23 um 16:41 schrieb Sean Paul: On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König wrote: hello Sean, On Wed, Jul 12, 2023 at 02:31:02PM -0400, Sean Paul wrote: I'd really prefer this patch (series or single) is not accepted. This will cause problems for everyone cherry-picking patches to a downstream kernel (LTS or distro tree). I usually wouldn't expect sympathy here, but the questionable benefit does not outweigh the cost IM[biased]O. I agree that for backports this isn't so nice. However with the split approach (that was argumented against here) it's not soo bad. Patch #1 (and similar changes for the other affected structures) could be trivially backported and with that it doesn't matter if you write dev or drm (or whatever name is chosen in the end); both work in the same way. Patch #1 avoids the need to backport the entire set, however every change occuring after the rename patches will cause conflicts on future cherry-picks. Downstream kernels will have to backport the whole set. Backporting the entire set will create an epoch in downstream kernels where cherry-picking patches preceding this set will need to undergo conflict resolution as well. As mentioned in my previous email, I don't expect sympathy here, it's part of maintaining a downstream kernel, but there is a real cost to kernel consumers. But even with the one-patch-per-rename approach I'd consider the renaming a net win, because ease of understanding code has a big value. It's value is not so easy measurable as "conflicts when backporting", but it also matters in say two years from now, while backporting shouldn't be an issue then any more. You've rightly identified the conjecture in your statement. I've been on both sides of the argument, having written/maintained drm code upstream and cherry-picked changes to a downstream kernel. Perhaps it's because drm's definition of dev is ingrained in my muscle memory, or maybe it's because I don't do a lot of upstream development these days, but I just have a hard time seeing the benefit here. I can only second what Sean writes. I've done quite a bit of backporting of DRM code. It's hard already. And this kind of change is going to to affect almost every backported DRM patch in the coming years. Not just for distribution kernels, but also for upstream's stable series. It's really only possible to do this change over many releases while keeping compatible with the old name. So the more I think about it, the less I like this change. Best regards Thomas I appreciate your engagement on the topic, thank you! Sean Thanks for your input, best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg) OpenPGP_signature Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On 13/07/2023 16:09, Thomas Zimmermann wrote: Hi Am 13.07.23 um 16:41 schrieb Sean Paul: On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König wrote: hello Sean, On Wed, Jul 12, 2023 at 02:31:02PM -0400, Sean Paul wrote: I'd really prefer this patch (series or single) is not accepted. This will cause problems for everyone cherry-picking patches to a downstream kernel (LTS or distro tree). I usually wouldn't expect sympathy here, but the questionable benefit does not outweigh the cost IM[biased]O. I agree that for backports this isn't so nice. However with the split approach (that was argumented against here) it's not soo bad. Patch #1 (and similar changes for the other affected structures) could be trivially backported and with that it doesn't matter if you write dev or drm (or whatever name is chosen in the end); both work in the same way. Patch #1 avoids the need to backport the entire set, however every change occuring after the rename patches will cause conflicts on future cherry-picks. Downstream kernels will have to backport the whole set. Backporting the entire set will create an epoch in downstream kernels where cherry-picking patches preceding this set will need to undergo conflict resolution as well. As mentioned in my previous email, I don't expect sympathy here, it's part of maintaining a downstream kernel, but there is a real cost to kernel consumers. But even with the one-patch-per-rename approach I'd consider the renaming a net win, because ease of understanding code has a big value. It's value is not so easy measurable as "conflicts when backporting", but it also matters in say two years from now, while backporting shouldn't be an issue then any more. You've rightly identified the conjecture in your statement. I've been on both sides of the argument, having written/maintained drm code upstream and cherry-picked changes to a downstream kernel. Perhaps it's because drm's definition of dev is ingrained in my muscle memory, or maybe it's because I don't do a lot of upstream development these days, but I just have a hard time seeing the benefit here. I can only second what Sean writes. I've done quite a bit of backporting of DRM code. It's hard already. And this kind of change is going to to affect almost every backported DRM patch in the coming years. Not just for distribution kernels, but also for upstream's stable series. It's really only possible to do this change over many releases while keeping compatible with the old name. So the more I think about it, the less I like this change. I've done my share of backporting, and still am doing it, so I can say I dislike it as much as anyone, however.. Is this an argument which the kernel as a wider entity typically accepts? If not could it be a slippery slope to start a precedent? It is a honest question - I am not familiar if there were or were not any similar discussions in the past. My gut feeling is that *if* there is a consensus that something _improves_ the code base significantly, backporting pains should probably not be weighted very heavily as a contra argument. Regards, Tvrtko
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König wrote: > > hello Sean, > > On Wed, Jul 12, 2023 at 02:31:02PM -0400, Sean Paul wrote: > > I'd really prefer this patch (series or single) is not accepted. This > > will cause problems for everyone cherry-picking patches to a > > downstream kernel (LTS or distro tree). I usually wouldn't expect > > sympathy here, but the questionable benefit does not outweigh the cost > > IM[biased]O. > > I agree that for backports this isn't so nice. However with the split > approach (that was argumented against here) it's not soo bad. Patch #1 > (and similar changes for the other affected structures) could be > trivially backported and with that it doesn't matter if you write dev or > drm (or whatever name is chosen in the end); both work in the same way. Patch #1 avoids the need to backport the entire set, however every change occuring after the rename patches will cause conflicts on future cherry-picks. Downstream kernels will have to backport the whole set. Backporting the entire set will create an epoch in downstream kernels where cherry-picking patches preceding this set will need to undergo conflict resolution as well. As mentioned in my previous email, I don't expect sympathy here, it's part of maintaining a downstream kernel, but there is a real cost to kernel consumers. > > But even with the one-patch-per-rename approach I'd consider the > renaming a net win, because ease of understanding code has a big value. > It's value is not so easy measurable as "conflicts when backporting", > but it also matters in say two years from now, while backporting > shouldn't be an issue then any more. You've rightly identified the conjecture in your statement. I've been on both sides of the argument, having written/maintained drm code upstream and cherry-picked changes to a downstream kernel. Perhaps it's because drm's definition of dev is ingrained in my muscle memory, or maybe it's because I don't do a lot of upstream development these days, but I just have a hard time seeing the benefit here. I appreciate your engagement on the topic, thank you! Sean > > Thanks for your input, best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König| > Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Thu, Jul 13, 2023 at 04:14:55PM +0100, Tvrtko Ursulin wrote: > > On 13/07/2023 16:09, Thomas Zimmermann wrote: > > Hi > > > > Am 13.07.23 um 16:41 schrieb Sean Paul: > > > On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König > > > wrote: > > > > > > > > hello Sean, > > > > > > > > On Wed, Jul 12, 2023 at 02:31:02PM -0400, Sean Paul wrote: > > > > > I'd really prefer this patch (series or single) is not accepted. This > > > > > will cause problems for everyone cherry-picking patches to a > > > > > downstream kernel (LTS or distro tree). I usually wouldn't expect > > > > > sympathy here, but the questionable benefit does not outweigh the cost > > > > > IM[biased]O. > > > > > > > > I agree that for backports this isn't so nice. However with the split > > > > approach (that was argumented against here) it's not soo bad. Patch #1 > > > > (and similar changes for the other affected structures) could be > > > > trivially backported and with that it doesn't matter if you write dev or > > > > drm (or whatever name is chosen in the end); both work in the same way. > > > > > > Patch #1 avoids the need to backport the entire set, however every > > > change occuring after the rename patches will cause conflicts on > > > future cherry-picks. Downstream kernels will have to backport the > > > whole set. Backporting the entire set will create an epoch in > > > downstream kernels where cherry-picking patches preceding this set > > > will need to undergo conflict resolution as well. As mentioned in my > > > previous email, I don't expect sympathy here, it's part of maintaining > > > a downstream kernel, but there is a real cost to kernel consumers. > > > > > > > > > > > But even with the one-patch-per-rename approach I'd consider the > > > > renaming a net win, because ease of understanding code has a big value. > > > > It's value is not so easy measurable as "conflicts when backporting", > > > > but it also matters in say two years from now, while backporting > > > > shouldn't be an issue then any more. > > > > > > You've rightly identified the conjecture in your statement. I've been > > > on both sides of the argument, having written/maintained drm code > > > upstream and cherry-picked changes to a downstream kernel. Perhaps > > > it's because drm's definition of dev is ingrained in my muscle memory, > > > or maybe it's because I don't do a lot of upstream development these > > > days, but I just have a hard time seeing the benefit here. > > > > I can only second what Sean writes. I've done quite a bit of backporting > > of DRM code. It's hard already. And this kind of change is going to to > > affect almost every backported DRM patch in the coming years. Not just > > for distribution kernels, but also for upstream's stable series. It's > > really only possible to do this change over many releases while keeping > > compatible with the old name. So the more I think about it, the less I > > like this change. > > I've done my share of backporting, and still am doing it, so I can say I > dislike it as much as anyone, however.. Is this an argument which the kernel > as a wider entity typically accepts? If not could it be a slippery slope to > start a precedent? > > It is a honest question - I am not familiar if there were or were not any > similar discussions in the past. Eventually, it's a trade-off. There's always pros and cons to merging every patch, and "backporting pains" is indeed not a very strong con. But it's definitely the kind of patch where everyone and their mother will have their opinion, without every reaching a clear consensus, and there's no clear benefit either (but I might be biaised on that one). So imo, while that downside is fairly weak, the pros are certainly weaker. > My gut feeling is that *if* there is a consensus that something _improves_ > the code base significantly, backporting pains should probably not be > weighted very heavily as a contra argument. 100% agreed here, but I'm afraid we're far from that point. Maxime signature.asc Description: PGP signature
Re: [Freedreno] [PATCH 1/2] dt-bindings: display/msm: qcom, sdm845-mdss: add memory-region property
On Thu, 13 Jul 2023 at 18:34, Amit Pundir wrote: > > On Wed, 12 Jul 2023 at 18:45, Dmitry Baryshkov > wrote: > > > > On 12/07/2023 16:02, Amit Pundir wrote: > > > Add and document the reserved memory region property > > > in the qcom,sdm845-mdss schema. > > > > > > Signed-off-by: Amit Pundir > > > --- > > > .../devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml| 5 + > > > 1 file changed, 5 insertions(+) > > > > > > diff --git > > > a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > > b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > > index 6ecb00920d7f..3ea1dbd7e317 100644 > > > --- a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > > +++ b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > > @@ -39,6 +39,11 @@ properties: > > > interconnect-names: > > > maxItems: 2 > > > > > > + memory-region: > > > +maxItems: 1 > > > +description: > > > + Phandle to a node describing a reserved memory region. > > > + > > > > Please add it to mdss-common.yaml instead > > mdss-common.yaml didn't like this property at all and > I ran into a lot of new dtbs_check warnings: > https://www.irccloud.com/pastebin/raw/pEYAeaB1 > > I need some help in decoding these please. I'm not sure what happened there (and it's hard to understand without seeing your patch). But after applying your patch to mdss-common.yaml, `make dt_binding_check' passes: diff --git a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml index ccd7d6417523..924fe383e4a1 100644 --- a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml +++ b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml @@ -77,6 +77,11 @@ properties: items: - description: MDSS_CORE reset + memory-region: +maxItems: 1 +description: + Phandle to a node describing a reserved memory region. + required: - reg - reg-names > > Regards, > Amit Pundir > > > > > > patternProperties: > > > "^display-controller@[0-9a-f]+$": > > > type: object > > > > -- > > With best wishes > > Dmitry > > -- With best wishes Dmitry
Re: [Freedreno] [PATCH 1/2] dt-bindings: display/msm: qcom, sdm845-mdss: add memory-region property
On Wed, 12 Jul 2023 at 19:46, Krzysztof Kozlowski wrote: > > On 12/07/2023 15:02, Amit Pundir wrote: > > Add and document the reserved memory region property > > in the qcom,sdm845-mdss schema. > > > > Signed-off-by: Amit Pundir > > Please keep consistent versioning, so this is new patch in v4. ACK. > > > --- > > .../devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml| 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > index 6ecb00920d7f..3ea1dbd7e317 100644 > > --- a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > +++ b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > @@ -39,6 +39,11 @@ properties: > >interconnect-names: > > maxItems: 2 > > > > + memory-region: > > +maxItems: 1 > > +description: > > + Phandle to a node describing a reserved memory region. > > Your description says nothing new. It's entirely redundant/obvious. > Instead please describe what reserved memory is expected to be here. On it. I'll update in v5. Thanks. Regards, Amit Pundir > > > Best regards, > Krzysztof >
Re: [Freedreno] [PATCH 1/2] dt-bindings: display/msm: qcom, sdm845-mdss: add memory-region property
On Wed, 12 Jul 2023 at 18:45, Dmitry Baryshkov wrote: > > On 12/07/2023 16:02, Amit Pundir wrote: > > Add and document the reserved memory region property > > in the qcom,sdm845-mdss schema. > > > > Signed-off-by: Amit Pundir > > --- > > .../devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml| 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > index 6ecb00920d7f..3ea1dbd7e317 100644 > > --- a/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > +++ b/Documentation/devicetree/bindings/display/msm/qcom,sdm845-mdss.yaml > > @@ -39,6 +39,11 @@ properties: > > interconnect-names: > > maxItems: 2 > > > > + memory-region: > > +maxItems: 1 > > +description: > > + Phandle to a node describing a reserved memory region. > > + > > Please add it to mdss-common.yaml instead mdss-common.yaml didn't like this property at all and I ran into a lot of new dtbs_check warnings: https://www.irccloud.com/pastebin/raw/pEYAeaB1 I need some help in decoding these please. Regards, Amit Pundir > > > patternProperties: > > "^display-controller@[0-9a-f]+$": > > type: object > > -- > With best wishes > Dmitry >
Re: [Freedreno] [PATCH v2 3/3] drm/komeda: Fix IS_ERR() vs NULL check in komeda_component_get_avail_scaler()
On Thu, Jul 13, 2023 at 05:51:34PM +0800, cuigaosheng wrote:
> Thanks for taking time to review this patch.
>
> Maybe we can submit another separate patch to fix ERR_CAST(st), because I'm
> not
> sure which commit should be used as a fixes-tag.
>
> Also, Maybe we should fix ERR_CAST(st) in
> komeda_pipeline_get_state_and_set_crtc()
> and komeda_component_get_state_and_set_user(), please make another separate
> patch.
Yeah, you're right, there are other places where this needs to be fixed.
I will add this to my list of ToDos.
Best regards,
Liviu
>
> Let me know if there's anything I can do, thanks for your work again!
>
> Gaosheng,
>
> On 2023/7/13 16:54, Liviu Dudau wrote:
> > Hello,
> >
> > On Thu, Jul 13, 2023 at 10:05:56AM +0800, Gaosheng Cui wrote:
> > > The komeda_pipeline_get_state() returns an ERR_PTR() on failure, we should
> > > use IS_ERR() to check the return value.
> > While reviewing the change I've realised that
> > komeda_pipeline_get_state_and_set_crtc()
> > is also mishandling the return value from komeda_pipeline_get_state(). If
> > IS_ERR(st) is
> > true it should use return ERR_CAST(st), following the same pattern as
> > komeda_pipeline_get_state().
> >
> > If you don't want to update this patch I can send a separate patch.
> > Otherwise,
> > the change looks good to me.
> >
> > Reviewed-by: Liviu Dudau
> >
> > Best regards,
> > Liviu
> >
> >
> > > Fixes: 502932a03fce ("drm/komeda: Add the initial scaler support for
> > > CORE")
> > > Signed-off-by: Gaosheng Cui
> > > ---
> > > drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > > b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > > index 3276a3e82c62..e9c92439398d 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > > @@ -259,7 +259,7 @@ komeda_component_get_avail_scaler(struct
> > > komeda_component *c,
> > > u32 avail_scalers;
> > > pipe_st = komeda_pipeline_get_state(c->pipeline, state);
> > > - if (!pipe_st)
> > > + if (IS_ERR(pipe_st))
> > > return NULL;
> > > avail_scalers = (pipe_st->active_comps &
> > > KOMEDA_PIPELINE_SCALERS) ^
> > > --
> > > 2.25.1
> > >
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ツ)_/¯
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
hello Sean, On Wed, Jul 12, 2023 at 02:31:02PM -0400, Sean Paul wrote: > I'd really prefer this patch (series or single) is not accepted. This > will cause problems for everyone cherry-picking patches to a > downstream kernel (LTS or distro tree). I usually wouldn't expect > sympathy here, but the questionable benefit does not outweigh the cost > IM[biased]O. I agree that for backports this isn't so nice. However with the split approach (that was argumented against here) it's not soo bad. Patch #1 (and similar changes for the other affected structures) could be trivially backported and with that it doesn't matter if you write dev or drm (or whatever name is chosen in the end); both work in the same way. But even with the one-patch-per-rename approach I'd consider the renaming a net win, because ease of understanding code has a big value. It's value is not so easy measurable as "conflicts when backporting", but it also matters in say two years from now, while backporting shouldn't be an issue then any more. Thanks for your input, best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Am 12.07.23 um 11:46 schrieb Uwe Kleine-König: Hello, while I debugged an issue in the imx-lcdc driver I was constantly irritated about struct drm_device pointer variables being named "dev" because with that name I usually expect a struct device pointer. Rather than renaming dev in all the DRM structs, did you consider renaming struct drm_device.dev instead? Everyone in DRM-land knows that 'dev' is the DRM device. But for struct drm_device.dev a more expressive name would be helpful. Maybe 'parent'. (It's also much less churn.) Best regards Thomas I think there is a big benefit when these are all renamed to "drm_dev". I have no strong preference here though, so "drmdev" or "drm" are fine for me, too. Let the bikesheding begin! Some statistics: $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | sort -n 1 struct drm_device *adev_to_drm 1 struct drm_device *drm_ 1 struct drm_device *drm_dev 1 struct drm_device*drm_dev 1 struct drm_device *pdev 1 struct drm_device *rdev 1 struct drm_device *vdev 2 struct drm_device *dcss_drv_dev_to_drm 2 struct drm_device **ddev 2 struct drm_device *drm_dev_alloc 2 struct drm_device *mock 2 struct drm_device *p_ddev 5 struct drm_device *device 9 struct drm_device * dev 25 struct drm_device *d 95 struct drm_device * 216 struct drm_device *ddev 234 struct drm_device *drm_dev 611 struct drm_device *drm 4190 struct drm_device *dev This series starts with renaming struct drm_crtc::dev to drm_dev. If it's not only me and others like the result of this effort it should be followed up by adapting the other structs and the individual usages in the different drivers. To make this series a bit easier handleable, I first added an alias for drm_crtc::dev, then converted the drivers one after another and the last patch drops the "dev" name. This has the advantage of being easier to review, and if I should have missed an instance only the last patch must be dropped/reverted. Also this series might conflict with other patches, in this case the remaining patches can still go in (apart from the last one of course). Maybe it also makes sense to delay applying the last patch by one development cycle? The series was compile tested for arm, arm64, powerpc and amd64 using an allmodconfig (though I only build drivers/gpu/). Best regards Uwe Uwe Kleine-König (52): drm/crtc: Start renaming struct drm_crtc::dev to drm_dev drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/armada: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/aspeed: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/exynos: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/gma500: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/hyperv: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/ingenic: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/logicvc: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mediatek: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/meson: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mgag200: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/nouveau: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/omapdrm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/panel-ili9341: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/pl111: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/qxl: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/radeon: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/renesas: Use struct drm_crtc::drm_dev instead of struct
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
> Hello Jani,
>
> On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
>> On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
>> > Hello,
>> >
>> > while I debugged an issue in the imx-lcdc driver I was constantly
>> > irritated about struct drm_device pointer variables being named "dev"
>> > because with that name I usually expect a struct device pointer.
>> >
>> > I think there is a big benefit when these are all renamed to "drm_dev".
>> > I have no strong preference here though, so "drmdev" or "drm" are fine
>> > for me, too. Let the bikesheding begin!
>> >
>> > Some statistics:
>> >
>> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq
>> > -c | sort -n
>> > 1 struct drm_device *adev_to_drm
>> > 1 struct drm_device *drm_
>> > 1 struct drm_device *drm_dev
>> > 1 struct drm_device*drm_dev
>> > 1 struct drm_device *pdev
>> > 1 struct drm_device *rdev
>> > 1 struct drm_device *vdev
>> > 2 struct drm_device *dcss_drv_dev_to_drm
>> > 2 struct drm_device **ddev
>> > 2 struct drm_device *drm_dev_alloc
>> > 2 struct drm_device *mock
>> > 2 struct drm_device *p_ddev
>> > 5 struct drm_device *device
>> > 9 struct drm_device * dev
>> > 25 struct drm_device *d
>> > 95 struct drm_device *
>> > 216 struct drm_device *ddev
>> > 234 struct drm_device *drm_dev
>> > 611 struct drm_device *drm
>> >4190 struct drm_device *dev
>> >
>> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
>> > it's not only me and others like the result of this effort it should be
>> > followed up by adapting the other structs and the individual usages in
>> > the different drivers.
>>
>> I think this is an unnecessary change. In drm, a dev is usually a drm
>> device, i.e. struct drm_device *.
>
> Well, unless it's not. Prominently there is
>
> struct drm_device {
> ...
> struct device *dev;
> ...
> };
>
> which yields quite a few code locations using dev->dev which is
> IMHO unnecessary irritating:
>
> $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> 1633
>
> Also the functions that deal with both a struct device and a struct
> drm_device often use "dev" for the struct device and then "ddev" for
> the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
Why is specifically struct drm_device *dev so irritating to you?
You lead us to believe it's an outlier in kernel, something that goes
against common kernel style, but it's really not:
$ git grep -how "struct [A-Za-z0-9_]\+ \*dev" | sort | uniq -c | sort -rn |
head -20
38494 struct device *dev
16388 struct net_device *dev
4184 struct drm_device *dev
2780 struct pci_dev *dev
1916 struct comedi_device *dev
1510 struct mlx5_core_dev *dev
1057 struct mlx4_dev *dev
894 struct b43_wldev *dev
762 struct input_dev *dev
623 struct usbnet *dev
561 struct mlx5_ib_dev *dev
525 struct mt76_dev *dev
465 struct mt76x02_dev *dev
435 struct platform_device *dev
431 struct usb_device *dev
411 struct mt7915_dev *dev
398 struct cx231xx *dev
378 struct mei_device *dev
363 struct ksz_device *dev
359 struct mthca_dev *dev
A good portion of the above also have a dev member.
Are you planning on changing all of the above too, or are you only
annoyed by drm?
I'm really not convinced at all.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Jani,
On Thu, Jul 13, 2023 at 11:03 AM Jani Nikula wrote:
> On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
> > On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
> >> On Wed, 12 Jul 2023, Uwe Kleine-König
> >> wrote:
> >> > while I debugged an issue in the imx-lcdc driver I was constantly
> >> > irritated about struct drm_device pointer variables being named "dev"
> >> > because with that name I usually expect a struct device pointer.
> >> >
> >> > I think there is a big benefit when these are all renamed to "drm_dev".
> >> > I have no strong preference here though, so "drmdev" or "drm" are fine
> >> > for me, too. Let the bikesheding begin!
> >> >
> >> > Some statistics:
> >> >
> >> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort |
> >> > uniq -c | sort -n
> >> > 1 struct drm_device *adev_to_drm
> >> > 1 struct drm_device *drm_
> >> > 1 struct drm_device *drm_dev
> >> > 1 struct drm_device*drm_dev
> >> > 1 struct drm_device *pdev
> >> > 1 struct drm_device *rdev
> >> > 1 struct drm_device *vdev
> >> > 2 struct drm_device *dcss_drv_dev_to_drm
> >> > 2 struct drm_device **ddev
> >> > 2 struct drm_device *drm_dev_alloc
> >> > 2 struct drm_device *mock
> >> > 2 struct drm_device *p_ddev
> >> > 5 struct drm_device *device
> >> > 9 struct drm_device * dev
> >> > 25 struct drm_device *d
> >> > 95 struct drm_device *
> >> > 216 struct drm_device *ddev
> >> > 234 struct drm_device *drm_dev
> >> > 611 struct drm_device *drm
> >> >4190 struct drm_device *dev
> >> >
> >> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> >> > it's not only me and others like the result of this effort it should be
> >> > followed up by adapting the other structs and the individual usages in
> >> > the different drivers.
> >>
> >> I think this is an unnecessary change. In drm, a dev is usually a drm
> >> device, i.e. struct drm_device *.
> >
> > Well, unless it's not. Prominently there is
> >
> > struct drm_device {
> > ...
> > struct device *dev;
> > ...
> > };
> >
> > which yields quite a few code locations using dev->dev which is
> > IMHO unnecessary irritating:
> >
> > $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> > 1633
> >
> > Also the functions that deal with both a struct device and a struct
> > drm_device often use "dev" for the struct device and then "ddev" for
> > the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
>
> Why is specifically struct drm_device *dev so irritating to you?
>
> You lead us to believe it's an outlier in kernel, something that goes
> against common kernel style, but it's really not:
>
> $ git grep -how "struct [A-Za-z0-9_]\+ \*dev" | sort | uniq -c | sort -rn |
> head -20
> 38494 struct device *dev
> 16388 struct net_device *dev
>4184 struct drm_device *dev
>2780 struct pci_dev *dev
>1916 struct comedi_device *dev
>1510 struct mlx5_core_dev *dev
>1057 struct mlx4_dev *dev
> 894 struct b43_wldev *dev
> 762 struct input_dev *dev
> 623 struct usbnet *dev
> 561 struct mlx5_ib_dev *dev
> 525 struct mt76_dev *dev
> 465 struct mt76x02_dev *dev
> 435 struct platform_device *dev
> 431 struct usb_device *dev
> 411 struct mt7915_dev *dev
> 398 struct cx231xx *dev
> 378 struct mei_device *dev
> 363 struct ksz_device *dev
> 359 struct mthca_dev *dev
>
> A good portion of the above also have a dev member.
Not all of them access both the foo_device and the device pointers.
Let's put the number of 435 platform_device pointers named "dev"
into perspective:
10095 struct platform_device *pdev
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Thu, Jul 13, 2023 at 08:52:12AM +0200, Geert Uytterhoeven wrote:
> Hi Uwe,
>
> Let's add some fuel to keep the thread alive ;-)
>
> On Wed, Jul 12, 2023 at 6:13 PM Uwe Kleine-König
> wrote:
> > On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
> > > I think this is an unnecessary change. In drm, a dev is usually a drm
> > > device, i.e. struct drm_device *.
> >
> > Well, unless it's not. Prominently there is
> >
> > struct drm_device {
> > ...
> > struct device *dev;
> > ...
> > };
> >
> > which yields quite a few code locations using dev->dev which is
> > IMHO unnecessary irritating:
> >
> > $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> > 1633
>
> I find that irritating as well...
>
> Same for e.g. crtc->crtc.
>
> Hence that's why I had sent patches to rename the base members in the
> shmob_drm-specific subclasses of drm_{crtc,connector,plane} to "base".
> https://lore.kernel.org/dri-devel/b3daca80f82625ba14e3aeaf2fca6dcefa056e47.1687423204.git.geert+rene...@glider.be
>
> > Also the functions that deal with both a struct device and a struct
> > drm_device often use "dev" for the struct device and then "ddev" for
> > the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
>
> I guess you considered "drm_dev", because it is still a short name?
I considered drm_dev because it is still moderately short and a good
approximation of "drm_device". Other than that the main driving force to
pick "drm_dev" was that it's unique enough that I could have done
s/\/$nameofchoice/ on the initial patch and get it mostly
right.
> Code dealing with platform devices usually uses "pdev" and "dev".
> Same for PCI drivers (despite "pci_dev" being a short name).
pci_dev and platform_device both typlically using pdev already annoyed
me in the past. However less than drm_device *dev because for pci_dev +
platform_device there is little overlap.
> So my personal preference goes to "ddev".
I sticked to "drm" for the new series. I think this provides less fuel.
Best regards and thanks for your thoughts,
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Am 12.07.23 um 20:31 schrieb Sean Paul: On Wed, Jul 12, 2023 at 10:52 AM Jani Nikula wrote: On Wed, 12 Jul 2023, Uwe Kleine-König wrote: Hello, while I debugged an issue in the imx-lcdc driver I was constantly irritated about struct drm_device pointer variables being named "dev" because with that name I usually expect a struct device pointer. I think there is a big benefit when these are all renamed to "drm_dev". I have no strong preference here though, so "drmdev" or "drm" are fine for me, too. Let the bikesheding begin! Some statistics: $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | sort -n 1 struct drm_device *adev_to_drm 1 struct drm_device *drm_ 1 struct drm_device *drm_dev 1 struct drm_device*drm_dev 1 struct drm_device *pdev 1 struct drm_device *rdev 1 struct drm_device *vdev 2 struct drm_device *dcss_drv_dev_to_drm 2 struct drm_device **ddev 2 struct drm_device *drm_dev_alloc 2 struct drm_device *mock 2 struct drm_device *p_ddev 5 struct drm_device *device 9 struct drm_device * dev 25 struct drm_device *d 95 struct drm_device * 216 struct drm_device *ddev 234 struct drm_device *drm_dev 611 struct drm_device *drm 4190 struct drm_device *dev This series starts with renaming struct drm_crtc::dev to drm_dev. If it's not only me and others like the result of this effort it should be followed up by adapting the other structs and the individual usages in the different drivers. I think this is an unnecessary change. In drm, a dev is usually a drm device, i.e. struct drm_device *. As shown by the numbers above. I'd really prefer this patch (series or single) is not accepted. This will cause problems for everyone cherry-picking patches to a downstream kernel (LTS or distro tree). I usually wouldn't expect sympathy here, but the questionable benefit does not outweigh the cost IM[biased]O. I agree. Sean If folks insist on following through with this anyway, I'm firmly in the camp the name should be "drm" and nothing else. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg) OpenPGP_signature Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi
Am 12.07.23 um 18:10 schrieb Uwe Kleine-König:
Hello Jani,
On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
Hello,
while I debugged an issue in the imx-lcdc driver I was constantly
irritated about struct drm_device pointer variables being named "dev"
because with that name I usually expect a struct device pointer.
I think there is a big benefit when these are all renamed to "drm_dev".
I have no strong preference here though, so "drmdev" or "drm" are fine
for me, too. Let the bikesheding begin!
Some statistics:
$ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c |
sort -n
1 struct drm_device *adev_to_drm
1 struct drm_device *drm_
1 struct drm_device *drm_dev
1 struct drm_device*drm_dev
1 struct drm_device *pdev
1 struct drm_device *rdev
1 struct drm_device *vdev
2 struct drm_device *dcss_drv_dev_to_drm
2 struct drm_device **ddev
2 struct drm_device *drm_dev_alloc
2 struct drm_device *mock
2 struct drm_device *p_ddev
5 struct drm_device *device
9 struct drm_device * dev
25 struct drm_device *d
95 struct drm_device *
216 struct drm_device *ddev
234 struct drm_device *drm_dev
611 struct drm_device *drm
4190 struct drm_device *dev
This series starts with renaming struct drm_crtc::dev to drm_dev. If
it's not only me and others like the result of this effort it should be
followed up by adapting the other structs and the individual usages in
the different drivers.
I think this is an unnecessary change. In drm, a dev is usually a drm
device, i.e. struct drm_device *.
Well, unless it's not. Prominently there is
struct drm_device {
...
struct device *dev;
...
};
Jani's point is that it's only inconvenient at the first time. Everyone
gets use to it.
Best regards
Thomas
which yields quite a few code locations using dev->dev which is
IMHO unnecessary irritating:
$ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
1633
Also the functions that deal with both a struct device and a struct
drm_device often use "dev" for the struct device and then "ddev" for
the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
If folks insist on following through with this anyway, I'm firmly in the
camp the name should be "drm" and nothing else.
Up to now positive feedback is in the majority.
Best regards
Uwe
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
OpenPGP_signature
Description: OpenPGP digital signature
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Thu, Jul 13, 2023 at 12:03:05PM +0300, Jani Nikula wrote:
> On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
> > Hello Jani,
> >
> > On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
> >> On Wed, 12 Jul 2023, Uwe Kleine-König
> >> wrote:
> >> > Hello,
> >> >
> >> > while I debugged an issue in the imx-lcdc driver I was constantly
> >> > irritated about struct drm_device pointer variables being named "dev"
> >> > because with that name I usually expect a struct device pointer.
> >> >
> >> > I think there is a big benefit when these are all renamed to "drm_dev".
> >> > I have no strong preference here though, so "drmdev" or "drm" are fine
> >> > for me, too. Let the bikesheding begin!
> >> >
> >> > Some statistics:
> >> >
> >> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort |
> >> > uniq -c | sort -n
> >> > 1 struct drm_device *adev_to_drm
> >> > 1 struct drm_device *drm_
> >> > 1 struct drm_device *drm_dev
> >> > 1 struct drm_device*drm_dev
> >> > 1 struct drm_device *pdev
> >> > 1 struct drm_device *rdev
> >> > 1 struct drm_device *vdev
> >> > 2 struct drm_device *dcss_drv_dev_to_drm
> >> > 2 struct drm_device **ddev
> >> > 2 struct drm_device *drm_dev_alloc
> >> > 2 struct drm_device *mock
> >> > 2 struct drm_device *p_ddev
> >> > 5 struct drm_device *device
> >> > 9 struct drm_device * dev
> >> > 25 struct drm_device *d
> >> > 95 struct drm_device *
> >> > 216 struct drm_device *ddev
> >> > 234 struct drm_device *drm_dev
> >> > 611 struct drm_device *drm
> >> >4190 struct drm_device *dev
> >> >
> >> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> >> > it's not only me and others like the result of this effort it should be
> >> > followed up by adapting the other structs and the individual usages in
> >> > the different drivers.
> >>
> >> I think this is an unnecessary change. In drm, a dev is usually a drm
> >> device, i.e. struct drm_device *.
> >
> > Well, unless it's not. Prominently there is
> >
> > struct drm_device {
> > ...
> > struct device *dev;
> > ...
> > };
> >
> > which yields quite a few code locations using dev->dev which is
> > IMHO unnecessary irritating:
> >
> > $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> > 1633
> >
> > Also the functions that deal with both a struct device and a struct
> > drm_device often use "dev" for the struct device and then "ddev" for
> > the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
>
> Why is specifically struct drm_device *dev so irritating to you?
>
> You lead us to believe it's an outlier in kernel, something that goes
> against common kernel style, but it's really not:
>
> $ git grep -how "struct [A-Za-z0-9_]\+ \*dev" | sort | uniq -c | sort -rn |
> head -20
> 38494 struct device *dev
> 16388 struct net_device *dev
>4184 struct drm_device *dev
>2780 struct pci_dev *dev
>1916 struct comedi_device *dev
>1510 struct mlx5_core_dev *dev
>1057 struct mlx4_dev *dev
> 894 struct b43_wldev *dev
> 762 struct input_dev *dev
> 623 struct usbnet *dev
> 561 struct mlx5_ib_dev *dev
> 525 struct mt76_dev *dev
> 465 struct mt76x02_dev *dev
> 435 struct platform_device *dev
> 431 struct usb_device *dev
> 411 struct mt7915_dev *dev
> 398 struct cx231xx *dev
> 378 struct mei_device *dev
> 363 struct ksz_device *dev
> 359 struct mthca_dev *dev
>
> A good portion of the above also have a dev member.
Yeah, other subsystems and drivers have the same problem. You're lucky
that I noticed drm first and invested some effort to improve it. IMHO
other subsystems being bad shouldn't stop drm to improve here.
And note that for example for pci_dev there are 5794 instances that are
named "pdev" and there are 9971 struct platform_device that are called
"pdev", too. So the majority for these does it better. And agreed,
net_device and others are also inconsistent. If you want an area that is
better, look at the naming of i2c_client or spi_device. (And take into
account that these are spread all over the tree and so are not in
control of a single maintainer team.)
> Are you planning on changing all of the above too, or are you only
> annoyed by drm?
Would you be more welcoming if I promised to tackle some of the above,
too? If so: I might. I hesitate a bit because I didn't suffer from the
others. (Apart from asking ctags for "dev" is a nightmare.)
And regarding the second part of your question: I was annoyed by drm
because that was the one that offended me while researching a problem in
a drm driver. And the variable/struct member name irritated me enough to
believe that with consistent naming I would have found it quicker.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Uwe,
Let's add some fuel to keep the thread alive ;-)
On Wed, Jul 12, 2023 at 6:13 PM Uwe Kleine-König
wrote:
> On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
> > On Wed, 12 Jul 2023, Uwe Kleine-König
> > wrote:
> > > Hello,
> > >
> > > while I debugged an issue in the imx-lcdc driver I was constantly
> > > irritated about struct drm_device pointer variables being named "dev"
> > > because with that name I usually expect a struct device pointer.
> > >
> > > I think there is a big benefit when these are all renamed to "drm_dev".
> > > I have no strong preference here though, so "drmdev" or "drm" are fine
> > > for me, too. Let the bikesheding begin!
> > >
> > > Some statistics:
> > >
> > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq
> > > -c | sort -n
> > > 1 struct drm_device *adev_to_drm
> > > 1 struct drm_device *drm_
> > > 1 struct drm_device *drm_dev
> > > 1 struct drm_device*drm_dev
> > > 1 struct drm_device *pdev
> > > 1 struct drm_device *rdev
> > > 1 struct drm_device *vdev
> > > 2 struct drm_device *dcss_drv_dev_to_drm
> > > 2 struct drm_device **ddev
> > > 2 struct drm_device *drm_dev_alloc
> > > 2 struct drm_device *mock
> > > 2 struct drm_device *p_ddev
> > > 5 struct drm_device *device
> > > 9 struct drm_device * dev
> > > 25 struct drm_device *d
> > > 95 struct drm_device *
> > > 216 struct drm_device *ddev
> > > 234 struct drm_device *drm_dev
> > > 611 struct drm_device *drm
> > >4190 struct drm_device *dev
> > >
> > > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> > > it's not only me and others like the result of this effort it should be
> > > followed up by adapting the other structs and the individual usages in
> > > the different drivers.
> >
> > I think this is an unnecessary change. In drm, a dev is usually a drm
> > device, i.e. struct drm_device *.
>
> Well, unless it's not. Prominently there is
>
> struct drm_device {
> ...
> struct device *dev;
> ...
> };
>
> which yields quite a few code locations using dev->dev which is
> IMHO unnecessary irritating:
>
> $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> 1633
I find that irritating as well...
Same for e.g. crtc->crtc.
Hence that's why I had sent patches to rename the base members in the
shmob_drm-specific subclasses of drm_{crtc,connector,plane} to "base".
https://lore.kernel.org/dri-devel/b3daca80f82625ba14e3aeaf2fca6dcefa056e47.1687423204.git.geert+rene...@glider.be
> Also the functions that deal with both a struct device and a struct
> drm_device often use "dev" for the struct device and then "ddev" for
> the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
I guess you considered "drm_dev", because it is still a short name?
Code dealing with platform devices usually uses "pdev" and "dev".
Same for PCI drivers (despite "pci_dev" being a short name).
So my personal preference goes to "ddev".
EOF (End-of-Fuel ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [Freedreno] [PATCH v2 3/3] drm/komeda: Fix IS_ERR() vs NULL check in komeda_component_get_avail_scaler()
Thanks for taking time to review this patch.
Maybe we can submit another separate patch to fix ERR_CAST(st), because I'm not
sure which commit should be used as a fixes-tag.
Also, Maybe we should fix ERR_CAST(st) in
komeda_pipeline_get_state_and_set_crtc()
and komeda_component_get_state_and_set_user(), please make another separate
patch.
Let me know if there's anything I can do, thanks for your work again!
Gaosheng,
On 2023/7/13 16:54, Liviu Dudau wrote:
Hello,
On Thu, Jul 13, 2023 at 10:05:56AM +0800, Gaosheng Cui wrote:
The komeda_pipeline_get_state() returns an ERR_PTR() on failure, we should
use IS_ERR() to check the return value.
While reviewing the change I've realised that
komeda_pipeline_get_state_and_set_crtc()
is also mishandling the return value from komeda_pipeline_get_state(). If
IS_ERR(st) is
true it should use return ERR_CAST(st), following the same pattern as
komeda_pipeline_get_state().
If you don't want to update this patch I can send a separate patch. Otherwise,
the change looks good to me.
Reviewed-by: Liviu Dudau
Best regards,
Liviu
Fixes: 502932a03fce ("drm/komeda: Add the initial scaler support for CORE")
Signed-off-by: Gaosheng Cui
---
drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index 3276a3e82c62..e9c92439398d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -259,7 +259,7 @@ komeda_component_get_avail_scaler(struct komeda_component
*c,
u32 avail_scalers;
pipe_st = komeda_pipeline_get_state(c->pipeline, state);
- if (!pipe_st)
+ if (IS_ERR(pipe_st))
return NULL;
avail_scalers = (pipe_st->active_comps & KOMEDA_PIPELINE_SCALERS) ^
--
2.25.1
Re: [Freedreno] [PATCH v2 8/8] arm64: dts: qcom: sm8450: provide MDSS cfg interconnect
On Thu, 13 Jul 2023 at 11:41, Konrad Dybcio wrote:
>
> On 12.07.2023 14:11, Dmitry Baryshkov wrote:
> > Add support for the MDSS cfg-cpu bus vote on the SM8450 platform.
> >
> > Signed-off-by: Dmitry Baryshkov
> > ---
> > arch/arm64/boot/dts/qcom/sm8450.dtsi | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > index 595533aeafc4..0b01f3027ee3 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > @@ -13,6 +13,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -2672,8 +2673,12 @@ mdss: display-subsystem@ae0 {
> >
> > /* same path used twice */
> > interconnects = <_noc MASTER_MDP_DISP 0 _virt
> > SLAVE_EBI1_DISP 0>,
> > - <_noc MASTER_MDP_DISP 0 _virt
> > SLAVE_EBI1_DISP 0>;
> > - interconnect-names = "mdp0-mem", "mdp1-mem";
> > + <_noc MASTER_MDP_DISP 0 _virt
> > SLAVE_EBI1_DISP 0>,
> > + <_noc MASTER_APPSS_PROC
> > QCOM_ICC_TAG_ACTIVE_ONLY
> > + _noc SLAVE_DISPLAY_CFG
> > QCOM_ICC_TAG_ACTIVE_ONLY>;
> Looking at icc_set_tag occurences in msm-5.10/techpack/display,
> I *think* active-only is only possible for the data bus (MDP-EBI)
Here I followed the vendor mdss fbdev driver (mdss_mdp.c), which
explicitly states:
static struct msm_bus_scale_pdata mdp_reg_bus_scale_table = {
.usecase = mdp_reg_bus_usecases,
.num_usecases = ARRAY_SIZE(mdp_reg_bus_usecases),
.name = "mdss_reg",
.active_only = true,
};
>
> Moreover, I think Linux is supposed to cast MDSS votes through the
> APPS RSC (so, nodes without _DISP [1][2]) and conversely, DISP_RSC is
> supposed to active-only votes
We can change this once your DISP_RSC lands. Anyway, I think we will
have to add the LLCC-MEM vote at some point later.
>
> Konrad
>
> [1] not that it matters today because it's not implemented yet
> [2]
> https://lore.kernel.org/linux-arm-msm/20230708-topic-rpmh_icc_rsc-v1-0-b223bd2ac...@linaro.org
>
> > + interconnect-names = "mdp0-mem",
> > + "mdp1-mem",
> > + "cpu-cfg";
> >
> > resets = < DISP_CC_MDSS_CORE_BCR>;
> >
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v2 3/3] drm/komeda: Fix IS_ERR() vs NULL check in komeda_component_get_avail_scaler()
Hello,
On Thu, Jul 13, 2023 at 10:05:56AM +0800, Gaosheng Cui wrote:
> The komeda_pipeline_get_state() returns an ERR_PTR() on failure, we should
> use IS_ERR() to check the return value.
While reviewing the change I've realised that
komeda_pipeline_get_state_and_set_crtc()
is also mishandling the return value from komeda_pipeline_get_state(). If
IS_ERR(st) is
true it should use return ERR_CAST(st), following the same pattern as
komeda_pipeline_get_state().
If you don't want to update this patch I can send a separate patch. Otherwise,
the change looks good to me.
Reviewed-by: Liviu Dudau
Best regards,
Liviu
>
> Fixes: 502932a03fce ("drm/komeda: Add the initial scaler support for CORE")
> Signed-off-by: Gaosheng Cui
> ---
> drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index 3276a3e82c62..e9c92439398d 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -259,7 +259,7 @@ komeda_component_get_avail_scaler(struct komeda_component
> *c,
> u32 avail_scalers;
>
> pipe_st = komeda_pipeline_get_state(c->pipeline, state);
> - if (!pipe_st)
> + if (IS_ERR(pipe_st))
> return NULL;
>
> avail_scalers = (pipe_st->active_comps & KOMEDA_PIPELINE_SCALERS) ^
> --
> 2.25.1
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ツ)_/¯
Re: [Freedreno] [PATCH v2 8/8] arm64: dts: qcom: sm8450: provide MDSS cfg interconnect
On 12.07.2023 14:11, Dmitry Baryshkov wrote:
> Add support for the MDSS cfg-cpu bus vote on the SM8450 platform.
>
> Signed-off-by: Dmitry Baryshkov
> ---
> arch/arm64/boot/dts/qcom/sm8450.dtsi | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 595533aeafc4..0b01f3027ee3 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -2672,8 +2673,12 @@ mdss: display-subsystem@ae0 {
>
> /* same path used twice */
> interconnects = <_noc MASTER_MDP_DISP 0 _virt
> SLAVE_EBI1_DISP 0>,
> - <_noc MASTER_MDP_DISP 0 _virt
> SLAVE_EBI1_DISP 0>;
> - interconnect-names = "mdp0-mem", "mdp1-mem";
> + <_noc MASTER_MDP_DISP 0 _virt
> SLAVE_EBI1_DISP 0>,
> + <_noc MASTER_APPSS_PROC
> QCOM_ICC_TAG_ACTIVE_ONLY
> + _noc SLAVE_DISPLAY_CFG
> QCOM_ICC_TAG_ACTIVE_ONLY>;
Looking at icc_set_tag occurences in msm-5.10/techpack/display,
I *think* active-only is only possible for the data bus (MDP-EBI)
Moreover, I think Linux is supposed to cast MDSS votes through the
APPS RSC (so, nodes without _DISP [1][2]) and conversely, DISP_RSC is
supposed to active-only votes
Konrad
[1] not that it matters today because it's not implemented yet
[2]
https://lore.kernel.org/linux-arm-msm/20230708-topic-rpmh_icc_rsc-v1-0-b223bd2ac...@linaro.org
> + interconnect-names = "mdp0-mem",
> + "mdp1-mem",
> + "cpu-cfg";
>
> resets = < DISP_CC_MDSS_CORE_BCR>;
>
