RE: [v10] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280

[Kalyan Thota <kalyant@qti.qualcomm.com>]

>-----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
>>