Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: Drop unused members from HW structs
On 21/04/2023 00:39, Marijn Suijten wrote:
On 2023-04-20 08:46:46, Abhinav Kumar wrote:
On 4/20/2023 7:33 AM, Dmitry Baryshkov wrote:
On 18/04/2023 02:14, Marijn Suijten wrote:
Some of these members were initialized while never read, while others
were not even assigned any value at all. Drop them to save some space,
and above all confusion when looking at these members.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Fixes: 84a33d0fd921 ("drm/msm/dpu: add dpu_hw_wb abstraction for
writeback blocks")
The fixes headers are slightly questionable, as unused fields are not a
bug. Nevertheless:
Yes, I would also not treat this as a "fix" but just cleanup.
Ack to both. This seems like a fine line to me as the fields are not
contributing to anything except confusion. Specifically hw_mdp which is
never initialized and may accidentally be used without realizing that
it'll stay NULL, but that is again up to the developer using the field
at that point.
Feel free to drop them while applying, or should I reword the message to
at least still link these commits to mention the origin of the unused
fields?
I'm fine with having the Fixes removed. If you can send the v2 reordered
& with tags removed, that would be the best.
- Marijn
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: Drop unused members from HW structs
On 2023-04-20 08:46:46, Abhinav Kumar wrote:
>
>
> On 4/20/2023 7:33 AM, Dmitry Baryshkov wrote:
> > On 18/04/2023 02:14, Marijn Suijten wrote:
> >> Some of these members were initialized while never read, while others
> >> were not even assigned any value at all. Drop them to save some space,
> >> and above all confusion when looking at these members.
> >>
> >> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> >> Fixes: 84a33d0fd921 ("drm/msm/dpu: add dpu_hw_wb abstraction for
> >> writeback blocks")
> >
> > The fixes headers are slightly questionable, as unused fields are not a
> > bug. Nevertheless:
> >
>
> Yes, I would also not treat this as a "fix" but just cleanup.
Ack to both. This seems like a fine line to me as the fields are not
contributing to anything except confusion. Specifically hw_mdp which is
never initialized and may accidentally be used without realizing that
it'll stay NULL, but that is again up to the developer using the field
at that point.
Feel free to drop them while applying, or should I reword the message to
at least still link these commits to mention the origin of the unused
fields?
- Marijn
Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: Drop unused members from HW structs
On 4/20/2023 7:33 AM, Dmitry Baryshkov wrote:
On 18/04/2023 02:14, Marijn Suijten wrote:
Some of these members were initialized while never read, while others
were not even assigned any value at all. Drop them to save some space,
and above all confusion when looking at these members.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Fixes: 84a33d0fd921 ("drm/msm/dpu: add dpu_hw_wb abstraction for
writeback blocks")
The fixes headers are slightly questionable, as unused fields are not a
bug. Nevertheless:
Yes, I would also not treat this as a "fix" but just cleanup.
I think if this series is slightly re-ordered to keep the second patch
before this one, I can pick that one up for -fixes and then this one and
the third can be left for 6.5
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 2 --
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 5 -
6 files changed, 11 deletions(-)
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 84ee2efa9c66..a9c90249a6ac 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -374,7 +374,6 @@ struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf
idx,
*/
c->idx = idx;
c->cap = cfg;
- c->mdss = m;
_setup_intf_ops(>ops, c->cap->features);
return c;
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 643dd10bc030..e07b2e33af3e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -90,7 +90,6 @@ struct dpu_hw_intf {
/* intf */
enum dpu_intf idx;
const struct dpu_intf_cfg *cap;
- const struct dpu_mdss_cfg *mdss;
/* ops */
struct dpu_hw_intf_ops ops;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
index cf70a9bd1034..bb3ddec5c7d7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -812,7 +812,6 @@ struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp
idx,
}
/* Assign ops */
- hw_pipe->catalog = catalog;
hw_pipe->ubwc = catalog->ubwc;
hw_pipe->idx = idx;
hw_pipe->cap = cfg;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
index 74b98b6b3bc3..5004a02fd61e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -341,7 +341,6 @@ struct dpu_hw_sspp_ops {
* struct dpu_hw_sspp - pipe description
* @base: hardware block base structure
* @hw: block hardware details
- * @catalog: back pointer to catalog
* @ubwc: ubwc configuration data
* @idx: pipe index
* @cap: pointer to layer_cfg
@@ -350,7 +349,6 @@ struct dpu_hw_sspp_ops {
struct dpu_hw_sspp {
struct dpu_hw_blk base;
struct dpu_hw_blk_reg_map hw;
- const struct dpu_mdss_cfg *catalog;
const struct dpu_ubwc_cfg *ubwc;
/* Pipe */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
index 2d28afdf860e..db5e6040017f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
@@ -262,7 +262,6 @@ struct dpu_hw_wb *dpu_hw_wb_init(enum dpu_wb idx,
}
/* Assign ops */
- c->mdp = >mdp[0];
c->idx = idx;
c->caps = cfg;
_setup_wb_ops(>ops, c->caps->features);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
index 3ff5a48541e2..b91923f879f1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
@@ -76,15 +76,12 @@ struct dpu_hw_wb_ops {
/**
* struct dpu_hw_wb : WB driver object
* @hw: block hardware details
- * @mdp: pointer to associated mdp portion of the catalog
* @idx: hardware index number within type
* @wb_hw_caps: hardware capabilities
* @ops: function pointers
- * @hw_mdp: MDP top level hardware block
*/
struct dpu_hw_wb {
struct dpu_hw_blk_reg_map hw;
- const struct dpu_mdp_cfg *mdp;
/* wb path */
int idx;
@@ -92,8 +89,6 @@ struct dpu_hw_wb {
/* ops */
struct dpu_hw_wb_ops ops;
-
- struct dpu_hw_mdp *hw_mdp;
};
/**
Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: Drop unused members from HW structs
On 18/04/2023 02:14, Marijn Suijten wrote:
Some of these members were initialized while never read, while others
were not even assigned any value at all. Drop them to save some space,
and above all confusion when looking at these members.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Fixes: 84a33d0fd921 ("drm/msm/dpu: add dpu_hw_wb abstraction for writeback
blocks")
The fixes headers are slightly questionable, as unused fields are not a
bug. Nevertheless:
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 2 --
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 5 -
6 files changed, 11 deletions(-)
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 84ee2efa9c66..a9c90249a6ac 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -374,7 +374,6 @@ struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx,
*/
c->idx = idx;
c->cap = cfg;
- c->mdss = m;
_setup_intf_ops(>ops, c->cap->features);
return c;
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 643dd10bc030..e07b2e33af3e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -90,7 +90,6 @@ struct dpu_hw_intf {
/* intf */
enum dpu_intf idx;
const struct dpu_intf_cfg *cap;
- const struct dpu_mdss_cfg *mdss;
/* ops */
struct dpu_hw_intf_ops ops;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
index cf70a9bd1034..bb3ddec5c7d7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -812,7 +812,6 @@ struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
}
/* Assign ops */
- hw_pipe->catalog = catalog;
hw_pipe->ubwc = catalog->ubwc;
hw_pipe->idx = idx;
hw_pipe->cap = cfg;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
index 74b98b6b3bc3..5004a02fd61e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -341,7 +341,6 @@ struct dpu_hw_sspp_ops {
* struct dpu_hw_sspp - pipe description
* @base: hardware block base structure
* @hw: block hardware details
- * @catalog: back pointer to catalog
* @ubwc: ubwc configuration data
* @idx: pipe index
* @cap: pointer to layer_cfg
@@ -350,7 +349,6 @@ struct dpu_hw_sspp_ops {
struct dpu_hw_sspp {
struct dpu_hw_blk base;
struct dpu_hw_blk_reg_map hw;
- const struct dpu_mdss_cfg *catalog;
const struct dpu_ubwc_cfg *ubwc;
/* Pipe */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
index 2d28afdf860e..db5e6040017f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
@@ -262,7 +262,6 @@ struct dpu_hw_wb *dpu_hw_wb_init(enum dpu_wb idx,
}
/* Assign ops */
- c->mdp = >mdp[0];
c->idx = idx;
c->caps = cfg;
_setup_wb_ops(>ops, c->caps->features);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
index 3ff5a48541e2..b91923f879f1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
@@ -76,15 +76,12 @@ struct dpu_hw_wb_ops {
/**
* struct dpu_hw_wb : WB driver object
* @hw: block hardware details
- * @mdp: pointer to associated mdp portion of the catalog
* @idx: hardware index number within type
* @wb_hw_caps: hardware capabilities
* @ops: function pointers
- * @hw_mdp: MDP top level hardware block
*/
struct dpu_hw_wb {
struct dpu_hw_blk_reg_map hw;
- const struct dpu_mdp_cfg *mdp;
/* wb path */
int idx;
@@ -92,8 +89,6 @@ struct dpu_hw_wb {
/* ops */
struct dpu_hw_wb_ops ops;
-
- struct dpu_hw_mdp *hw_mdp;
};
/**
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: Drop unused members from HW structs
On 2023-04-17 18:54:18, Abhinav Kumar wrote:
>
> On 4/17/2023 4:14 PM, Marijn Suijten wrote:
> > Some of these members were initialized while never read, while others
> > were not even assigned any value at all. Drop them to save some space,
> > and above all confusion when looking at these members.
> >
> > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> > Fixes: 84a33d0fd921 ("drm/msm/dpu: add dpu_hw_wb abstraction for writeback
> > blocks")
> > Signed-off-by: Marijn Suijten
> > ---
>
> It seems like WB UBWC formats are not supported today. Because otherwise
> ctx->mdp would be used for writeback. I guess we can add a ubwc member
> similar to hw_sspp, when we do add the support for this. Hence this is,
That seems preferable to me. SSPP does the same in patch 3/3, and I
have been thinking to replace the dpu_mdss_cfg *catalog parameter in
dpu_hw_sspp_init with just the ubwc cfg pointer. Likewise
dpu_hw_ctl_init also takes the full dpu_mdss_cfg only to take out the
mixers.
- Marijn
> Reviewed-by: Abhinav Kumar
Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: Drop unused members from HW structs
On 4/17/2023 4:14 PM, Marijn Suijten wrote:
Some of these members were initialized while never read, while others
were not even assigned any value at all. Drop them to save some space,
and above all confusion when looking at these members.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Fixes: 84a33d0fd921 ("drm/msm/dpu: add dpu_hw_wb abstraction for writeback
blocks")
Signed-off-by: Marijn Suijten
---
It seems like WB UBWC formats are not supported today. Because otherwise
ctx->mdp would be used for writeback. I guess we can add a ubwc member
similar to hw_sspp, when we do add the support for this. Hence this is,
Reviewed-by: Abhinav Kumar
[Freedreno] [PATCH 1/3] drm/msm/dpu: Drop unused members from HW structs
Some of these members were initialized while never read, while others
were not even assigned any value at all. Drop them to save some space,
and above all confusion when looking at these members.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Fixes: 84a33d0fd921 ("drm/msm/dpu: add dpu_hw_wb abstraction for writeback
blocks")
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 2 --
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 5 -
6 files changed, 11 deletions(-)
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 84ee2efa9c66..a9c90249a6ac 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -374,7 +374,6 @@ struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx,
*/
c->idx = idx;
c->cap = cfg;
- c->mdss = m;
_setup_intf_ops(>ops, c->cap->features);
return c;
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 643dd10bc030..e07b2e33af3e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -90,7 +90,6 @@ struct dpu_hw_intf {
/* intf */
enum dpu_intf idx;
const struct dpu_intf_cfg *cap;
- const struct dpu_mdss_cfg *mdss;
/* ops */
struct dpu_hw_intf_ops ops;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
index cf70a9bd1034..bb3ddec5c7d7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -812,7 +812,6 @@ struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
}
/* Assign ops */
- hw_pipe->catalog = catalog;
hw_pipe->ubwc = catalog->ubwc;
hw_pipe->idx = idx;
hw_pipe->cap = cfg;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
index 74b98b6b3bc3..5004a02fd61e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -341,7 +341,6 @@ struct dpu_hw_sspp_ops {
* struct dpu_hw_sspp - pipe description
* @base: hardware block base structure
* @hw: block hardware details
- * @catalog: back pointer to catalog
* @ubwc: ubwc configuration data
* @idx: pipe index
* @cap: pointer to layer_cfg
@@ -350,7 +349,6 @@ struct dpu_hw_sspp_ops {
struct dpu_hw_sspp {
struct dpu_hw_blk base;
struct dpu_hw_blk_reg_map hw;
- const struct dpu_mdss_cfg *catalog;
const struct dpu_ubwc_cfg *ubwc;
/* Pipe */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
index 2d28afdf860e..db5e6040017f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
@@ -262,7 +262,6 @@ struct dpu_hw_wb *dpu_hw_wb_init(enum dpu_wb idx,
}
/* Assign ops */
- c->mdp = >mdp[0];
c->idx = idx;
c->caps = cfg;
_setup_wb_ops(>ops, c->caps->features);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
index 3ff5a48541e2..b91923f879f1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
@@ -76,15 +76,12 @@ struct dpu_hw_wb_ops {
/**
* struct dpu_hw_wb : WB driver object
* @hw: block hardware details
- * @mdp: pointer to associated mdp portion of the catalog
* @idx: hardware index number within type
* @wb_hw_caps: hardware capabilities
* @ops: function pointers
- * @hw_mdp: MDP top level hardware block
*/
struct dpu_hw_wb {
struct dpu_hw_blk_reg_map hw;
- const struct dpu_mdp_cfg *mdp;
/* wb path */
int idx;
@@ -92,8 +89,6 @@ struct dpu_hw_wb {
/* ops */
struct dpu_hw_wb_ops ops;
-
- struct dpu_hw_mdp *hw_mdp;
};
/**
--
2.40.0
