| Message ID | 20250430-b4-sm8750-display-v5-21-8cab30c3e4df@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm: Add support for SM8750 | expand |
On Wed, Apr 30, 2025 at 03:00:51PM +0200, Krzysztof Kozlowski wrote: > v12.0 DPU on SM8750 comes with 10-bit color alpha. Add register > differences and new implementations of setup_alpha_out(), > setup_border_color() and setup_blend_config(). > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > Changes in v4: > 1. Lowercase hex, use spaces for define indentation > 2. _dpu_crtc_setup_blend_cfg(): pass mdss_ver instead of ctl > > Changes in v3: > 1. New patch, split from previous big DPU v12.0. > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 19 ++++--- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 84 +++++++++++++++++++++++++++++-- > 2 files changed, 94 insertions(+), 9 deletions(-) > > @@ -175,12 +246,19 @@ struct dpu_hw_mixer *dpu_hw_lm_init(struct drm_device *dev, > c->idx = cfg->id; > c->cap = cfg; > c->ops.setup_mixer_out = dpu_hw_lm_setup_out; > - if (mdss_ver->core_major_ver >= 4) > + if (mdss_ver->core_major_ver >= 12) > + c->ops.setup_blend_config = dpu_hw_lm_setup_blend_config_combined_alpha_v12; > + else if (mdss_ver->core_major_ver >= 4) > c->ops.setup_blend_config = dpu_hw_lm_setup_blend_config_combined_alpha; > else > c->ops.setup_blend_config = dpu_hw_lm_setup_blend_config; > - c->ops.setup_alpha_out = dpu_hw_lm_setup_color3; > - c->ops.setup_border_color = dpu_hw_lm_setup_border_color; > + if (mdss_ver->core_major_ver < 12) { > + c->ops.setup_alpha_out = dpu_hw_lm_setup_color3; > + c->ops.setup_border_color = dpu_hw_lm_setup_border_color; > + } else { > + c->ops.setup_alpha_out = dpu_hw_lm_setup_color3_v12; > + c->ops.setup_border_color = dpu_hw_lm_setup_border_color_v12; > + } I tried picking up these patches, and choked on this one. This heavility depends on the DPU fetures bits rework patchset (mentioned in the cover letter, it's fine), but granted the lack of the reviews / updates on that patchset I can neither apply this patch (and its dependencies) nor steer Krzysztof away from basing on that patchset (this patch provides a perfect example of why that series is useful and correct). Abhinav, could you please continue reviewing that patch series?
On 5/5/2025 5:24 AM, Dmitry Baryshkov wrote: > On Wed, Apr 30, 2025 at 03:00:51PM +0200, Krzysztof Kozlowski wrote: >> v12.0 DPU on SM8750 comes with 10-bit color alpha. Add register >> differences and new implementations of setup_alpha_out(), >> setup_border_color() and setup_blend_config(). >> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> --- >> >> Changes in v4: >> 1. Lowercase hex, use spaces for define indentation >> 2. _dpu_crtc_setup_blend_cfg(): pass mdss_ver instead of ctl >> >> Changes in v3: >> 1. New patch, split from previous big DPU v12.0. >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 19 ++++--- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 84 +++++++++++++++++++++++++++++-- >> 2 files changed, 94 insertions(+), 9 deletions(-) >> >> @@ -175,12 +246,19 @@ struct dpu_hw_mixer *dpu_hw_lm_init(struct drm_device *dev, >> c->idx = cfg->id; >> c->cap = cfg; >> c->ops.setup_mixer_out = dpu_hw_lm_setup_out; >> - if (mdss_ver->core_major_ver >= 4) >> + if (mdss_ver->core_major_ver >= 12) >> + c->ops.setup_blend_config = dpu_hw_lm_setup_blend_config_combined_alpha_v12; >> + else if (mdss_ver->core_major_ver >= 4) >> c->ops.setup_blend_config = dpu_hw_lm_setup_blend_config_combined_alpha; >> else >> c->ops.setup_blend_config = dpu_hw_lm_setup_blend_config; >> - c->ops.setup_alpha_out = dpu_hw_lm_setup_color3; >> - c->ops.setup_border_color = dpu_hw_lm_setup_border_color; >> + if (mdss_ver->core_major_ver < 12) { >> + c->ops.setup_alpha_out = dpu_hw_lm_setup_color3; >> + c->ops.setup_border_color = dpu_hw_lm_setup_border_color; >> + } else { >> + c->ops.setup_alpha_out = dpu_hw_lm_setup_color3_v12; >> + c->ops.setup_border_color = dpu_hw_lm_setup_border_color_v12; >> + } > > I tried picking up these patches, and choked on this one. This heavility > depends on the DPU fetures bits rework patchset (mentioned in the cover > letter, it's fine), but granted the lack of the reviews / updates on > that patchset I can neither apply this patch (and its dependencies) nor > steer Krzysztof away from basing on that patchset (this patch provides a > perfect example of why that series is useful and correct). > > Abhinav, could you please continue reviewing that patch series? > I think we could have continued this series on top of the current feature bits model and I thought we were doing that based on #linux-arm-msm chats in Feb between you and me. Not sure what happened there. Regarding the review, myself and Jessica have discussed this last week and Jessica will take over the review of that series and please work with addressing the comments provided there by her.
On 19/05/2025 19:49, Abhinav Kumar wrote: > > > On 5/5/2025 5:24 AM, Dmitry Baryshkov wrote: >> On Wed, Apr 30, 2025 at 03:00:51PM +0200, Krzysztof Kozlowski wrote: >>> v12.0 DPU on SM8750 comes with 10-bit color alpha. Add register >>> differences and new implementations of setup_alpha_out(), >>> setup_border_color() and setup_blend_config(). >>> >>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>> >>> --- >>> >>> Changes in v4: >>> 1. Lowercase hex, use spaces for define indentation >>> 2. _dpu_crtc_setup_blend_cfg(): pass mdss_ver instead of ctl >>> >>> Changes in v3: >>> 1. New patch, split from previous big DPU v12.0. >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 19 ++++--- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 84 ++++++++++++++++++++ >>> +++++++++-- >>> 2 files changed, 94 insertions(+), 9 deletions(-) >>> >>> @@ -175,12 +246,19 @@ struct dpu_hw_mixer *dpu_hw_lm_init(struct >>> drm_device *dev, >>> c->idx = cfg->id; >>> c->cap = cfg; >>> c->ops.setup_mixer_out = dpu_hw_lm_setup_out; >>> - if (mdss_ver->core_major_ver >= 4) >>> + if (mdss_ver->core_major_ver >= 12) >>> + c->ops.setup_blend_config = >>> dpu_hw_lm_setup_blend_config_combined_alpha_v12; >>> + else if (mdss_ver->core_major_ver >= 4) >>> c->ops.setup_blend_config = >>> dpu_hw_lm_setup_blend_config_combined_alpha; >>> else >>> c->ops.setup_blend_config = dpu_hw_lm_setup_blend_config; >>> - c->ops.setup_alpha_out = dpu_hw_lm_setup_color3; >>> - c->ops.setup_border_color = dpu_hw_lm_setup_border_color; >>> + if (mdss_ver->core_major_ver < 12) { >>> + c->ops.setup_alpha_out = dpu_hw_lm_setup_color3; >>> + c->ops.setup_border_color = dpu_hw_lm_setup_border_color; >>> + } else { >>> + c->ops.setup_alpha_out = dpu_hw_lm_setup_color3_v12; >>> + c->ops.setup_border_color = dpu_hw_lm_setup_border_color_v12; >>> + } >> >> I tried picking up these patches, and choked on this one. This heavility >> depends on the DPU fetures bits rework patchset (mentioned in the cover >> letter, it's fine), but granted the lack of the reviews / updates on >> that patchset I can neither apply this patch (and its dependencies) nor >> steer Krzysztof away from basing on that patchset (this patch provides a >> perfect example of why that series is useful and correct). >> >> Abhinav, could you please continue reviewing that patch series? >> > > I think we could have continued this series on top of the current > feature bits model and I thought we were doing that based on #linux-arm- > msm chats in Feb between you and me. Not sure what happened there. I'm also not so sure. Krzysztof has been posting it on top of the feature-removal series, so be it. Let's see, how many patches of that series would be acceptable in the end and decide the fate of this series afterwards. > > Regarding the review, myself and Jessica have discussed this last week > and Jessica will take over the review of that series and please work > with addressing the comments provided there by her. Ack
On Fri, May 23, 2025 at 10:02:03AM +0300, Abel Vesa wrote: > On 25-05-23 09:55:00, Abel Vesa wrote: > > On 25-04-30 15:00:51, Krzysztof Kozlowski wrote: > > > v12.0 DPU on SM8750 comes with 10-bit color alpha. Add register > > > differences and new implementations of setup_alpha_out(), > > > setup_border_color() and setup_blend_config(). > > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > > > --- > > > > > > Changes in v4: > > > 1. Lowercase hex, use spaces for define indentation > > > 2. _dpu_crtc_setup_blend_cfg(): pass mdss_ver instead of ctl > > > > > > Changes in v3: > > > 1. New patch, split from previous big DPU v12.0. > > > --- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 19 ++++--- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 84 +++++++++++++++++++++++++++++-- > > > 2 files changed, 94 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > index a4b0fe0d9899b32141928f0b6a16503a49b3c27a..90f47fc15ee5708795701d78a1380f4ab01c1427 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > @@ -320,14 +320,20 @@ static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc, > > > } > > > > > > static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer *mixer, > > > - struct dpu_plane_state *pstate, const struct msm_format *format) > > > + struct dpu_plane_state *pstate, > > > + const struct msm_format *format, > > > + const struct dpu_mdss_version *mdss_ver) > > > { > > > struct dpu_hw_mixer *lm = mixer->hw_lm; > > > uint32_t blend_op; > > > - uint32_t fg_alpha, bg_alpha; > > > + uint32_t fg_alpha, bg_alpha, max_alpha; > > > > > > fg_alpha = pstate->base.alpha >> 8; > > > > For the 10-bit alpha, you need to shift here by 5 instead of 8. > > Typo. "6 instead of 8". Granted there would be a next iteration of this patch, I'd suggest to modify _dpu_crtc_setup_blend_cfg() to always use 16-bit values and pass them down to LM's setup_blend_config() callback. Then LM can perform version-specific shifts, utilizing either 8 bits or 10 bits of alpha channel values.
On 23/05/2025 09:02, Abel Vesa wrote: >>> static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer *mixer, >>> - struct dpu_plane_state *pstate, const struct msm_format *format) >>> + struct dpu_plane_state *pstate, >>> + const struct msm_format *format, >>> + const struct dpu_mdss_version *mdss_ver) >>> { >>> struct dpu_hw_mixer *lm = mixer->hw_lm; >>> uint32_t blend_op; >>> - uint32_t fg_alpha, bg_alpha; >>> + uint32_t fg_alpha, bg_alpha, max_alpha; >>> >>> fg_alpha = pstate->base.alpha >> 8; >> >> For the 10-bit alpha, you need to shift here by 5 instead of 8. > > Typo. "6 instead of 8". > Thanks, this indeed fixes the darkness! Best regards, Krzysztof
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index a4b0fe0d9899b32141928f0b6a16503a49b3c27a..90f47fc15ee5708795701d78a1380f4ab01c1427 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -320,14 +320,20 @@ static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc, } static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer *mixer, - struct dpu_plane_state *pstate, const struct msm_format *format) + struct dpu_plane_state *pstate, + const struct msm_format *format, + const struct dpu_mdss_version *mdss_ver) { struct dpu_hw_mixer *lm = mixer->hw_lm; uint32_t blend_op; - uint32_t fg_alpha, bg_alpha; + uint32_t fg_alpha, bg_alpha, max_alpha; fg_alpha = pstate->base.alpha >> 8; - bg_alpha = 0xff - fg_alpha; + if (mdss_ver->core_major_ver < 12) + max_alpha = 0xff; + else + max_alpha = 0x3ff; + bg_alpha = max_alpha - fg_alpha; /* default to opaque blending */ if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE || @@ -337,7 +343,7 @@ static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer *mixer, } else if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PREMULTI) { blend_op = DPU_BLEND_FG_ALPHA_FG_CONST | DPU_BLEND_BG_ALPHA_FG_PIXEL; - if (fg_alpha != 0xff) { + if (fg_alpha != max_alpha) { bg_alpha = fg_alpha; blend_op |= DPU_BLEND_BG_MOD_ALPHA | DPU_BLEND_BG_INV_MOD_ALPHA; @@ -348,7 +354,7 @@ static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer *mixer, /* coverage blending */ blend_op = DPU_BLEND_FG_ALPHA_FG_PIXEL | DPU_BLEND_BG_ALPHA_FG_PIXEL; - if (fg_alpha != 0xff) { + if (fg_alpha != max_alpha) { bg_alpha = fg_alpha; blend_op |= DPU_BLEND_FG_MOD_ALPHA | DPU_BLEND_FG_INV_MOD_ALPHA | @@ -482,7 +488,8 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, /* blend config update */ for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) { - _dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate, format); + _dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate, format, + ctl->mdss_ver); if (bg_alpha_enable && !format->alpha_enable) mixer[lm_idx].mixer_op_mode = 0; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c index 3bfb61cb83672dca4236bdbbbfb1e442223576d2..f220a68e138cb9e7c88194e53e47391de7ed04f7 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c @@ -19,12 +19,20 @@ /* These register are offset to mixer base + stage base */ #define LM_BLEND0_OP 0x00 + +/* <v12 DPU with offset to mixer base + stage base */ #define LM_BLEND0_CONST_ALPHA 0x04 #define LM_FG_COLOR_FILL_COLOR_0 0x08 #define LM_FG_COLOR_FILL_COLOR_1 0x0C #define LM_FG_COLOR_FILL_SIZE 0x10 #define LM_FG_COLOR_FILL_XY 0x14 +/* >= v12 DPU */ +#define LM_BORDER_COLOR_0_V12 0x1c +#define LM_BORDER_COLOR_1_V12 0x20 + +/* >= v12 DPU with offset to mixer base + stage base */ +#define LM_BLEND0_CONST_ALPHA_V12 0x08 #define LM_BLEND0_FG_ALPHA 0x04 #define LM_BLEND0_BG_ALPHA 0x08 @@ -83,6 +91,22 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx, } } +static void dpu_hw_lm_setup_border_color_v12(struct dpu_hw_mixer *ctx, + struct dpu_mdss_color *color, + u8 border_en) +{ + struct dpu_hw_blk_reg_map *c = &ctx->hw; + + if (border_en) { + DPU_REG_WRITE(c, LM_BORDER_COLOR_0_V12, + (color->color_0 & 0x3ff) | + ((color->color_1 & 0x3ff) << 16)); + DPU_REG_WRITE(c, LM_BORDER_COLOR_1_V12, + (color->color_2 & 0x3ff) | + ((color->color_3 & 0x3ff) << 16)); + } +} + static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx) { dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, 0x0); @@ -112,6 +136,27 @@ static void dpu_hw_lm_setup_blend_config_combined_alpha(struct dpu_hw_mixer *ctx DPU_REG_WRITE(c, LM_BLEND0_OP + stage_off, blend_op); } +static void +dpu_hw_lm_setup_blend_config_combined_alpha_v12(struct dpu_hw_mixer *ctx, + u32 stage, u32 fg_alpha, + u32 bg_alpha, u32 blend_op) +{ + struct dpu_hw_blk_reg_map *c = &ctx->hw; + int stage_off; + u32 const_alpha; + + if (stage == DPU_STAGE_BASE) + return; + + stage_off = _stage_offset(ctx, stage); + if (WARN_ON(stage_off < 0)) + return; + + const_alpha = (bg_alpha & 0x3ff) | ((fg_alpha & 0x3ff) << 16); + DPU_REG_WRITE(c, LM_BLEND0_CONST_ALPHA_V12 + stage_off, const_alpha); + DPU_REG_WRITE(c, LM_BLEND0_OP + stage_off, blend_op); +} + static void dpu_hw_lm_setup_blend_config(struct dpu_hw_mixer *ctx, u32 stage, u32 fg_alpha, u32 bg_alpha, u32 blend_op) { @@ -144,6 +189,32 @@ static void dpu_hw_lm_setup_color3(struct dpu_hw_mixer *ctx, DPU_REG_WRITE(c, LM_OP_MODE, op_mode); } +static void dpu_hw_lm_setup_color3_v12(struct dpu_hw_mixer *ctx, + uint32_t mixer_op_mode) +{ + struct dpu_hw_blk_reg_map *c = &ctx->hw; + int op_mode, stages, stage_off, i; + + stages = ctx->cap->sblk->maxblendstages; + if (stages <= 0) + return; + + for (i = DPU_STAGE_0; i <= stages; i++) { + stage_off = _stage_offset(ctx, i); + if (WARN_ON(stage_off < 0)) + return; + + /* set color_out3 bit in blend0_op when enabled in mixer_op_mode */ + op_mode = DPU_REG_READ(c, LM_BLEND0_OP + stage_off); + if (mixer_op_mode & BIT(i)) + op_mode |= BIT(30); + else + op_mode &= ~BIT(30); + + DPU_REG_WRITE(c, LM_BLEND0_OP + stage_off, op_mode); + } +} + /** * dpu_hw_lm_init() - Initializes the mixer hw driver object. * should be called once before accessing every mixer. @@ -175,12 +246,19 @@ struct dpu_hw_mixer *dpu_hw_lm_init(struct drm_device *dev, c->idx = cfg->id; c->cap = cfg; c->ops.setup_mixer_out = dpu_hw_lm_setup_out; - if (mdss_ver->core_major_ver >= 4) + if (mdss_ver->core_major_ver >= 12) + c->ops.setup_blend_config = dpu_hw_lm_setup_blend_config_combined_alpha_v12; + else if (mdss_ver->core_major_ver >= 4) c->ops.setup_blend_config = dpu_hw_lm_setup_blend_config_combined_alpha; else c->ops.setup_blend_config = dpu_hw_lm_setup_blend_config; - c->ops.setup_alpha_out = dpu_hw_lm_setup_color3; - c->ops.setup_border_color = dpu_hw_lm_setup_border_color; + if (mdss_ver->core_major_ver < 12) { + c->ops.setup_alpha_out = dpu_hw_lm_setup_color3; + c->ops.setup_border_color = dpu_hw_lm_setup_border_color; + } else { + c->ops.setup_alpha_out = dpu_hw_lm_setup_color3_v12; + c->ops.setup_border_color = dpu_hw_lm_setup_border_color_v12; + } c->ops.setup_misr = dpu_hw_lm_setup_misr; c->ops.collect_misr = dpu_hw_lm_collect_misr;