[v10] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280
Commit Message
Flush mechanism for DSPP blocks has changed in sc7280 family, it
allows individual sub blocks to be flushed in coordination with
master flush control.
Representation: master_flush && (PCC_flush | IGC_flush .. etc )
This change adds necessary support for the above design.
Changes in v1:
- Few nits (Doug, Dmitry)
- Restrict sub-block flush programming to dpu_hw_ctl file (Dmitry)
Changes in v2:
- Move the address offset to flush macro (Dmitry)
- Seperate ops for the sub block flush (Dmitry)
Changes in v3:
- Reuse the DPU_DSPP_xx enum instead of a new one (Dmitry)
Changes in v4:
- Use shorter version for unsigned int (Stephen)
Changes in v5:
- Spurious patch please ignore.
Changes in v6:
- Add SOB tag (Doug, Dmitry)
Changes in v7:
- Cache flush mask per dspp (Dmitry)
- Few nits (Marijn)
Changes in v8:
- Few nits (Marijn)
Changes in v9:
- use DSPP enum while accessing flush mask to make it readable (Dmitry)
- Few nits (Dmitry)
Changes in v10:
- fix white spaces in a seperate patch (Dmitry)
Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 5 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 44 ++++++++++++++++++++++++--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 5 ++-
5 files changed, 55 insertions(+), 5 deletions(-)
Comments
On 2022-12-07 04:59:23, Kalyan Thota wrote:
> Flush mechanism for DSPP blocks has changed in sc7280 family, it
> allows individual sub blocks to be flushed in coordination with
> master flush control.
>
> Representation: master_flush && (PCC_flush | IGC_flush .. etc )
>
> This change adds necessary support for the above design.
>
> Changes in v1:
> - Few nits (Doug, Dmitry)
> - Restrict sub-block flush programming to dpu_hw_ctl file (Dmitry)
>
> Changes in v2:
> - Move the address offset to flush macro (Dmitry)
> - Seperate ops for the sub block flush (Dmitry)
>
> Changes in v3:
> - Reuse the DPU_DSPP_xx enum instead of a new one (Dmitry)
>
> Changes in v4:
> - Use shorter version for unsigned int (Stephen)
>
> Changes in v5:
> - Spurious patch please ignore.
>
> Changes in v6:
> - Add SOB tag (Doug, Dmitry)
>
> Changes in v7:
> - Cache flush mask per dspp (Dmitry)
> - Few nits (Marijn)
>
> Changes in v8:
> - Few nits (Marijn)
>
> Changes in v9:
> - use DSPP enum while accessing flush mask to make it readable (Dmitry)
> - Few nits (Dmitry)
>
> Changes in v10:
> - fix white spaces in a seperate patch (Dmitry)
>
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 5 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 +++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 44 ++++++++++++++++++++++++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 5 ++-
> 5 files changed, 55 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 601d687..4170fbe 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -766,7 +766,7 @@ static void _dpu_crtc_setup_cp_blocks(struct drm_crtc *crtc)
>
> /* stage config flush mask */
> ctl->ops.update_pending_flush_dspp(ctl,
> - mixer[i].hw_dspp->idx);
> + mixer[i].hw_dspp->idx, DPU_DSPP_PCC);
> }
> }
>
> 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 27f029f..0eecb2f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -65,7 +65,10 @@
> (PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2))
>
> #define CTL_SC7280_MASK \
> - (BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_FETCH_ACTIVE) | BIT(DPU_CTL_VM_CFG))
> + (BIT(DPU_CTL_ACTIVE_CFG) | \
> + BIT(DPU_CTL_FETCH_ACTIVE) | \
> + BIT(DPU_CTL_VM_CFG) | \
> + BIT(DPU_CTL_DSPP_SUB_BLOCK_FLUSH))
>
> #define MERGE_3D_SM8150_MASK (0)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index 38aa38a..126ee37 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -161,10 +161,12 @@ enum {
> * DSPP sub-blocks
> * @DPU_DSPP_PCC Panel color correction block
> * @DPU_DSPP_GC Gamma correction block
> + * @DPU_DSPP_IGC Inverse gamma correction block
> */
> enum {
> DPU_DSPP_PCC = 0x1,
> DPU_DSPP_GC,
> + DPU_DSPP_IGC,
> DPU_DSPP_MAX
> };
>
> @@ -191,6 +193,7 @@ enum {
> * @DPU_CTL_SPLIT_DISPLAY: CTL supports video mode split display
> * @DPU_CTL_FETCH_ACTIVE: Active CTL for fetch HW (SSPPs)
> * @DPU_CTL_VM_CFG: CTL config to support multiple VMs
> + * @DPU_CTL_DSPP_BLOCK_FLUSH CTL config to support dspp sub-block flush
The above uses tabs, why does this use spaces?
> * @DPU_CTL_MAX
> */
> enum {
> @@ -198,6 +201,7 @@ enum {
> DPU_CTL_ACTIVE_CFG,
> DPU_CTL_FETCH_ACTIVE,
> DPU_CTL_VM_CFG,
> + DPU_CTL_DSPP_SUB_BLOCK_FLUSH,
> DPU_CTL_MAX
> };
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> index a35ecb6..e801be1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> @@ -33,6 +33,7 @@
> #define CTL_INTF_FLUSH 0x110
> #define CTL_INTF_MASTER 0x134
> #define CTL_FETCH_PIPE_ACTIVE 0x0FC
> +#define CTL_DSPP_n_FLUSH(n) ((0x13C) + ((n) * 4))
>
> #define CTL_MIXER_BORDER_OUT BIT(24)
> #define CTL_FLUSH_MASK_CTL BIT(17)
> @@ -113,6 +114,9 @@ static inline void dpu_hw_ctl_clear_pending_flush(struct dpu_hw_ctl *ctx)
> trace_dpu_hw_ctl_clear_pending_flush(ctx->pending_flush_mask,
> dpu_hw_ctl_get_flush_register(ctx));
> ctx->pending_flush_mask = 0x0;
> +
> + memset(ctx->pending_dspp_flush_mask, 0,
> + sizeof(ctx->pending_dspp_flush_mask));
> }
>
> static inline void dpu_hw_ctl_update_pending_flush(struct dpu_hw_ctl *ctx,
> @@ -130,6 +134,8 @@ static u32 dpu_hw_ctl_get_pending_flush(struct dpu_hw_ctl *ctx)
>
> static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
> {
> + int dspp;
> +
> if (ctx->pending_flush_mask & BIT(MERGE_3D_IDX))
> DPU_REG_WRITE(&ctx->hw, CTL_MERGE_3D_FLUSH,
> ctx->pending_merge_3d_flush_mask);
> @@ -140,6 +146,11 @@ static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
> DPU_REG_WRITE(&ctx->hw, CTL_WB_FLUSH,
> ctx->pending_wb_flush_mask);
>
> + for(dspp = DSPP_0; dspp < DSPP_MAX; dspp++)
Space between for and (?
> + if (ctx->pending_dspp_flush_mask[dspp - DSPP_0])
> + DPU_REG_WRITE(&ctx->hw, CTL_DSPP_n_FLUSH(dspp - DSPP_0),
> + ctx->pending_dspp_flush_mask[dspp - DSPP_0]);
Shouldn't this loop as a whole check if _any_ DSPP flush is requested
via `pending_flush_mask & BIT(29)`? The other flushes don't check the
per-block mask value either (and could write zero that way) but only
base this check on the presence of a global flush mask for that block.
> +
> DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
> }
>
> @@ -287,8 +298,9 @@ static void dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx,
> }
>
> static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx,
> - enum dpu_dspp dspp)
> + enum dpu_dspp dspp, u32 dspp_sub_blk)
> {
> +
Empty line needed for?
> switch (dspp) {
> case DSPP_0:
> ctx->pending_flush_mask |= BIT(13);
> @@ -307,6 +319,30 @@ static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx,
> }
> }
>
> +static void dpu_hw_ctl_update_pending_flush_dspp_subblocks(
> + struct dpu_hw_ctl *ctx, enum dpu_dspp dspp, u32 dspp_sub_blk)
> +{
> +
And here?
> + if (dspp >= DSPP_MAX)
> + return;
> +
> + switch (dspp_sub_blk) {
> + case DPU_DSPP_IGC:
> + ctx->pending_dspp_flush_mask[dspp - DSPP_0] |= BIT(2);
> + break;
> + case DPU_DSPP_PCC:
> + ctx->pending_dspp_flush_mask[dspp - DSPP_0] |= BIT(4);
> + break;
> + case DPU_DSPP_GC:
> + ctx->pending_dspp_flush_mask[dspp - DSPP_0] |= BIT(5);
> + break;
> + default:
> + return;
> + }
> +
> + ctx->pending_flush_mask |= BIT(29);
Can/should we define an _IDX for this, like is done with MERGE_3D_IDX,
DSC_IDX, INTF_IDX and WB_IDX?
> +}
> +
> static u32 dpu_hw_ctl_poll_reset_status(struct dpu_hw_ctl *ctx, u32 timeout_us)
> {
> struct dpu_hw_blk_reg_map *c = &ctx->hw;
> @@ -675,7 +711,11 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops,
> ops->setup_blendstage = dpu_hw_ctl_setup_blendstage;
> ops->update_pending_flush_sspp = dpu_hw_ctl_update_pending_flush_sspp;
> ops->update_pending_flush_mixer = dpu_hw_ctl_update_pending_flush_mixer;
> - ops->update_pending_flush_dspp = dpu_hw_ctl_update_pending_flush_dspp;
> + if (cap & BIT(DPU_CTL_DSPP_SUB_BLOCK_FLUSH))
> + ops->update_pending_flush_dspp = dpu_hw_ctl_update_pending_flush_dspp_subblocks;
> + else
> + ops->update_pending_flush_dspp = dpu_hw_ctl_update_pending_flush_dspp;
> +
> if (cap & BIT(DPU_CTL_FETCH_ACTIVE))
> ops->set_active_pipes = dpu_hw_ctl_set_fetch_pipe_active;
> };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> index 96c012e..78611a8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> @@ -152,9 +152,11 @@ struct dpu_hw_ctl_ops {
> * No effect on hardware
> * @ctx : ctl path ctx pointer
> * @blk : DSPP block index
> + * @dspp_sub_blk : DSPP sub-block index
> */
> void (*update_pending_flush_dspp)(struct dpu_hw_ctl *ctx,
> - enum dpu_dspp blk);
> + enum dpu_dspp blk, u32 dspp_sub_blk);
> +
> /**
> * Write the value of the pending_flush_mask to hardware
> * @ctx : ctl path ctx pointer
> @@ -242,6 +244,7 @@ struct dpu_hw_ctl {
> u32 pending_intf_flush_mask;
> u32 pending_wb_flush_mask;
> u32 pending_merge_3d_flush_mask;
> + u32 pending_dspp_flush_mask[DSPP_MAX - DSPP_0];
>
> /* ops */
> struct dpu_hw_ctl_ops ops;
> --
> 2.7.4
>
>-----Original Message-----
>From: Marijn Suijten <marijn.suijten@somainline.org>
>Sent: Wednesday, December 7, 2022 7:39 PM
>To: Kalyan Thota (QUIC) <quic_kalyant@quicinc.com>
>Cc: dri-devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org;
>freedreno@lists.freedesktop.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; robdclark@chromium.org; dianders@chromium.org;
>swboyd@chromium.org; Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>;
>dmitry.baryshkov@linaro.org; Abhinav Kumar (QUIC)
><quic_abhinavk@quicinc.com>
>Subject: Re: [v10] drm/msm/disp/dpu1: add support for dspp sub block flush in
>sc7280
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of
>any links or attachments, and do not enable macros.
>
>On 2022-12-07 04:59:23, Kalyan Thota wrote:
>> Flush mechanism for DSPP blocks has changed in sc7280 family, it
>> allows individual sub blocks to be flushed in coordination with master
>> flush control.
>>
>> Representation: master_flush && (PCC_flush | IGC_flush .. etc )
>>
>> This change adds necessary support for the above design.
>>
>> Changes in v1:
>> - Few nits (Doug, Dmitry)
>> - Restrict sub-block flush programming to dpu_hw_ctl file (Dmitry)
>>
>> Changes in v2:
>> - Move the address offset to flush macro (Dmitry)
>> - Seperate ops for the sub block flush (Dmitry)
>>
>> Changes in v3:
>> - Reuse the DPU_DSPP_xx enum instead of a new one (Dmitry)
>>
>> Changes in v4:
>> - Use shorter version for unsigned int (Stephen)
>>
>> Changes in v5:
>> - Spurious patch please ignore.
>>
>> Changes in v6:
>> - Add SOB tag (Doug, Dmitry)
>>
>> Changes in v7:
>> - Cache flush mask per dspp (Dmitry)
>> - Few nits (Marijn)
>>
>> Changes in v8:
>> - Few nits (Marijn)
>>
>> Changes in v9:
>> - use DSPP enum while accessing flush mask to make it readable
>> (Dmitry)
>> - Few nits (Dmitry)
>>
>> Changes in v10:
>> - fix white spaces in a seperate patch (Dmitry)
>>
>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 5 ++-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 +++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 44
>++++++++++++++++++++++++--
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 5 ++-
>> 5 files changed, 55 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 601d687..4170fbe 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -766,7 +766,7 @@ static void _dpu_crtc_setup_cp_blocks(struct
>> drm_crtc *crtc)
>>
>> /* stage config flush mask */
>> ctl->ops.update_pending_flush_dspp(ctl,
>> - mixer[i].hw_dspp->idx);
>> + mixer[i].hw_dspp->idx, DPU_DSPP_PCC);
>> }
>> }
>>
>> 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 27f029f..0eecb2f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -65,7 +65,10 @@
>> (PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2))
>>
>> #define CTL_SC7280_MASK \
>> - (BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_FETCH_ACTIVE) |
>BIT(DPU_CTL_VM_CFG))
>> + (BIT(DPU_CTL_ACTIVE_CFG) | \
>> + BIT(DPU_CTL_FETCH_ACTIVE) | \
>> + BIT(DPU_CTL_VM_CFG) | \
>> + BIT(DPU_CTL_DSPP_SUB_BLOCK_FLUSH))
>>
>> #define MERGE_3D_SM8150_MASK (0)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> index 38aa38a..126ee37 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -161,10 +161,12 @@ enum {
>> * DSPP sub-blocks
>> * @DPU_DSPP_PCC Panel color correction block
>> * @DPU_DSPP_GC Gamma correction block
>> + * @DPU_DSPP_IGC Inverse gamma correction block
>> */
>> enum {
>> DPU_DSPP_PCC = 0x1,
>> DPU_DSPP_GC,
>> + DPU_DSPP_IGC,
>> DPU_DSPP_MAX
>> };
>>
>> @@ -191,6 +193,7 @@ enum {
>> * @DPU_CTL_SPLIT_DISPLAY: CTL supports video mode split display
>> * @DPU_CTL_FETCH_ACTIVE: Active CTL for fetch HW (SSPPs)
>> * @DPU_CTL_VM_CFG: CTL config to support multiple VMs
>> + * @DPU_CTL_DSPP_BLOCK_FLUSH CTL config to support dspp sub-block
>> + flush
>
>The above uses tabs, why does this use spaces?
>
>> * @DPU_CTL_MAX
>> */
>> enum {
>> @@ -198,6 +201,7 @@ enum {
>> DPU_CTL_ACTIVE_CFG,
>> DPU_CTL_FETCH_ACTIVE,
>> DPU_CTL_VM_CFG,
>> + DPU_CTL_DSPP_SUB_BLOCK_FLUSH,
>> DPU_CTL_MAX
>> };
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> index a35ecb6..e801be1 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> @@ -33,6 +33,7 @@
>> #define CTL_INTF_FLUSH 0x110
>> #define CTL_INTF_MASTER 0x134
>> #define CTL_FETCH_PIPE_ACTIVE 0x0FC
>> +#define CTL_DSPP_n_FLUSH(n) ((0x13C) + ((n) * 4))
>>
>> #define CTL_MIXER_BORDER_OUT BIT(24)
>> #define CTL_FLUSH_MASK_CTL BIT(17)
>> @@ -113,6 +114,9 @@ static inline void
>dpu_hw_ctl_clear_pending_flush(struct dpu_hw_ctl *ctx)
>> trace_dpu_hw_ctl_clear_pending_flush(ctx->pending_flush_mask,
>> dpu_hw_ctl_get_flush_register(ctx));
>> ctx->pending_flush_mask = 0x0;
>> +
>> + memset(ctx->pending_dspp_flush_mask, 0,
>> + sizeof(ctx->pending_dspp_flush_mask));
>> }
>>
>> static inline void dpu_hw_ctl_update_pending_flush(struct dpu_hw_ctl
>> *ctx, @@ -130,6 +134,8 @@ static u32
>> dpu_hw_ctl_get_pending_flush(struct dpu_hw_ctl *ctx)
>>
>> static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl
>> *ctx) {
>> + int dspp;
>> +
>> if (ctx->pending_flush_mask & BIT(MERGE_3D_IDX))
>> DPU_REG_WRITE(&ctx->hw, CTL_MERGE_3D_FLUSH,
>> ctx->pending_merge_3d_flush_mask);
>> @@ -140,6 +146,11 @@ static inline void dpu_hw_ctl_trigger_flush_v1(struct
>dpu_hw_ctl *ctx)
>> DPU_REG_WRITE(&ctx->hw, CTL_WB_FLUSH,
>> ctx->pending_wb_flush_mask);
>>
>> + for(dspp = DSPP_0; dspp < DSPP_MAX; dspp++)
>
>Space between for and (?
>
>> + if (ctx->pending_dspp_flush_mask[dspp - DSPP_0])
>> + DPU_REG_WRITE(&ctx->hw, CTL_DSPP_n_FLUSH(dspp - DSPP_0),
>> + ctx->pending_dspp_flush_mask[dspp -
>> + DSPP_0]);
>
>Shouldn't this loop as a whole check if _any_ DSPP flush is requested via
>`pending_flush_mask & BIT(29)`? The other flushes don't check the per-block
>mask value either (and could write zero that way) but only base this check on the
>presence of a global flush mask for that block.
>
BIT(29) enables dspp flush only from DPU rev 7.x.x where hierarchal flush is introduced. For other targets that supports CTL_ACTIVE, it's a NOP.
With the check "pending_flush_mask & BIT(29)", unintended DSPP registers for that CTL path will be programmed to "0" which is not correct IMO.
Secondly "pending_flush_mask & BIT(29)" although will not be true for DPU 6.x.x versions but can be confusing w.r.t code readability.
Let me know your thoughts.
>> +
>> DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask); }
>>
>> @@ -287,8 +298,9 @@ static void
>> dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx, }
>>
>> static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx,
>> - enum dpu_dspp dspp)
>> + enum dpu_dspp dspp, u32 dspp_sub_blk)
>> {
>> +
>
>Empty line needed for?
>
>> switch (dspp) {
>> case DSPP_0:
>> ctx->pending_flush_mask |= BIT(13); @@ -307,6 +319,30 @@
>> static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx,
>> }
>> }
>>
>> +static void dpu_hw_ctl_update_pending_flush_dspp_subblocks(
>> + struct dpu_hw_ctl *ctx, enum dpu_dspp dspp, u32 dspp_sub_blk) {
>> +
>
>And here?
>
>> + if (dspp >= DSPP_MAX)
>> + return;
>> +
>> + switch (dspp_sub_blk) {
>> + case DPU_DSPP_IGC:
>> + ctx->pending_dspp_flush_mask[dspp - DSPP_0] |= BIT(2);
>> + break;
>> + case DPU_DSPP_PCC:
>> + ctx->pending_dspp_flush_mask[dspp - DSPP_0] |= BIT(4);
>> + break;
>> + case DPU_DSPP_GC:
>> + ctx->pending_dspp_flush_mask[dspp - DSPP_0] |= BIT(5);
>> + break;
>> + default:
>> + return;
>> + }
>> +
>> + ctx->pending_flush_mask |= BIT(29);
>
>Can/should we define an _IDX for this, like is done with MERGE_3D_IDX,
>DSC_IDX, INTF_IDX and WB_IDX?
>
>> +}
>> +
>> static u32 dpu_hw_ctl_poll_reset_status(struct dpu_hw_ctl *ctx, u32
>> timeout_us) {
>> struct dpu_hw_blk_reg_map *c = &ctx->hw; @@ -675,7 +711,11 @@
>> static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops,
>> ops->setup_blendstage = dpu_hw_ctl_setup_blendstage;
>> ops->update_pending_flush_sspp =
>dpu_hw_ctl_update_pending_flush_sspp;
>> ops->update_pending_flush_mixer =
>dpu_hw_ctl_update_pending_flush_mixer;
>> - ops->update_pending_flush_dspp =
>dpu_hw_ctl_update_pending_flush_dspp;
>> + if (cap & BIT(DPU_CTL_DSPP_SUB_BLOCK_FLUSH))
>> + ops->update_pending_flush_dspp =
>dpu_hw_ctl_update_pending_flush_dspp_subblocks;
>> + else
>> + ops->update_pending_flush_dspp =
>> + dpu_hw_ctl_update_pending_flush_dspp;
>> +
>> if (cap & BIT(DPU_CTL_FETCH_ACTIVE))
>> ops->set_active_pipes =
>> dpu_hw_ctl_set_fetch_pipe_active; }; diff --git
>> a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
>> index 96c012e..78611a8 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
>> @@ -152,9 +152,11 @@ struct dpu_hw_ctl_ops {
>> * No effect on hardware
>> * @ctx : ctl path ctx pointer
>> * @blk : DSPP block index
>> + * @dspp_sub_blk : DSPP sub-block index
>> */
>> void (*update_pending_flush_dspp)(struct dpu_hw_ctl *ctx,
>> - enum dpu_dspp blk);
>> + enum dpu_dspp blk, u32 dspp_sub_blk);
>> +
>> /**
>> * Write the value of the pending_flush_mask to hardware
>> * @ctx : ctl path ctx pointer
>> @@ -242,6 +244,7 @@ struct dpu_hw_ctl {
>> u32 pending_intf_flush_mask;
>> u32 pending_wb_flush_mask;
>> u32 pending_merge_3d_flush_mask;
>> + u32 pending_dspp_flush_mask[DSPP_MAX - DSPP_0];
>>
>> /* ops */
>> struct dpu_hw_ctl_ops ops;
>> --
>> 2.7.4
>>
On 2022-12-12 11:35:15, Kalyan Thota wrote:
> [..]
> >> + if (ctx->pending_dspp_flush_mask[dspp - DSPP_0])
> >> + DPU_REG_WRITE(&ctx->hw, CTL_DSPP_n_FLUSH(dspp - DSPP_0),
> >> + ctx->pending_dspp_flush_mask[dspp -
> >> + DSPP_0]);
> >
> >Shouldn't this loop as a whole check if _any_ DSPP flush is requested via
> >`pending_flush_mask & BIT(29)`? The other flushes don't check the per-block
> >mask value either (and could write zero that way) but only base this check on the
> >presence of a global flush mask for that block.
> >
> BIT(29) enables dspp flush only from DPU rev 7.x.x where hierarchal flush is introduced. For other targets that supports CTL_ACTIVE, it's a NOP.
The only way this patch ever writes pending_dspp_flush_mask is followed
by unconditionally setting BIT(29) in pending_flush_mask. I was under
the assumption that pending_dspp_flush_mask should be considered invalid
or irrelevant unless BIT(29) is set.
> With the check "pending_flush_mask & BIT(29)", unintended DSPP registers for that CTL path will be programmed to "0" which is not correct IMO.
You can also keep the second `if` to guard against that; as said the
code above does exactly this though, but I think we could assume that
if a pending sub-block flush is set, pending_dspp_flush_mask is nonzero?
> Secondly "pending_flush_mask & BIT(29)" although will not be true for DPU 6.x.x versions but can be confusing w.r.t code readability.
> Let me know your thoughts.
Ack, it is /super/ confusing that BIT(29) is used for DSPP (sub-block)
flush, but also to flash INTF_2??
In fact there are many overlapping flush bits used for different
components. Only few are clarified via a #define. Can you confirm
whether this is correct? And whether these should all be pulled out
into numerically-sorted defines to improve readability and document
intentional overlap?
- Marijn
@@ -766,7 +766,7 @@ static void _dpu_crtc_setup_cp_blocks(struct drm_crtc *crtc)
/* stage config flush mask */
ctl->ops.update_pending_flush_dspp(ctl,
- mixer[i].hw_dspp->idx);
+ mixer[i].hw_dspp->idx, DPU_DSPP_PCC);
}
}
@@ -65,7 +65,10 @@
(PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2))
#define CTL_SC7280_MASK \
- (BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_FETCH_ACTIVE) | BIT(DPU_CTL_VM_CFG))
+ (BIT(DPU_CTL_ACTIVE_CFG) | \
+ BIT(DPU_CTL_FETCH_ACTIVE) | \
+ BIT(DPU_CTL_VM_CFG) | \
+ BIT(DPU_CTL_DSPP_SUB_BLOCK_FLUSH))
#define MERGE_3D_SM8150_MASK (0)
@@ -161,10 +161,12 @@ enum {
* DSPP sub-blocks
* @DPU_DSPP_PCC Panel color correction block
* @DPU_DSPP_GC Gamma correction block
+ * @DPU_DSPP_IGC Inverse gamma correction block
*/
enum {
DPU_DSPP_PCC = 0x1,
DPU_DSPP_GC,
+ DPU_DSPP_IGC,
DPU_DSPP_MAX
};
@@ -191,6 +193,7 @@ enum {
* @DPU_CTL_SPLIT_DISPLAY: CTL supports video mode split display
* @DPU_CTL_FETCH_ACTIVE: Active CTL for fetch HW (SSPPs)
* @DPU_CTL_VM_CFG: CTL config to support multiple VMs
+ * @DPU_CTL_DSPP_BLOCK_FLUSH CTL config to support dspp sub-block flush
* @DPU_CTL_MAX
*/
enum {
@@ -198,6 +201,7 @@ enum {
DPU_CTL_ACTIVE_CFG,
DPU_CTL_FETCH_ACTIVE,
DPU_CTL_VM_CFG,
+ DPU_CTL_DSPP_SUB_BLOCK_FLUSH,
DPU_CTL_MAX
};
@@ -33,6 +33,7 @@
#define CTL_INTF_FLUSH 0x110
#define CTL_INTF_MASTER 0x134
#define CTL_FETCH_PIPE_ACTIVE 0x0FC
+#define CTL_DSPP_n_FLUSH(n) ((0x13C) + ((n) * 4))
#define CTL_MIXER_BORDER_OUT BIT(24)
#define CTL_FLUSH_MASK_CTL BIT(17)
@@ -113,6 +114,9 @@ static inline void dpu_hw_ctl_clear_pending_flush(struct dpu_hw_ctl *ctx)
trace_dpu_hw_ctl_clear_pending_flush(ctx->pending_flush_mask,
dpu_hw_ctl_get_flush_register(ctx));
ctx->pending_flush_mask = 0x0;
+
+ memset(ctx->pending_dspp_flush_mask, 0,
+ sizeof(ctx->pending_dspp_flush_mask));
}
static inline void dpu_hw_ctl_update_pending_flush(struct dpu_hw_ctl *ctx,
@@ -130,6 +134,8 @@ static u32 dpu_hw_ctl_get_pending_flush(struct dpu_hw_ctl *ctx)
static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
{
+ int dspp;
+
if (ctx->pending_flush_mask & BIT(MERGE_3D_IDX))
DPU_REG_WRITE(&ctx->hw, CTL_MERGE_3D_FLUSH,
ctx->pending_merge_3d_flush_mask);
@@ -140,6 +146,11 @@ static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
DPU_REG_WRITE(&ctx->hw, CTL_WB_FLUSH,
ctx->pending_wb_flush_mask);
+ for(dspp = DSPP_0; dspp < DSPP_MAX; dspp++)
+ if (ctx->pending_dspp_flush_mask[dspp - DSPP_0])
+ DPU_REG_WRITE(&ctx->hw, CTL_DSPP_n_FLUSH(dspp - DSPP_0),
+ ctx->pending_dspp_flush_mask[dspp - DSPP_0]);
+
DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
}
@@ -287,8 +298,9 @@ static void dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx,
}
static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx,
- enum dpu_dspp dspp)
+ enum dpu_dspp dspp, u32 dspp_sub_blk)
{
+
switch (dspp) {
case DSPP_0:
ctx->pending_flush_mask |= BIT(13);
@@ -307,6 +319,30 @@ static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx,
}
}
+static void dpu_hw_ctl_update_pending_flush_dspp_subblocks(
+ struct dpu_hw_ctl *ctx, enum dpu_dspp dspp, u32 dspp_sub_blk)
+{
+
+ if (dspp >= DSPP_MAX)
+ return;
+
+ switch (dspp_sub_blk) {
+ case DPU_DSPP_IGC:
+ ctx->pending_dspp_flush_mask[dspp - DSPP_0] |= BIT(2);
+ break;
+ case DPU_DSPP_PCC:
+ ctx->pending_dspp_flush_mask[dspp - DSPP_0] |= BIT(4);
+ break;
+ case DPU_DSPP_GC:
+ ctx->pending_dspp_flush_mask[dspp - DSPP_0] |= BIT(5);
+ break;
+ default:
+ return;
+ }
+
+ ctx->pending_flush_mask |= BIT(29);
+}
+
static u32 dpu_hw_ctl_poll_reset_status(struct dpu_hw_ctl *ctx, u32 timeout_us)
{
struct dpu_hw_blk_reg_map *c = &ctx->hw;
@@ -675,7 +711,11 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops,
ops->setup_blendstage = dpu_hw_ctl_setup_blendstage;
ops->update_pending_flush_sspp = dpu_hw_ctl_update_pending_flush_sspp;
ops->update_pending_flush_mixer = dpu_hw_ctl_update_pending_flush_mixer;
- ops->update_pending_flush_dspp = dpu_hw_ctl_update_pending_flush_dspp;
+ if (cap & BIT(DPU_CTL_DSPP_SUB_BLOCK_FLUSH))
+ ops->update_pending_flush_dspp = dpu_hw_ctl_update_pending_flush_dspp_subblocks;
+ else
+ ops->update_pending_flush_dspp = dpu_hw_ctl_update_pending_flush_dspp;
+
if (cap & BIT(DPU_CTL_FETCH_ACTIVE))
ops->set_active_pipes = dpu_hw_ctl_set_fetch_pipe_active;
};
@@ -152,9 +152,11 @@ struct dpu_hw_ctl_ops {
* No effect on hardware
* @ctx : ctl path ctx pointer
* @blk : DSPP block index
+ * @dspp_sub_blk : DSPP sub-block index
*/
void (*update_pending_flush_dspp)(struct dpu_hw_ctl *ctx,
- enum dpu_dspp blk);
+ enum dpu_dspp blk, u32 dspp_sub_blk);
+
/**
* Write the value of the pending_flush_mask to hardware
* @ctx : ctl path ctx pointer
@@ -242,6 +244,7 @@ struct dpu_hw_ctl {
u32 pending_intf_flush_mask;
u32 pending_wb_flush_mask;
u32 pending_merge_3d_flush_mask;
+ u32 pending_dspp_flush_mask[DSPP_MAX - DSPP_0];
/* ops */
struct dpu_hw_ctl_ops ops;