Re: [Freedreno] [PATCH v2 04/17] drm/msm/dpu: Fix PP_BLK_DIPHER -> DITHER typo
On 2023-04-24 16:09:45, Abhinav Kumar wrote:
> >> dither block should be present on many other chipsets too but looks like
> >> on sm8550 was enabling it. Not sure how it was validated there. But we
> >> are enabling dither, even other chipsets have this block.
> >
> > Correct, they all seem to have it starting at sdm845. My patch message
> > seems to lack the word "exclusively" as the PP on sm8550 appears to
> > exclusively contain a DITHER subblock (unless other blocks are available
> > that simply aren't supported within this driver yet) and no other
> > registers. Hence this aptly named macro exist to emit just the feature
> > bitflag for that and a .len of zero.
> >
>
> I think after the TE blocks were moved to INTF, dither is the only
> sub-block for all Ping-Pongs not just in sm8550.
So you are asking / leaving context to make all >= 5.0.0 pingpong blocks
use this macro with only a single DITHER sblk in PP?
As far as I recall SM8550 is the first SoC to use zero registers in PP,
which is specifically what this macro takes care of too. Then, there
are only a few SoCs downstream still (erroneously?) referencing TE2 as
the only other sub-blk, those SoCs still use sdm845_pp_sblk_te.
> > Now, whether we should have the features contain subblock flags rather
> > than just scanning for their id's or presence in the subblocks is a
> > different discussion / cleanup we should have.
> >
>
> Yes, separate patch and hence I gave R-b on this one. But had to leave
> this comment to not lose context.
Fwiw this is a different suggestion: we already have these flags in the
sub-block `.id` field so there seems to be no reason to duplicate info
in the top-level `.features` field, deduplicating some info and
simplifying some defines.
- Marijn
> > - Marijn
> >
> >>> - PP_BLK_DIPHER("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0,
> >>> sc7280_pp_sblk,
> >>> + PP_BLK_DITHER("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0,
> >>> sc7280_pp_sblk,
> >>> DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
> >>> -1),
> >>> - PP_BLK_DIPHER("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0,
> >>> sc7280_pp_sblk,
> >>> + PP_BLK_DITHER("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0,
> >>> sc7280_pp_sblk,
> >>> DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
> >>> -1),
> >>> - PP_BLK_DIPHER("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1,
> >>> sc7280_pp_sblk,
> >>> + PP_BLK_DITHER("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1,
> >>> sc7280_pp_sblk,
> >>> DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
> >>> -1),
> >>> - PP_BLK_DIPHER("pingpong_3", PINGPONG_3, 0x6c000, MERGE_3D_1,
> >>> sc7280_pp_sblk,
> >>> + PP_BLK_DITHER("pingpong_3", PINGPONG_3, 0x6c000, MERGE_3D_1,
> >>> sc7280_pp_sblk,
> >>> DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
> >>> -1),
> >>> - PP_BLK_DIPHER("pingpong_4", PINGPONG_4, 0x6d000, MERGE_3D_2,
> >>> sc7280_pp_sblk,
> >>> + PP_BLK_DITHER("pingpong_4", PINGPONG_4, 0x6d000, MERGE_3D_2,
> >>> sc7280_pp_sblk,
> >>> DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 30),
> >>> -1),
> >>> - PP_BLK_DIPHER("pingpong_5", PINGPONG_5, 0x6e000, MERGE_3D_2,
> >>> sc7280_pp_sblk,
> >>> + PP_BLK_DITHER("pingpong_5", PINGPONG_5, 0x6e000, MERGE_3D_2,
> >>> sc7280_pp_sblk,
> >>> DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 31),
> >>> -1),
> >>> - PP_BLK_DIPHER("pingpong_6", PINGPONG_6, 0x66000, MERGE_3D_3,
> >>> sc7280_pp_sblk,
> >>> + PP_BLK_DITHER("pingpong_6", PINGPONG_6, 0x66000, MERGE_3D_3,
> >>> sc7280_pp_sblk,
> >>> -1,
> >>> -1),
> >>> - PP_BLK_DIPHER("pingpong_7", PINGPONG_7, 0x66400, MERGE_3D_3,
> >>> sc7280_pp_sblk,
> >>> + PP_BLK_DITHER("pingpong_7", PINGPONG_7, 0x66400, MERGE_3D_3,
> >>> sc7280_pp_sblk,
> >>> -1,
> >>> -1),
> >>>};
> >>> 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 03f162af1a50..ca8a02debda9 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>> @@ -491,7 +491,7 @@ static const struct dpu_pingpong_sub_blks
> >>> sc7280_pp_sblk = {
> >>> .len = 0x20, .version = 0x2},
> >>>};
> >>>
> >>> -#define PP_BLK_DIPHER(_name, _id, _base, _merge_3d, _sblk, _done,
> >>> _rdptr) \
> >>> +#define PP_BLK_DITHER(_name, _id, _base, _merge_3d, _sblk, _done,
> >>> _rdptr) \
> >>> {\
> >>> .name = _name, .id = _id, \
> >>> .base = _base, .len = 0, \
> >>>
Re: [Freedreno] [PATCH 10/11] drm/msm/dpu: tweak lm pairings in msm8998 hw catalog
On 4/19/2023 7:41 AM, Arnaud Vrac wrote: Change lm blocks pairs so that lm blocks with the same features are paired together: LM_0 and LM_1 with PP and DSPP LM_2 and LM_5 with PP LM_3 and LM_4 This matches the sdm845 configuration and allows using pp or dspp when 2 lm blocks are needed in the topology. In the previous config the reservation code could never find an lm pair without a matching feature set. Signed-off-by: Arnaud Vrac Reviewed-by: Abhinav Kumar
Re: [Freedreno] [PATCH v2 00/17] drm/msm/dpu: Implement tearcheck support on INTF block
On 4/17/2023 1:21 PM, Marijn Suijten wrote:
Since DPU 5.0.0 the TEARCHECK registers and interrupts moved out of the
PINGPONG block and into the INTF. Implement the necessary callbacks in
the INTF block, and use these callbacks together with the INTF_TEAR
interrupts. Additionally, disable previous register writes and remove
unused interrupts in the PINGPONG and MDP_TOP blocks for these newer
platforms.
With these patches the devices on DPU >= 5.0.0 listed below now update
their panels at 60fps without tearing (nor sluggishness), and without
repeated timeouts in dmesg.
Tested on the following devices with command-mode panels and TE pins:
- Sony Xperia XZ3 (sdm845, DPU 4.0.0, cmdmode panel): no regressions on
PINGPONG TE;
- Sony Xperia 5 (sm8150, DPU 5.0.0);
- Sony Xperia 10 II (sm6125, DPU 5.0.4).
---
I will pickup the fixes from this one and for the rest, since jessica
has been rebasing the DSC 1.2 over DSI on top of this series since the
RFC stage, will let her review and test this out and we can pick up the
rest for 6.5
So please keep her CCed on the next revisions of this if there are more.
Changes in v2:
- Rebase on -next with all the new SC8280XP and SM8[345]50 support;
- Remove duplicate PP_BLK_TE macro now that .features is an argument;
- Fix PP_BLK_DIPHER -> DITHER typo that was added recently;
- Add INTF_TEAR interrupt blocks for DPU 7.0.0 (moved to different
register range);
- Describe INTF_TEAR support for the newly added SM8350, SM8450,
SM8550 and SC8280XP SoCs;
- Remove TE2 subblocks from 8[34]50 and sc8280xp (new patch);
- Rebase on -next with DPU catalog rework;
- Remove dpu_hw_intf_v1_get_status which was inlined in the original
dpu_hw_intf_get_status function in e3969eadc8ee ("drm/msm/disp/dpu:
get timing engine status from intf status register");
- Many changes to move all catalog edits to separate files;
- Add documentation for DPU_MDP_VSYNC_SEL;
- Fix sdm8150_mdp typo, should be sm8150_mdp;
- Move unrelated INTF_INTR offsets out of hwio header (new patch);
- Remove _reg argument from INTF_BLK, since we now have a third
interrupt with a different base register. To prevent confusion all
three interrupts should provide the final value from DPU_IRQ_IDX
directly.
- Only request the new tear_rd_ptr in a new INTF_BLK_DSI_TE macro;
- Drop stray INTF_MISR_SIGNATURE register definition;
- Clean up registers in dpu_hw_intf.c (many new patches);
- merged setup_tearcheck() and enable_tearcheck() callbacks;
- replaced enable_tearcheck(false) with new disable_tearcheck()
callback;
- Moved dpu_encoder_phys_cmd_enable_te intestines (just autorefresh
disablement) to INTF and PP block, replacing 3 callbacks in both
blocks with just a single disable_autorefresh() callback.
v1:
https://lore.kernel.org/r/20221231215006.211860-1-marijn.suij...@somainline.org
---
Konrad Dybcio (1):
drm/msm/dpu: Move dpu_hw_{tear_check,pp_vsync_info} to dpu_hw_mdss.h
Marijn Suijten (16):
drm/msm/dpu: Remove unused INTF0 interrupt mask from SM6115/QCM2290
drm/msm/dpu: Remove TE2 block and feature from DPU >= 7.0.0 hardware
drm/msm/dpu: Move non-MDP_TOP INTF_INTR offsets out of hwio header
drm/msm/dpu: Fix PP_BLK_DIPHER -> DITHER typo
drm/msm/dpu: Remove duplicate register defines from INTF
drm/msm/dpu: Remove extraneous register define indentation
drm/msm/dpu: Sort INTF registers numerically
drm/msm/dpu: Drop unused poll_timeout_wr_ptr PINGPONG callback
drm/msm/dpu: Move autorefresh disable from CMD encoder to pingpong
drm/msm/dpu: Disable pingpong TE on DPU 5.0.0 and above
drm/msm/dpu: Disable MDP vsync source selection on DPU 5.0.0 and above
drm/msm/dpu: Factor out shared interrupt register in INTF_BLK macro
drm/msm/dpu: Document and enable TEAR interrupts on DSI interfaces
drm/msm/dpu: Merge setup_- and enable_tearcheck pingpong callbacks
drm/msm/dpu: Implement tearcheck support on INTF block
drm/msm/dpu: Remove intr_rdptr from DPU >= 5.0.0 pingpong config
.../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h| 26 +-
.../gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 26 +-
.../gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 40 +--
.../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h| 48 ++--
.../gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 40 +--
.../gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h | 16 +-
.../gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h | 15 +-
.../drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h| 15 +-
.../gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 40 +--
.../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 22 +-
.../drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 64 +++--
.../gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 46 ++--
.../gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 36 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 11 +
driv
Re: [Freedreno] [PATCH v2 06/17] drm/msm/dpu: Remove extraneous register define indentation
On 4/17/2023 1:21 PM, Marijn Suijten wrote: A bunch of registers are indented with two extra spaces, looking as if these are values corresponding to the previous register which is not the case, rather these are simply also register offsets and should only have a single space separating them and the #define keyword. Signed-off-by: Marijn Suijten --- Reviewed-by: Abhinav Kumar
Re: [Freedreno] [PATCH v2 05/17] drm/msm/dpu: Remove duplicate register defines from INTF
On 4/17/2023 1:21 PM, Marijn Suijten wrote:
The INTF_FRAME_LINE_COUNT_EN, INTF_FRAME_COUNT and INTF_LINE_COUNT
registers are already defined higher up, in the right place when sorted
numerically.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Marijn Suijten
---
Reviewed-by: Abhinav Kumar
Re: [Freedreno] [PATCH v2 03/17] drm/msm/dpu: Move non-MDP_TOP INTF_INTR offsets out of hwio header
On 4/24/2023 3:25 PM, Marijn Suijten wrote:
On 2023-04-24 13:44:55, Abhinav Kumar wrote:
On 4/17/2023 1:21 PM, Marijn Suijten wrote:
These offsets do not fall under the MDP TOP block and do not fit the
comment right above. Move them to dpu_hw_interrupts.c next to the
repsective MDP_INTF_x_OFF interrupt block offsets.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Marijn Suijten
This change itself is fine, hence
Reviewed-by: Abhinav Kumar
One comment below.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 5 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h | 3 ---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index 53326f25e40e..85c0bda3ff90 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -15,7 +15,7 @@
/*
* Register offsets in MDSS register file for the interrupt registers
- * w.r.t. to the MDP base
+ * w.r.t. the MDP base
*/
#define MDP_SSPP_TOP0_OFF0x0
#define MDP_INTF_0_OFF 0x6A000
@@ -24,6 +24,9 @@
#define MDP_INTF_3_OFF 0x6B800
#define MDP_INTF_4_OFF 0x6C000
#define MDP_INTF_5_OFF 0x6C800
+#define INTF_INTR_EN 0x1c0
+#define INTF_INTR_STATUS 0x1c4
+#define INTF_INTR_CLEAR0x1c8
#define MDP_AD4_0_OFF0x7C000
#define MDP_AD4_1_OFF0x7D000
#define MDP_AD4_INTR_EN_OFF 0x41c
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
index feb9a729844a..5acd5683d25a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
@@ -21,9 +21,6 @@
#define HIST_INTR_EN0x01c
#define HIST_INTR_STATUS0x020
#define HIST_INTR_CLEAR 0x024
Even HIST_INTR_*** need to be moved then.
These are relative to MDP_SSPP_TOP0_OFF too just like
INTR(2)_{CLEAR,EN,STATUS} so I left them here. Otherwise, *all* these
interrupt masks are probably best moved to dpu_hw_interrupts.c for
clarity, as that's also the only place they are used?
Let me know which way you prefer.
- Marijn
Ah okay, understood, this is fine then.
-#define INTF_INTR_EN0x1C0
-#define INTF_INTR_STATUS0x1C4
-#define INTF_INTR_CLEAR 0x1C8
#define SPLIT_DISPLAY_EN0x2F4
#define SPLIT_DISPLAY_UPPER_PIPE_CTRL 0x2F8
#define DSPP_IGC_COLOR0_RAM_LUTN0x300
Re: [Freedreno] [PATCH v2 3/3] drm/msm/dpu: Pass catalog pointers directly from RM instead of IDs
On 4/24/2023 3:54 PM, Dmitry Baryshkov wrote:
On Tue, 25 Apr 2023 at 01:03, Marijn Suijten
wrote:
On 2023-04-21 16:25:15, Abhinav Kumar wrote:
On 4/21/2023 1:53 PM, Marijn Suijten wrote:
The Resource Manager already iterates over all available blocks from the
catalog, only to pass their ID to a dpu_hw_xxx_init() function which
uses an _xxx_offset() helper to search for and find the exact same
catalog pointer again to initialize the block with, fallible error
handling and all.
Instead, pass const pointers to the catalog entries directly to these
_init functions and drop the for loops entirely, saving on both
readability complexity and unnecessary cycles at boot.
Signed-off-by: Marijn Suijten
Reviewed-by: Dmitry Baryshkov
Overall, a nice cleanup!
One comment below.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c| 37 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h| 14
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c| 32 +++---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h| 11 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.c | 38 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.h | 12 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 40
++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 12 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 38 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.h | 10 +++---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_merge3d.c| 33 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_merge3d.h| 14
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 33 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h | 14
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 39 --
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 12 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.c | 33 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h | 11 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 33 ---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 11 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 17 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 18 +-
23 files changed, 139 insertions(+), 375 deletions(-)
-struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx,
- void __iomem *addr,
- const struct dpu_mdss_cfg *m)
+struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
+ void __iomem *addr)
{
struct dpu_hw_intf *c;
- const struct dpu_intf_cfg *cfg;
+
+ if (cfg->type == INTF_NONE) {
+ pr_err("Cannot create interface hw object for INTF_NONE type\n");
+ return ERR_PTR(-EINVAL);
+ }
The caller of dpu_hw_intf_init which is the RM already has protection
for INTF_NONE, see below
for (i = 0; i < cat->intf_count; i++) {
struct dpu_hw_intf *hw;
const struct dpu_intf_cfg *intf = &cat->intf[i];
if (intf->type == INTF_NONE) {
DPU_DEBUG("skip intf %d with type none\n", i);
continue;
}
if (intf->id < INTF_0 || intf->id >= INTF_MAX) {
DPU_ERROR("skip intf %d with invalid id\n",
intf->id);
continue;
}
hw = dpu_hw_intf_init(intf->id, mmio, cat);
So this part can be dropped.
I mainly intended to keep original validation where _intf_offset would
skip INTF_NONE, and error out. RM init is hence expected to filter out
INTF_NONE instead of running into that `-EINVAL`, which I maintained
here.
If you think there won't be another caller of dpu_hw_intf_init, and that
such validation is hence excessive, I can remove it in a followup v3.
I'd prefer to see the checks at dpu_rm to be dropped.
dpu_hw_intf_init() (and other dpu_hw_foo_init() functions) should be
self-contained. If they can not init HW block (e.g. because the index
is out of the boundaries), they should return an error.
They already do that today because even without this it will call into
_intf_offset() and that will bail out for INTF_NONE.
I feel this is a duplicated check because the caller with the loop needs
to validate the index before passing it to dpu_hw_intf_init() otherwise
the loop will get broken at the first return of the error and rest of
the blocks will also not be initialized.
- Marijn
c = kzalloc(sizeof(*c), GFP_KERNEL);
if (!c)
return ERR_PTR(-ENOMEM);
- cfg = _intf_offset(idx, m, addr, &c->hw);
- if (IS_ERR_OR_NULL(cfg)) {
- kfree(c);
- pr_err("failed to create dpu_hw_intf %d\n", idx);
- retur
Re: [Freedreno] [PATCH v2 04/17] drm/msm/dpu: Fix PP_BLK_DIPHER -> DITHER typo
On 4/24/2023 3:30 PM, Marijn Suijten wrote:
On 2023-04-24 13:53:13, Abhinav Kumar wrote:
On 4/17/2023 1:21 PM, Marijn Suijten wrote:
SM8550 only comes with a DITHER subblock inside the PINGPONG block,
hence the name and a block length of zero. However, the PP_BLK macro
name was typo'd to DIPHER rather than DITHER.
Fixes: efcd0107727c ("drm/msm/dpu: add support for SM8550")
Signed-off-by: Marijn Suijten
This change itself is fine, hence
Reviewed-by: Abhinav Kumar
one comment below
---
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 16
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
index 9e403034093f..d0ab351b6a8b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
@@ -132,28 +132,28 @@ static const struct dpu_dspp_cfg sm8550_dspp[] = {
&sm8150_dspp_sblk),
};
static const struct dpu_pingpong_cfg sm8550_pp[] = {
dither block should be present on many other chipsets too but looks like
on sm8550 was enabling it. Not sure how it was validated there. But we
are enabling dither, even other chipsets have this block.
Correct, they all seem to have it starting at sdm845. My patch message
seems to lack the word "exclusively" as the PP on sm8550 appears to
exclusively contain a DITHER subblock (unless other blocks are available
that simply aren't supported within this driver yet) and no other
registers. Hence this aptly named macro exist to emit just the feature
bitflag for that and a .len of zero.
I think after the TE blocks were moved to INTF, dither is the only
sub-block for all Ping-Pongs not just in sm8550.
Now, whether we should have the features contain subblock flags rather
than just scanning for their id's or presence in the subblocks is a
different discussion / cleanup we should have.
Yes, separate patch and hence I gave R-b on this one. But had to leave
this comment to not lose context.
- Marijn
- PP_BLK_DIPHER("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0,
sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
-1),
- PP_BLK_DIPHER("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0,
sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
-1),
- PP_BLK_DIPHER("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1,
sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
-1),
- PP_BLK_DIPHER("pingpong_3", PINGPONG_3, 0x6c000, MERGE_3D_1,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_3", PINGPONG_3, 0x6c000, MERGE_3D_1,
sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
-1),
- PP_BLK_DIPHER("pingpong_4", PINGPONG_4, 0x6d000, MERGE_3D_2,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_4", PINGPONG_4, 0x6d000, MERGE_3D_2,
sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 30),
-1),
- PP_BLK_DIPHER("pingpong_5", PINGPONG_5, 0x6e000, MERGE_3D_2,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_5", PINGPONG_5, 0x6e000, MERGE_3D_2,
sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 31),
-1),
- PP_BLK_DIPHER("pingpong_6", PINGPONG_6, 0x66000, MERGE_3D_3,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_6", PINGPONG_6, 0x66000, MERGE_3D_3,
sc7280_pp_sblk,
-1,
-1),
- PP_BLK_DIPHER("pingpong_7", PINGPONG_7, 0x66400, MERGE_3D_3,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_7", PINGPONG_7, 0x66400, MERGE_3D_3,
sc7280_pp_sblk,
-1,
-1),
};
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 03f162af1a50..ca8a02debda9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -491,7 +491,7 @@ static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
.len = 0x20, .version = 0x2},
};
-#define PP_BLK_DIPHER(_name, _id, _base, _merge_3d, _sblk, _done, _rdptr) \
+#define PP_BLK_DITHER(_name, _id, _base, _merge_3d, _sblk, _done, _rdptr) \
{\
.name = _name, .id = _id, \
.base = _base, .len = 0, \
Re: [Freedreno] [PATCH v2 3/3] drm/msm/dpu: Pass catalog pointers directly from RM instead of IDs
On Tue, 25 Apr 2023 at 01:03, Marijn Suijten
wrote:
>
> On 2023-04-21 16:25:15, Abhinav Kumar wrote:
> >
> >
> > On 4/21/2023 1:53 PM, Marijn Suijten wrote:
> > > The Resource Manager already iterates over all available blocks from the
> > > catalog, only to pass their ID to a dpu_hw_xxx_init() function which
> > > uses an _xxx_offset() helper to search for and find the exact same
> > > catalog pointer again to initialize the block with, fallible error
> > > handling and all.
> > >
> > > Instead, pass const pointers to the catalog entries directly to these
> > > _init functions and drop the for loops entirely, saving on both
> > > readability complexity and unnecessary cycles at boot.
> > >
> > > Signed-off-by: Marijn Suijten
> > > Reviewed-by: Dmitry Baryshkov
> >
> > Overall, a nice cleanup!
> >
> > One comment below.
> >
> > > ---
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c| 37
> > > +
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h| 14
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c| 32
> > > +++---
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h| 11 +++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.c | 38
> > > -
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.h | 12 +++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 2 +-
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 40
> > > ++-
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 12 +++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 38
> > > -
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.h | 10 +++---
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_merge3d.c| 33
> > > +++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_merge3d.h| 14
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 33
> > > +++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h | 14
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 39
> > > --
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 12 +++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.c | 33
> > > +++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h | 11 +++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 33
> > > ---
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 11 +++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 17 +-
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 18 +-
> > > 23 files changed, 139 insertions(+), 375 deletions(-)
> > >
> >
> >
> >
> > > -struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx,
> > > - void __iomem *addr,
> > > - const struct dpu_mdss_cfg *m)
> > > +struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> > > + void __iomem *addr)
> > > {
> > > struct dpu_hw_intf *c;
> > > - const struct dpu_intf_cfg *cfg;
> > > +
> > > + if (cfg->type == INTF_NONE) {
> > > + pr_err("Cannot create interface hw object for INTF_NONE
> > > type\n");
> > > + return ERR_PTR(-EINVAL);
> > > + }
> >
> > The caller of dpu_hw_intf_init which is the RM already has protection
> > for INTF_NONE, see below
> >
> > for (i = 0; i < cat->intf_count; i++) {
> > struct dpu_hw_intf *hw;
> > const struct dpu_intf_cfg *intf = &cat->intf[i];
> >
> > if (intf->type == INTF_NONE) {
> > DPU_DEBUG("skip intf %d with type none\n", i);
> > continue;
> > }
> > if (intf->id < INTF_0 || intf->id >= INTF_MAX) {
> > DPU_ERROR("skip intf %d with invalid id\n",
> > intf->id);
> > continue;
> > }
> > hw = dpu_hw_intf_init(intf->id, mmio, cat);
> >
> > So this part can be dropped.
>
> I mainly intended to keep original validation where _intf_offset would
> skip INTF_NONE, and error out. RM init is hence expected to filter out
> INTF_NONE instead of running into that `-EINVAL`, which I maintained
> here.
>
> If you think there won't be another caller of dpu_hw_intf_init, and that
> such validation is hence excessive, I can remove it in a followup v3.
I'd prefer to see the checks at dpu_rm to be dropped.
dpu_hw_intf_init() (and other dpu_hw_foo_init() functions) should be
self-contained. If they can not init HW block (e.g. because the index
is out of the boundaries), they should return an error.
>
> - Marijn
>
> > > c = kzalloc(sizeof(*c), GFP_KERNEL);
> > > if (!c)
> > > return ERR_PTR(-ENOMEM);
> > >
> > > - cfg = _intf_offset(idx, m, addr, &c->hw);
> > > - if (IS_ERR_OR_NULL(cfg)) {
> > > - kfree(c);
> > > - pr
Re: [Freedreno] [PATCH v2 04/17] drm/msm/dpu: Fix PP_BLK_DIPHER -> DITHER typo
On 2023-04-24 13:53:13, Abhinav Kumar wrote:
>
>
> On 4/17/2023 1:21 PM, Marijn Suijten wrote:
> > SM8550 only comes with a DITHER subblock inside the PINGPONG block,
> > hence the name and a block length of zero. However, the PP_BLK macro
> > name was typo'd to DIPHER rather than DITHER.
> >
> > Fixes: efcd0107727c ("drm/msm/dpu: add support for SM8550")
> > Signed-off-by: Marijn Suijten
>
> This change itself is fine, hence
>
> Reviewed-by: Abhinav Kumar
>
> one comment below
>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 16
> >
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
> > 2 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> > b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> > index 9e403034093f..d0ab351b6a8b 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> > @@ -132,28 +132,28 @@ static const struct dpu_dspp_cfg sm8550_dspp[] = {
> > &sm8150_dspp_sblk),
> > };
> > static const struct dpu_pingpong_cfg sm8550_pp[] = {
>
> dither block should be present on many other chipsets too but looks like
> on sm8550 was enabling it. Not sure how it was validated there. But we
> are enabling dither, even other chipsets have this block.
Correct, they all seem to have it starting at sdm845. My patch message
seems to lack the word "exclusively" as the PP on sm8550 appears to
exclusively contain a DITHER subblock (unless other blocks are available
that simply aren't supported within this driver yet) and no other
registers. Hence this aptly named macro exist to emit just the feature
bitflag for that and a .len of zero.
Now, whether we should have the features contain subblock flags rather
than just scanning for their id's or presence in the subblocks is a
different discussion / cleanup we should have.
- Marijn
> > - PP_BLK_DIPHER("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0,
> > sc7280_pp_sblk,
> > + PP_BLK_DITHER("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0,
> > sc7280_pp_sblk,
> > DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
> > -1),
> > - PP_BLK_DIPHER("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0,
> > sc7280_pp_sblk,
> > + PP_BLK_DITHER("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0,
> > sc7280_pp_sblk,
> > DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
> > -1),
> > - PP_BLK_DIPHER("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1,
> > sc7280_pp_sblk,
> > + PP_BLK_DITHER("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1,
> > sc7280_pp_sblk,
> > DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
> > -1),
> > - PP_BLK_DIPHER("pingpong_3", PINGPONG_3, 0x6c000, MERGE_3D_1,
> > sc7280_pp_sblk,
> > + PP_BLK_DITHER("pingpong_3", PINGPONG_3, 0x6c000, MERGE_3D_1,
> > sc7280_pp_sblk,
> > DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
> > -1),
> > - PP_BLK_DIPHER("pingpong_4", PINGPONG_4, 0x6d000, MERGE_3D_2,
> > sc7280_pp_sblk,
> > + PP_BLK_DITHER("pingpong_4", PINGPONG_4, 0x6d000, MERGE_3D_2,
> > sc7280_pp_sblk,
> > DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 30),
> > -1),
> > - PP_BLK_DIPHER("pingpong_5", PINGPONG_5, 0x6e000, MERGE_3D_2,
> > sc7280_pp_sblk,
> > + PP_BLK_DITHER("pingpong_5", PINGPONG_5, 0x6e000, MERGE_3D_2,
> > sc7280_pp_sblk,
> > DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 31),
> > -1),
> > - PP_BLK_DIPHER("pingpong_6", PINGPONG_6, 0x66000, MERGE_3D_3,
> > sc7280_pp_sblk,
> > + PP_BLK_DITHER("pingpong_6", PINGPONG_6, 0x66000, MERGE_3D_3,
> > sc7280_pp_sblk,
> > -1,
> > -1),
> > - PP_BLK_DIPHER("pingpong_7", PINGPONG_7, 0x66400, MERGE_3D_3,
> > sc7280_pp_sblk,
> > + PP_BLK_DITHER("pingpong_7", PINGPONG_7, 0x66400, MERGE_3D_3,
> > sc7280_pp_sblk,
> > -1,
> > -1),
> > };
> > 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 03f162af1a50..ca8a02debda9 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > @@ -491,7 +491,7 @@ static const struct dpu_pingpong_sub_blks
> > sc7280_pp_sblk = {
> > .len = 0x20, .version = 0x2},
> > };
> >
> > -#define PP_BLK_DIPHER(_name, _id, _base, _merge_3d, _sblk, _done, _rdptr) \
> > +#define PP_BLK_DITHER(_name, _id, _base, _merge_3d, _sblk, _done, _rdptr) \
> > {\
> > .name = _name, .id = _id, \
> > .base = _base, .len = 0, \
> >
Re: [Freedreno] [PATCH v2 03/17] drm/msm/dpu: Move non-MDP_TOP INTF_INTR offsets out of hwio header
On 2023-04-24 13:44:55, Abhinav Kumar wrote:
>
>
> On 4/17/2023 1:21 PM, Marijn Suijten wrote:
> > These offsets do not fall under the MDP TOP block and do not fit the
> > comment right above. Move them to dpu_hw_interrupts.c next to the
> > repsective MDP_INTF_x_OFF interrupt block offsets.
> >
> > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> > Signed-off-by: Marijn Suijten
>
> This change itself is fine, hence
>
> Reviewed-by: Abhinav Kumar
>
> One comment below.
>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 5 -
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h | 3 ---
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> > index 53326f25e40e..85c0bda3ff90 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> > @@ -15,7 +15,7 @@
> >
> > /*
> >* Register offsets in MDSS register file for the interrupt registers
> > - * w.r.t. to the MDP base
> > + * w.r.t. the MDP base
> >*/
> > #define MDP_SSPP_TOP0_OFF 0x0
> > #define MDP_INTF_0_OFF0x6A000
> > @@ -24,6 +24,9 @@
> > #define MDP_INTF_3_OFF0x6B800
> > #define MDP_INTF_4_OFF0x6C000
> > #define MDP_INTF_5_OFF0x6C800
> > +#define INTF_INTR_EN 0x1c0
> > +#define INTF_INTR_STATUS 0x1c4
> > +#define INTF_INTR_CLEAR0x1c8
> > #define MDP_AD4_0_OFF 0x7C000
> > #define MDP_AD4_1_OFF 0x7D000
> > #define MDP_AD4_INTR_EN_OFF 0x41c
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
> > index feb9a729844a..5acd5683d25a 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
> > @@ -21,9 +21,6 @@
> > #define HIST_INTR_EN0x01c
> > #define HIST_INTR_STATUS0x020
> > #define HIST_INTR_CLEAR 0x024
>
> Even HIST_INTR_*** need to be moved then.
These are relative to MDP_SSPP_TOP0_OFF too just like
INTR(2)_{CLEAR,EN,STATUS} so I left them here. Otherwise, *all* these
interrupt masks are probably best moved to dpu_hw_interrupts.c for
clarity, as that's also the only place they are used?
Let me know which way you prefer.
- Marijn
> > -#define INTF_INTR_EN0x1C0
> > -#define INTF_INTR_STATUS0x1C4
> > -#define INTF_INTR_CLEAR 0x1C8
> > #define SPLIT_DISPLAY_EN0x2F4
> > #define SPLIT_DISPLAY_UPPER_PIPE_CTRL 0x2F8
> > #define DSPP_IGC_COLOR0_RAM_LUTN0x300
> >
Re: [Freedreno] [PATCH v2 02/17] drm/msm/dpu: Remove TE2 block and feature from DPU >= 7.0.0 hardware
On 2023-04-24 13:41:07, Abhinav Kumar wrote:
>
>
> On 4/17/2023 1:21 PM, Marijn Suijten wrote:
> > No hardware beyond kona (sm8250) defines the TE2 PINGPONG sub-block
> > offset downstream. Even though neither downstream nor upstream utilizes
> > these registers in any way, remove the erroneous specification for
> > SC8280XP, SM8350 and SM8450 to prevent confusion.
> >
> > Note that downstream enables the PPSPLIT (split-FIFO) topology (single
> > LM for 2 PP and 2 INTF) based on the presence of a TE2 block.
> >
> > Fixes: f0a1bdf64dd7 ("drm/msm/dpu: Introduce SC8280XP")
> > Fixes: 0a72f23f6ef8 ("drm/msm/dpu: Add SM8350 to hw catalog")
> > Fixes: 8cbbc3396065 ("drm/msm/dpu: add support for SM8450")
>
> I cannot find any commits with those hashes.
>
> Should this be
>
> Fixes: 4a352c2fc15a ("drm/msm/dpu: Introduce SC8280XP")
> Fixes: 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 to hw catalog")
> Fixes: 100d7ef6995d ("drm/msm/dpu: add support for SM8450")
Yes they are, thanks for spotting that. These patches were on drm-msm
/ msm-next when I made this patch on January 11th, hence these were the
hashes given to me by git bisect: see how those patches have an author
timestamp of January 9th, while the proper hashes that landed upstream
have a hash of January 12th: the branch has been force-pushed after.
Old:
https://gitlab.freedesktop.org/drm/msm/-/commit/f0a1bdf64dd7
https://gitlab.freedesktop.org/drm/msm/-/commit/0a72f23f6ef8
https://gitlab.freedesktop.org/drm/msm/-/commit/8cbbc3396065
New:
https://gitlab.freedesktop.org/drm/msm/-/commit/4a352c2fc15a
https://gitlab.freedesktop.org/drm/msm/-/commit/0e91bcbb0016
https://gitlab.freedesktop.org/drm/msm/-/commit/100d7ef6995d
> Will wait for a day to fix this up, otherwise I will do it while applying.
Thanks, that's appreciated.
- Marijn
> > Signed-off-by: Marijn Suijten
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 4 ++--
> > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 12 ++--
> > drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 4 ++--
> > 3 files changed, 10 insertions(+), 10 deletions(-)
Re: [Freedreno] [PATCH v2 3/3] drm/msm/dpu: Pass catalog pointers directly from RM instead of IDs
On 2023-04-21 16:25:15, Abhinav Kumar wrote:
>
>
> On 4/21/2023 1:53 PM, Marijn Suijten wrote:
> > The Resource Manager already iterates over all available blocks from the
> > catalog, only to pass their ID to a dpu_hw_xxx_init() function which
> > uses an _xxx_offset() helper to search for and find the exact same
> > catalog pointer again to initialize the block with, fallible error
> > handling and all.
> >
> > Instead, pass const pointers to the catalog entries directly to these
> > _init functions and drop the for loops entirely, saving on both
> > readability complexity and unnecessary cycles at boot.
> >
> > Signed-off-by: Marijn Suijten
> > Reviewed-by: Dmitry Baryshkov
>
> Overall, a nice cleanup!
>
> One comment below.
>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c| 37
> > +
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h| 14
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c| 32 +++---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h| 11 +++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.c | 38
> > -
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.h | 12 +++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 2 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 40
> > ++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 12 +++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 38
> > -
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.h | 10 +++---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_merge3d.c| 33 +++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_merge3d.h| 14
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 33 +++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h | 14
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 39
> > --
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 12 +++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.c | 33 +++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h | 11 +++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 33 ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 11 +++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 17 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 18 +-
> > 23 files changed, 139 insertions(+), 375 deletions(-)
> >
>
>
>
> > -struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx,
> > - void __iomem *addr,
> > - const struct dpu_mdss_cfg *m)
> > +struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> > + void __iomem *addr)
> > {
> > struct dpu_hw_intf *c;
> > - const struct dpu_intf_cfg *cfg;
> > +
> > + if (cfg->type == INTF_NONE) {
> > + pr_err("Cannot create interface hw object for INTF_NONE
> > type\n");
> > + return ERR_PTR(-EINVAL);
> > + }
>
> The caller of dpu_hw_intf_init which is the RM already has protection
> for INTF_NONE, see below
>
> for (i = 0; i < cat->intf_count; i++) {
> struct dpu_hw_intf *hw;
> const struct dpu_intf_cfg *intf = &cat->intf[i];
>
> if (intf->type == INTF_NONE) {
> DPU_DEBUG("skip intf %d with type none\n", i);
> continue;
> }
> if (intf->id < INTF_0 || intf->id >= INTF_MAX) {
> DPU_ERROR("skip intf %d with invalid id\n",
> intf->id);
> continue;
> }
> hw = dpu_hw_intf_init(intf->id, mmio, cat);
>
> So this part can be dropped.
I mainly intended to keep original validation where _intf_offset would
skip INTF_NONE, and error out. RM init is hence expected to filter out
INTF_NONE instead of running into that `-EINVAL`, which I maintained
here.
If you think there won't be another caller of dpu_hw_intf_init, and that
such validation is hence excessive, I can remove it in a followup v3.
- Marijn
> > c = kzalloc(sizeof(*c), GFP_KERNEL);
> > if (!c)
> > return ERR_PTR(-ENOMEM);
> >
> > - cfg = _intf_offset(idx, m, addr, &c->hw);
> > - if (IS_ERR_OR_NULL(cfg)) {
> > - kfree(c);
> > - pr_err("failed to create dpu_hw_intf %d\n", idx);
> > - return ERR_PTR(-EINVAL);
> > - }
> > + c->hw.blk_addr = addr + cfg->base;
> > + c->hw.log_mask = DPU_DBG_MASK_INTF;
> >
>
>
Re: [Freedreno] [PATCH v2 04/17] drm/msm/dpu: Fix PP_BLK_DIPHER -> DITHER typo
On 4/17/2023 1:21 PM, Marijn Suijten wrote:
SM8550 only comes with a DITHER subblock inside the PINGPONG block,
hence the name and a block length of zero. However, the PP_BLK macro
name was typo'd to DIPHER rather than DITHER.
Fixes: efcd0107727c ("drm/msm/dpu: add support for SM8550")
Signed-off-by: Marijn Suijten
This change itself is fine, hence
Reviewed-by: Abhinav Kumar
one comment below
---
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 16
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
index 9e403034093f..d0ab351b6a8b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
@@ -132,28 +132,28 @@ static const struct dpu_dspp_cfg sm8550_dspp[] = {
&sm8150_dspp_sblk),
};
static const struct dpu_pingpong_cfg sm8550_pp[] = {
dither block should be present on many other chipsets too but looks like
on sm8550 was enabling it. Not sure how it was validated there. But we
are enabling dither, even other chipsets have this block.
- PP_BLK_DIPHER("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0,
sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
-1),
- PP_BLK_DIPHER("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0,
sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
-1),
- PP_BLK_DIPHER("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1,
sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
-1),
- PP_BLK_DIPHER("pingpong_3", PINGPONG_3, 0x6c000, MERGE_3D_1,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_3", PINGPONG_3, 0x6c000, MERGE_3D_1,
sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
-1),
- PP_BLK_DIPHER("pingpong_4", PINGPONG_4, 0x6d000, MERGE_3D_2,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_4", PINGPONG_4, 0x6d000, MERGE_3D_2,
sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 30),
-1),
- PP_BLK_DIPHER("pingpong_5", PINGPONG_5, 0x6e000, MERGE_3D_2,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_5", PINGPONG_5, 0x6e000, MERGE_3D_2,
sc7280_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 31),
-1),
- PP_BLK_DIPHER("pingpong_6", PINGPONG_6, 0x66000, MERGE_3D_3,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_6", PINGPONG_6, 0x66000, MERGE_3D_3,
sc7280_pp_sblk,
-1,
-1),
- PP_BLK_DIPHER("pingpong_7", PINGPONG_7, 0x66400, MERGE_3D_3,
sc7280_pp_sblk,
+ PP_BLK_DITHER("pingpong_7", PINGPONG_7, 0x66400, MERGE_3D_3,
sc7280_pp_sblk,
-1,
-1),
};
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 03f162af1a50..ca8a02debda9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -491,7 +491,7 @@ static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
.len = 0x20, .version = 0x2},
};
-#define PP_BLK_DIPHER(_name, _id, _base, _merge_3d, _sblk, _done, _rdptr) \
+#define PP_BLK_DITHER(_name, _id, _base, _merge_3d, _sblk, _done, _rdptr) \
{\
.name = _name, .id = _id, \
.base = _base, .len = 0, \
Re: [Freedreno] [PATCH v2 03/17] drm/msm/dpu: Move non-MDP_TOP INTF_INTR offsets out of hwio header
On 4/17/2023 1:21 PM, Marijn Suijten wrote:
These offsets do not fall under the MDP TOP block and do not fit the
comment right above. Move them to dpu_hw_interrupts.c next to the
repsective MDP_INTF_x_OFF interrupt block offsets.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Marijn Suijten
This change itself is fine, hence
Reviewed-by: Abhinav Kumar
One comment below.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 5 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h | 3 ---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index 53326f25e40e..85c0bda3ff90 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -15,7 +15,7 @@
/*
* Register offsets in MDSS register file for the interrupt registers
- * w.r.t. to the MDP base
+ * w.r.t. the MDP base
*/
#define MDP_SSPP_TOP0_OFF 0x0
#define MDP_INTF_0_OFF0x6A000
@@ -24,6 +24,9 @@
#define MDP_INTF_3_OFF0x6B800
#define MDP_INTF_4_OFF0x6C000
#define MDP_INTF_5_OFF0x6C800
+#define INTF_INTR_EN 0x1c0
+#define INTF_INTR_STATUS 0x1c4
+#define INTF_INTR_CLEAR0x1c8
#define MDP_AD4_0_OFF 0x7C000
#define MDP_AD4_1_OFF 0x7D000
#define MDP_AD4_INTR_EN_OFF 0x41c
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
index feb9a729844a..5acd5683d25a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
@@ -21,9 +21,6 @@
#define HIST_INTR_EN0x01c
#define HIST_INTR_STATUS0x020
#define HIST_INTR_CLEAR 0x024
Even HIST_INTR_*** need to be moved then.
-#define INTF_INTR_EN0x1C0
-#define INTF_INTR_STATUS0x1C4
-#define INTF_INTR_CLEAR 0x1C8
#define SPLIT_DISPLAY_EN0x2F4
#define SPLIT_DISPLAY_UPPER_PIPE_CTRL 0x2F8
#define DSPP_IGC_COLOR0_RAM_LUTN0x300
Re: [Freedreno] [PATCH v2 02/17] drm/msm/dpu: Remove TE2 block and feature from DPU >= 7.0.0 hardware
On 4/17/2023 1:21 PM, Marijn Suijten wrote:
No hardware beyond kona (sm8250) defines the TE2 PINGPONG sub-block
offset downstream. Even though neither downstream nor upstream utilizes
these registers in any way, remove the erroneous specification for
SC8280XP, SM8350 and SM8450 to prevent confusion.
Note that downstream enables the PPSPLIT (split-FIFO) topology (single
LM for 2 PP and 2 INTF) based on the presence of a TE2 block.
Fixes: f0a1bdf64dd7 ("drm/msm/dpu: Introduce SC8280XP")
Fixes: 0a72f23f6ef8 ("drm/msm/dpu: Add SM8350 to hw catalog")
Fixes: 8cbbc3396065 ("drm/msm/dpu: add support for SM8450")
I cannot find any commits with those hashes.
Should this be
Fixes: 4a352c2fc15a ("drm/msm/dpu: Introduce SC8280XP")
Fixes: 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 to hw catalog")
Fixes: 100d7ef6995d ("drm/msm/dpu: add support for SM8450")
Will wait for a day to fix this up, otherwise I will do it while applying.
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 4 ++--
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 12 ++--
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 4 ++--
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
index ca107ca8de46..41ef0c8fc993 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
@@ -127,10 +127,10 @@ static const struct dpu_dspp_cfg sm8350_dspp[] = {
};
static const struct dpu_pingpong_cfg sm8350_pp[] = {
- PP_BLK_TE("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0,
sdm845_pp_sblk_te,
+ PP_BLK("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0, sdm845_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12)),
- PP_BLK_TE("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0,
sdm845_pp_sblk_te,
+ PP_BLK("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0, sdm845_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13)),
PP_BLK("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1, sdm845_pp_sblk,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
index 9aab110b8c44..12c14d15e386 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
@@ -121,17 +121,17 @@ static const struct dpu_dspp_cfg sc8280xp_dspp[] = {
};
static const struct dpu_pingpong_cfg sc8280xp_pp[] = {
- PP_BLK_TE("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0,
sdm845_pp_sblk_te,
+ PP_BLK("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0, sdm845_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8), -1),
- PP_BLK_TE("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0,
sdm845_pp_sblk_te,
+ PP_BLK("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0, sdm845_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9), -1),
- PP_BLK_TE("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1,
sdm845_pp_sblk_te,
+ PP_BLK("pingpong_2", PINGPONG_2, 0x6b000, MERGE_3D_1, sdm845_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10), -1),
- PP_BLK_TE("pingpong_3", PINGPONG_3, 0x6c000, MERGE_3D_1,
sdm845_pp_sblk_te,
+ PP_BLK("pingpong_3", PINGPONG_3, 0x6c000, MERGE_3D_1, sdm845_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11), -1),
- PP_BLK_TE("pingpong_4", PINGPONG_4, 0x6d000, MERGE_3D_2,
sdm845_pp_sblk_te,
+ PP_BLK("pingpong_4", PINGPONG_4, 0x6d000, MERGE_3D_2, sdm845_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 30), -1),
- PP_BLK_TE("pingpong_5", PINGPONG_5, 0x6e000, MERGE_3D_2,
sdm845_pp_sblk_te,
+ PP_BLK("pingpong_5", PINGPONG_5, 0x6e000, MERGE_3D_2, sdm845_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 31), -1),
};
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
index 02a259b6b426..e409c119b0a2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
@@ -128,10 +128,10 @@ static const struct dpu_dspp_cfg sm8450_dspp[] = {
};
/* FIXME: interrupts */
static const struct dpu_pingpong_cfg sm8450_pp[] = {
- PP_BLK_TE("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0,
sdm845_pp_sblk_te,
+ PP_BLK("pingpong_0", PINGPONG_0, 0x69000, MERGE_3D_0, sdm845_pp_sblk,
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12)),
- PP_BLK_TE("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0,
sdm845_pp_sblk_te,
+ PP_BLK("pingpong_1", PINGPONG_1, 0x6a000, MERGE_3D_0, sdm845_pp_sblk,
Re: [Freedreno] [PATCH v1 5/5] drm/msm/dpu: add DSC 1.2 hw blocks for relevant chipsets
On 4/21/2023 4:22 PM, Dmitry Baryshkov wrote:
On 22/04/2023 02:16, Kuogee Hsieh wrote:
On 4/21/2023 4:11 PM, Dmitry Baryshkov wrote:
On 22/04/2023 02:08, Kuogee Hsieh wrote:
On 4/21/2023 3:16 PM, Dmitry Baryshkov wrote:
On 22/04/2023 01:05, Kuogee Hsieh wrote:
On 4/20/2023 5:07 PM, Dmitry Baryshkov wrote:
On 21/04/2023 02:25, Kuogee Hsieh wrote:
From: Abhinav Kumar
Add DSC 1.2 hardware blocks to the catalog with necessary
sub-block and feature flag information.
Each display compression engine (DCE) contains dual hard
slice DSC encoders so both share same base address but with
its own different sub block address.
Please correct line wrapping. 72-75 is usually the preferred width
Signed-off-by: Abhinav Kumar
Signed-off-by: Kuogee Hsieh
---
.../gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 19
+++
.../gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 11
+++
.../drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 21
+
.../gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 19
+++
.../gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 19
+++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 12 ++--
6 files changed, 99 insertions(+), 2 deletions(-)
[I commented on sm8550, it applies to all the rest of platforms]
diff --git
a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
index 9e40303..72a7bcf 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
@@ -165,6 +165,23 @@ static const struct dpu_merge_3d_cfg
sm8550_merge_3d[] = {
MERGE_3D_BLK("merge_3d_3", MERGE_3D_3, 0x66700),
};
+static const struct dpu_dsc_sub_blks sm8550_dsc_sblk_0 = {
+ .enc = {.base = 0x100, .len = 0x100},
+ .ctl = {.base = 0xF00, .len = 0x10},
+};
+
+static const struct dpu_dsc_sub_blks sm8550_dsc_sblk_1 = {
+ .enc = {.base = 0x200, .len = 0x100},
+ .ctl = {.base = 0xF80, .len = 0x10},
+};
Please keep sblk in dpu_hw_catalog for now.
+
+static const struct dpu_dsc_cfg sm8550_dsc[] = {
+ DSC_BLK_1_2("dsc_0", DSC_0, 0x8, 0x100, 0,
sm8550_dsc_sblk_0),
+ DSC_BLK_1_2("dsc_0", DSC_1, 0x8, 0x100, 0,
sm8550_dsc_sblk_1),
Is there a reason why index in "dsc_N" doesn't match the DSC_n
which comes next to it?
usually each DCE (display compression engine) contains two hard
slice encoders.
DSC_0 and DSC_1 (index) is belong to dsc_0.
If there are two DCE, then DSC_2 and DSC_3 belong to dsc_1
Ah, I see now. So, the block register space is the following:
DCEi ->
common
dsc0_enc
dsc1_enc
dsc0_ctl
dsc1_ctl
Instead of declaring a single DCE unit with two DSC blocks, we
declare two distinct DSC blocks. This raises a question, how
independent are these two parts of a single DCE block? For
example, can we use them to perform compression with different
parameters? Or use one of them for the DP DSC and another one for
DSI DSC? Can we have the following configuration:
DSC_0 => DP DSC
DSC_1, DSC_2 => DSI DSC in DSC_MERGE topology?
no, For merge mode you have to use same DCE, such as DSC_2 and DSC3
(pair)
Ok, this is for the merge mode. So the dpu_rm should be extended to
allocate them in pairs if merge mode is requested.
What about using DSC_0 for DP and DSC_1 for DSI? Is it possible?
I never do that, but i think it should works since they can work
independently.
Good, thanks for the confirmation. For v2, could you please describe
this arrangement of DCE -> 2xDSC in a comment close to DSC_BLK_1_2 and
corresponding sblk definitions?
Also could you please fix dpu_rm to allocate DSC blocks in pairs for
DSC_MERGE mode.
yes, I will fix DSC_MERGE mode at next patch serial.
Last, but not least, would it make sense to name the blocks as "dceN"
instead of "dscN"?
+ DSC_BLK_1_2("dsc_1", DSC_2, 0x81000, 0x100,
BIT(DPU_DSC_NATIVE_422_EN), sm8550_dsc_sblk_0),
+ DSC_BLK_1_2("dsc_1", DSC_3, 0x81000, 0x100,
BIT(DPU_DSC_NATIVE_422_EN), sm8550_dsc_sblk_1),
+};
+
static const struct dpu_intf_cfg sm8550_intf[] = {
INTF_BLK("intf_0", INTF_0, 0x34000, 0x280, INTF_DP,
MSM_DP_CONTROLLER_0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR,
24, 25),
/* TODO TE sub-blocks for intf1 & intf2 */
@@ -218,6 +235,8 @@ const struct dpu_mdss_cfg dpu_sm8550_cfg = {
.dspp = sm8550_dspp,
.pingpong_count = ARRAY_SIZE(sm8550_pp),
.pingpong = sm8550_pp,
+ .dsc = sm8550_dsc,
+ .dsc_count = ARRAY_SIZE(sm8550_dsc),
.merge_3d_count = ARRAY_SIZE(sm8550_merge_3d),
.merge_3d = sm8550_merge_3d,
.intf_count = ARRAY_SIZE(sm8550_intf),
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 03f162a..be08158 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -1,6 +1,6 @@
// SPDX-Li
Re: [Freedreno] [PATCH v2 01/17] drm/msm/dpu: Remove unused INTF0 interrupt mask from SM6115/QCM2290
On 4/17/2023 1:21 PM, Marijn Suijten wrote:
Neither of these SoCs has INTF0, they only have a DSI interface on index
1. Stop enabling an interrupt that can't fire.
Fixes: 3581b7062cec ("drm/msm/disp/dpu1: add support for display on SM6115")
Fixes: 5334087ee743 ("drm/msm: add support for QCM2290 MDSS")
Signed-off-by: Marijn Suijten
Reviewed-by: Dmitry Baryshkov
Reviewed-by: Konrad Dybcio
---
Yes, this is right, Both of these chipsets only have DSI on index 1.
Reviewed-by: Abhinav Kumar
Re: [Freedreno] [PATCH v2 4/5] drm/msm/mdss: Handle the reg bus ICC path
Hi Konrad,
On 18.04.23 15:10, Konrad Dybcio wrote:
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..
On the MDSS side, we only have to ensure that it's on at what Qualcomm
downstream calls "77 MHz", a.k.a 76.8 Mbps and turn it off at suspend.
To achieve that, make msm_mdss_icc_request_bw() accept a boolean to
indicate whether we want the busses to be on or off, as this function's
only use is to vote for minimum or no bandwidth at all.
Signed-off-by: Konrad Dybcio
---
drivers/gpu/drm/msm/msm_mdss.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
[..]
-static void msm_mdss_icc_request_bw(struct msm_mdss *msm_mdss, unsigned long
bw)
+static void msm_mdss_icc_request_bw(struct msm_mdss *msm_mdss, bool enable)
{
int i;
for (i = 0; i < msm_mdss->num_mdp_paths; i++)
- icc_set_bw(msm_mdss->mdp_path[i], 0, Bps_to_icc(bw));
+ icc_set_bw(msm_mdss->mdp_path[i], 0, enable ?
Bps_to_icc(MIN_IB_BW) : 0);
+
+ if (msm_mdss->reg_bus_path)
+ icc_set_bw(msm_mdss->reg_bus_path, 0, enable ? 76800 : 0);
Please use Bps_to_icc, kbps_to_icc or any of the other macros.
BR,
Georgi
Re: [Freedreno] [PATCH v2 2/3] DRM: Create new Content Protection connector property
Hi Mark (and Dmitry)
On Fri, 21 Apr 2023 at 18:07, Dmitry Baryshkov
wrote:
>
> On 21/04/2023 19:27, Mark Yacoub wrote:
> > From: Mark Yacoub
>
> Nit: is there a reason for this header? My first impression is that it
> matches your outgoing name & email address and as such is not necessary.
>
> Nit#2: subject should mention 'Key', as you are creating a property for
> the key.
>
> >
> > [Why]
> > To enable Protected Content, some drivers require a key to be injected
> > from user space to enable HDCP on the connector.
> >
> > [How]
> > Create new "Content Protection Property" of type "Blob"
>
> Generic observation is that the ability to inject HDCP keys manually
> seems to be quite unique to your hardware. As such, I think the debugfs
> or sysfs suits better in comparison to the DRM property.
I was about to reply to v1 with a very similar comment over the
requirement to keep HDCP keys secret.
v2 has added WRITE_ONLY blobs so at least another process can't just
read the blob back out again, but it feels like there are still
numerous ways to grab those keys. For an unsecured userspace to have
the keys in the first place seems like a bad move, and IMHO they
should only be held in either a secure environment, or only held in
hardware (passed direct from OTP to HDCP block).
There's also the DRM uAPI requirement for having reviewed patches for
an open source project to go alongside any uAPI change. Do such
patches exist?
https://docs.kernel.org/gpu/drm-uapi.html#open-source-userspace-requirements
Dave
> >
> > Signed-off-by: Mark Yacoub
> > ---
> > drivers/gpu/drm/drm_atomic_uapi.c | 9 +
> > include/drm/drm_connector.h | 6 ++
> > include/drm/drm_mode_config.h | 6 ++
> > 3 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > b/drivers/gpu/drm/drm_atomic_uapi.c
> > index d867e7f9f2cd5..e20bc57cdb05c 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -749,6 +749,11 @@ static int drm_atomic_connector_set_property(struct
> > drm_connector *connector,
> > state->content_protection = val;
> > } else if (property == config->hdcp_content_type_property) {
> > state->hdcp_content_type = val;
> > + } else if (property == config->content_protection_key_property) {
> > + ret = drm_atomic_replace_property_blob_from_id(
> > + dev, &state->content_protection_key, val, -1, -1,
> > + &replaced);
> > + return ret;
> > } else if (property == connector->colorspace_property) {
> > state->colorspace = val;
> > } else if (property == config->writeback_fb_id_property) {
> > @@ -843,6 +848,10 @@ drm_atomic_connector_get_property(struct drm_connector
> > *connector,
> > *val = state->content_protection;
> > } else if (property == config->hdcp_content_type_property) {
> > *val = state->hdcp_content_type;
> > + } else if (property == config->content_protection_key_property) {
> > + *val = state->content_protection_key ?
> > +state->content_protection_key->base.id :
> > +0;
> > } else if (property == config->writeback_fb_id_property) {
> > /* Writeback framebuffer is one-shot, write and forget */
> > *val = 0;
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 7b5048516185c..2fbe51272bfeb 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -896,6 +896,12 @@ struct drm_connector_state {
> >*/
> > unsigned int content_protection;
> >
> > + /**
> > + * @content_protection_key: DRM blob property for holding the Content
> > + * Protection Key injected from user space.
> > + */
> > + struct drm_property_blob *content_protection_key;
> > +
> > /**
> >* @colorspace: State variable for Connector property to request
> >* colorspace change on Sink. This is most commonly used to switch
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index e5b053001d22e..615d1e5f57562 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -887,6 +887,12 @@ struct drm_mode_config {
> >*/
> > struct drm_property *hdcp_content_type_property;
> >
> > + /**
> > + * @content_protection_key_property: DRM blob property that receives
> > the
> > + * content protection key from user space to be injected into the
> > kernel.
> > + */
> > + struct drm_property *content_protection_key_property;
> > +
> > /* dumb ioctl parameters */
> > uint32_t preferred_depth, prefer_shadow;
> >
>
> --
> With best wishes
> Dmitry
>
