| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <01e32ce9-8d6f-0bd1-fe12-b4ab575e8aa6@linaro.org>
Date: Wed, 9 Nov 2022 15:40:04 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Kalyan Thota <quic_kalyant@...cinc.com>,
dri-devel@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
freedreno@...ts.freedesktop.org, devicetree@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, robdclark@...omium.org,
dianders@...omium.org, swboyd@...omium.org,
quic_vpolimer@...cinc.com, quic_abhinavk@...cinc.com
Subject: Re: [v8] drm/msm/disp/dpu1: add support for dspp sub block flush in
sc7280
On 09/11/2022 15:14, 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)
>
> Signed-off-by: Kalyan Thota <quic_kalyant@...cinc.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 | 12 +++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 66 +++++++++++++++++++++-----
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 7 ++-
> 5 files changed, 72 insertions(+), 20 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..35f4810 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
> };
>
> @@ -188,16 +190,18 @@ enum {
>
> /**
> * CTL sub-blocks
> - * @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_MAX
> + * @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 Maximum value
No unnecessary whitespace changes please.
> */
> enum {
> DPU_CTL_SPLIT_DISPLAY = 0x1,
> 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..29821ea 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> @@ -28,22 +28,23 @@
> #define CTL_INTF_ACTIVE 0x0F4
> #define CTL_MERGE_3D_FLUSH 0x100
> #define CTL_DSC_ACTIVE 0x0E8
> -#define CTL_DSC_FLUSH 0x104
> +#define CTL_DSC_FLUSH 0x104
> #define CTL_WB_FLUSH 0x108
> #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)
> +#define CTL_MIXER_BORDER_OUT BIT(24)
> +#define CTL_FLUSH_MASK_CTL BIT(17)
>
> -#define DPU_REG_RESET_TIMEOUT_US 2000
> -#define MERGE_3D_IDX 23
> -#define DSC_IDX 22
> -#define INTF_IDX 31
> -#define WB_IDX 16
> -#define CTL_INVALID_BIT 0xffff
> -#define CTL_DEFAULT_GROUP_ID 0xf
> +#define DPU_REG_RESET_TIMEOUT_US 2000
> +#define MERGE_3D_IDX 23
> +#define DSC_IDX 22
> +#define INTF_IDX 31
> +#define WB_IDX 16
> +#define CTL_INVALID_BIT 0xffff
> +#define CTL_DEFAULT_GROUP_ID 0xf
>
> static const u32 fetch_tbl[SSPP_MAX] = {CTL_INVALID_BIT, 16, 17, 18, 19,
> CTL_INVALID_BIT, CTL_INVALID_BIT, CTL_INVALID_BIT, CTL_INVALID_BIT, 0,
> @@ -110,9 +111,14 @@ static inline void dpu_hw_ctl_trigger_pending(struct dpu_hw_ctl *ctx)
>
> static inline void dpu_hw_ctl_clear_pending_flush(struct dpu_hw_ctl *ctx)
> {
> + int i;
> +
> trace_dpu_hw_ctl_clear_pending_flush(ctx->pending_flush_mask,
> dpu_hw_ctl_get_flush_register(ctx));
> ctx->pending_flush_mask = 0x0;
> +
> + for(i = 0; i < ARRAY_SIZE(ctx->pending_dspp_flush_mask); i++)
> + ctx->pending_dspp_flush_mask[i] = 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 +136,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 i;
> +
> 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 +148,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(i = 0; i < ARRAY_SIZE(ctx->pending_dspp_flush_mask); i++)
> + if (ctx->pending_dspp_flush_mask[i])
> + DPU_REG_WRITE(&ctx->hw, CTL_DSPP_n_FLUSH(i),
> + ctx->pending_dspp_flush_mask[i]);
> +
I'm somewhat biased towards:
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),
ctx->pending_dspp_flush_mask[dspp]);
WDYT? Yes, you'd have to adjust CTL_DSPP_n_FLUSH to subtract DSPP_0 from
the passed value.
> DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
> }
>
> @@ -287,8 +300,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 +321,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 +713,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..ff4e92c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> @@ -148,13 +148,15 @@ struct dpu_hw_ctl_ops {
> enum dpu_lm blk);
>
> /**
> - * OR in the given flushbits to the cached pending_flush_mask
> + * OR in the given flushbits to the cached pending_dspp_flush_mask
The existence of pending_dspp_flush is a detail. For earlier revisions
there is no such mask. Thus I'd suggest leaving the comment intact.
> * 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;
--
With best wishes
Dmitry
Powered by blists - more mailing lists