| Message ID | 1684360919-28458-7-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add DSC 1.2 dpu supports | expand |
On 2023-05-17 15:01:57, Kuogee Hsieh wrote: > Currently DSC flushing happens during interface configuration at > dpu_hw_ctl_intf_cfg_v1(). Separate DSC flush away from > dpu_hw_ctl_intf_cfg_v1() by adding dpu_hw_ctl_update_pending_flush_dsc_v1() > to handle both per-DSC engine and DSC flush bits at same time to make it > consistent with the location of flush programming of other DPU sub-blocks. > > Changes in v10: > -- rewording commit text > -- pass ctl directly instead of dpu_enc to dsc_pipe_cfg() There are a few things missing from v8 review, see below. > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 10 ++++++++-- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 22 ++++++++++++++++------ > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 13 +++++++++++++ > 3 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index ffa6f04..1957545 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1834,7 +1834,8 @@ dpu_encoder_dsc_initial_line_calc(struct drm_dsc_config *dsc, > return DIV_ROUND_UP(total_pixels, dsc->slice_width); > } > > -static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, > +static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_ctl *ctl, > + struct dpu_hw_dsc *hw_dsc, > struct dpu_hw_pingpong *hw_pp, > struct drm_dsc_config *dsc, > u32 common_mode, > @@ -1854,6 +1855,9 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, > > if (hw_pp->ops.enable_dsc) > hw_pp->ops.enable_dsc(hw_pp); > + > + if (ctl->ops.update_pending_flush_dsc) > + ctl->ops.update_pending_flush_dsc(ctl, hw_dsc->idx); > } > > static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, > @@ -1861,6 +1865,7 @@ static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, > { > /* coding only for 2LM, 2enc, 1 dsc config */ > struct dpu_encoder_phys *enc_master = dpu_enc->cur_master; > + struct dpu_hw_ctl *ctl = enc_master->hw_ctl; > struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC]; > struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC]; > int this_frame_slices; > @@ -1898,7 +1903,8 @@ static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, > initial_lines = dpu_encoder_dsc_initial_line_calc(dsc, enc_ip_w); > > for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) > - dpu_encoder_dsc_pipe_cfg(hw_dsc[i], hw_pp[i], dsc, dsc_common_mode, initial_lines); > + dpu_encoder_dsc_pipe_cfg(ctl, hw_dsc[i], hw_pp[i], dsc, > + dsc_common_mode, initial_lines); > } > > void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) > 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 4f7cfa9..4e132d9 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > @@ -139,6 +139,11 @@ static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) > CTL_DSPP_n_FLUSH(dspp - DSPP_0), > ctx->pending_dspp_flush_mask[dspp - DSPP_0]); > } > + > + if (ctx->pending_flush_mask & BIT(DSC_IDX)) > + DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, > + ctx->pending_dsc_flush_mask); Again, when do we reset this mask to 0? (v8 review) > + > DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask); > } > > @@ -285,6 +290,13 @@ static void dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx, > ctx->pending_flush_mask |= BIT(MERGE_3D_IDX); > } > > +static void dpu_hw_ctl_update_pending_flush_dsc_v1(struct dpu_hw_ctl *ctx, > + enum dpu_dsc dsc_num) > +{ > + ctx->pending_dsc_flush_mask |= BIT(dsc_num - DSC_0); > + ctx->pending_flush_mask |= BIT(DSC_IDX); > +} > + > static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx, > enum dpu_dspp dspp, u32 dspp_sub_blk) > { > @@ -502,9 +514,6 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, > if ((test_bit(DPU_CTL_VM_CFG, &ctx->caps->features))) > mode_sel = CTL_DEFAULT_GROUP_ID << 28; > > - if (cfg->dsc) > - DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, cfg->dsc); > - > if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD) > mode_sel |= BIT(17); > > @@ -524,10 +533,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, > if (cfg->merge_3d) > DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, > BIT(cfg->merge_3d - MERGE_3D_0)); > - if (cfg->dsc) { > - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); Again, this bugfix of now wrapping DSC_IDX in BIT() should go in a separate Fixes: patch to have this semantic change documented. (v8 review) > + > + if (cfg->dsc) > DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); > - } > } > > static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, > @@ -630,6 +638,8 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops, > ops->update_pending_flush_merge_3d = > dpu_hw_ctl_update_pending_flush_merge_3d_v1; > ops->update_pending_flush_wb = dpu_hw_ctl_update_pending_flush_wb_v1; > + ops->update_pending_flush_dsc = > + dpu_hw_ctl_update_pending_flush_dsc_v1; > } else { > ops->trigger_flush = dpu_hw_ctl_trigger_flush; > ops->setup_intf_cfg = dpu_hw_ctl_intf_cfg; > 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 6292002..d5f3ef8 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > @@ -158,6 +158,15 @@ struct dpu_hw_ctl_ops { > enum dpu_dspp blk, u32 dspp_sub_blk); > > /** > + * OR in the given flushbits to the cached pending_(dsc_)flush_mask > + * No effect on hardware > + * @ctx: ctl path ctx pointer > + * @blk: interface block index > + */ > + void (*update_pending_flush_dsc)(struct dpu_hw_ctl *ctx, > + enum dpu_dsc blk); > + > + /** > * Write the value of the pending_flush_mask to hardware > * @ctx : ctl path ctx pointer > */ > @@ -229,6 +238,9 @@ struct dpu_hw_ctl_ops { > * @pending_flush_mask: storage for pending ctl_flush managed via ops > * @pending_intf_flush_mask: pending INTF flush > * @pending_wb_flush_mask: pending WB flush The above is all capitalized, so...: > + * @pending_merge_3d_flush_mask: pending merge_3d flush MERGE_3D? > + * @pending_dspp_flush_mask: pending dspp flush DSPP > + * @pending_dsc_flush_mask: pending dsc flush DSC - Marijn > * @ops: operation list > */ > struct dpu_hw_ctl { > @@ -245,6 +257,7 @@ struct dpu_hw_ctl { > u32 pending_wb_flush_mask; > u32 pending_merge_3d_flush_mask; > u32 pending_dspp_flush_mask[DSPP_MAX - DSPP_0]; > + u32 pending_dsc_flush_mask; > > /* ops */ > struct dpu_hw_ctl_ops ops; > -- > 2.7.4 >
On 18/05/2023 01:01, Kuogee Hsieh wrote: > Currently DSC flushing happens during interface configuration at > dpu_hw_ctl_intf_cfg_v1(). Separate DSC flush away from > dpu_hw_ctl_intf_cfg_v1() by adding dpu_hw_ctl_update_pending_flush_dsc_v1() > to handle both per-DSC engine and DSC flush bits at same time to make it > consistent with the location of flush programming of other DPU sub-blocks. > > Changes in v10: > -- rewording commit text > -- pass ctl directly instead of dpu_enc to dsc_pipe_cfg() > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 10 ++++++++-- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 22 ++++++++++++++++------ > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 13 +++++++++++++ > 3 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index ffa6f04..1957545 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1834,7 +1834,8 @@ dpu_encoder_dsc_initial_line_calc(struct drm_dsc_config *dsc, > return DIV_ROUND_UP(total_pixels, dsc->slice_width); > } > > -static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, > +static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_ctl *ctl, > + struct dpu_hw_dsc *hw_dsc, > struct dpu_hw_pingpong *hw_pp, > struct drm_dsc_config *dsc, > u32 common_mode, > @@ -1854,6 +1855,9 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, > > if (hw_pp->ops.enable_dsc) > hw_pp->ops.enable_dsc(hw_pp); > + > + if (ctl->ops.update_pending_flush_dsc) > + ctl->ops.update_pending_flush_dsc(ctl, hw_dsc->idx); > } > > static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, > @@ -1861,6 +1865,7 @@ static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, > { > /* coding only for 2LM, 2enc, 1 dsc config */ > struct dpu_encoder_phys *enc_master = dpu_enc->cur_master; > + struct dpu_hw_ctl *ctl = enc_master->hw_ctl; > struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC]; > struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC]; > int this_frame_slices; > @@ -1898,7 +1903,8 @@ static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, > initial_lines = dpu_encoder_dsc_initial_line_calc(dsc, enc_ip_w); > > for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) > - dpu_encoder_dsc_pipe_cfg(hw_dsc[i], hw_pp[i], dsc, dsc_common_mode, initial_lines); > + dpu_encoder_dsc_pipe_cfg(ctl, hw_dsc[i], hw_pp[i], dsc, > + dsc_common_mode, initial_lines); Moving dsc to the next line would be more logical. > } > > void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) > 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 4f7cfa9..4e132d9 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > @@ -139,6 +139,11 @@ static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) > CTL_DSPP_n_FLUSH(dspp - DSPP_0), > ctx->pending_dspp_flush_mask[dspp - DSPP_0]); > } > + > + if (ctx->pending_flush_mask & BIT(DSC_IDX)) > + DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, > + ctx->pending_dsc_flush_mask); > + > DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask); > } > > @@ -285,6 +290,13 @@ static void dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx, > ctx->pending_flush_mask |= BIT(MERGE_3D_IDX); > } > > +static void dpu_hw_ctl_update_pending_flush_dsc_v1(struct dpu_hw_ctl *ctx, > + enum dpu_dsc dsc_num) > +{ > + ctx->pending_dsc_flush_mask |= BIT(dsc_num - DSC_0); > + ctx->pending_flush_mask |= BIT(DSC_IDX); > +} > + > static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx, > enum dpu_dspp dspp, u32 dspp_sub_blk) > { > @@ -502,9 +514,6 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, > if ((test_bit(DPU_CTL_VM_CFG, &ctx->caps->features))) > mode_sel = CTL_DEFAULT_GROUP_ID << 28; > > - if (cfg->dsc) > - DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, cfg->dsc); > - > if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD) > mode_sel |= BIT(17); > > @@ -524,10 +533,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, > if (cfg->merge_3d) > DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, > BIT(cfg->merge_3d - MERGE_3D_0)); > - if (cfg->dsc) { > - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > + > + if (cfg->dsc) > DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); > - } > } > > static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, > @@ -630,6 +638,8 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops, > ops->update_pending_flush_merge_3d = > dpu_hw_ctl_update_pending_flush_merge_3d_v1; > ops->update_pending_flush_wb = dpu_hw_ctl_update_pending_flush_wb_v1; > + ops->update_pending_flush_dsc = > + dpu_hw_ctl_update_pending_flush_dsc_v1; > } else { > ops->trigger_flush = dpu_hw_ctl_trigger_flush; > ops->setup_intf_cfg = dpu_hw_ctl_intf_cfg; > 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 6292002..d5f3ef8 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > @@ -158,6 +158,15 @@ struct dpu_hw_ctl_ops { > enum dpu_dspp blk, u32 dspp_sub_blk); > > /** > + * OR in the given flushbits to the cached pending_(dsc_)flush_mask > + * No effect on hardware > + * @ctx: ctl path ctx pointer > + * @blk: interface block index > + */ > + void (*update_pending_flush_dsc)(struct dpu_hw_ctl *ctx, > + enum dpu_dsc blk); Please align to the opening parenthesis. > + > + /** > * Write the value of the pending_flush_mask to hardware > * @ctx : ctl path ctx pointer > */ > @@ -229,6 +238,9 @@ struct dpu_hw_ctl_ops { > * @pending_flush_mask: storage for pending ctl_flush managed via ops > * @pending_intf_flush_mask: pending INTF flush > * @pending_wb_flush_mask: pending WB flush > + * @pending_merge_3d_flush_mask: pending merge_3d flush > + * @pending_dspp_flush_mask: pending dspp flush These two can go to a separate patch. > + * @pending_dsc_flush_mask: pending dsc flush > * @ops: operation list > */ > struct dpu_hw_ctl { > @@ -245,6 +257,7 @@ struct dpu_hw_ctl { > u32 pending_wb_flush_mask; > u32 pending_merge_3d_flush_mask; > u32 pending_dspp_flush_mask[DSPP_MAX - DSPP_0]; > + u32 pending_dsc_flush_mask; > > /* ops */ > struct dpu_hw_ctl_ops ops;
On 2023-05-18 03:35:31, Dmitry Baryshkov wrote: <snip> > > 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 6292002..d5f3ef8 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > > @@ -158,6 +158,15 @@ struct dpu_hw_ctl_ops { > > enum dpu_dspp blk, u32 dspp_sub_blk); > > > > /** > > + * OR in the given flushbits to the cached pending_(dsc_)flush_mask > > + * No effect on hardware > > + * @ctx: ctl path ctx pointer > > + * @blk: interface block index > > + */ > > + void (*update_pending_flush_dsc)(struct dpu_hw_ctl *ctx, > > + enum dpu_dsc blk); > > Please align to the opening parenthesis. I requested this change to a single tab specifically to match the rest of the indentation of these callbacks. Perhaps we should submit a followup patch realigning all of them at once (and fixing the doc comments, and and and...). - Marijn > > + > > + /** > > * Write the value of the pending_flush_mask to hardware > > * @ctx : ctl path ctx pointer > > */
On 5/17/2023 3:31 PM, Marijn Suijten wrote: > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> @@ -139,6 +139,11 @@ static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) >> CTL_DSPP_n_FLUSH(dspp - DSPP_0), >> ctx->pending_dspp_flush_mask[dspp - DSPP_0]); >> } >> + >> + if (ctx->pending_flush_mask & BIT(DSC_IDX)) >> + DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, >> + ctx->pending_dsc_flush_mask); > Again, when do we reset this mask to 0? (v8 review) can not find it. let me add a separate patch to fix this. > >> + >> DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask); >> } >> >> @@ -285,6 +290,13 @@ static void dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx, >> ctx->pending_flush_mask |= BIT(MERGE_3D_IDX); >> } >> >> +static void dpu_hw_ctl_update_pending_flush_dsc_v1(struct dpu_hw_ctl *ctx, >> + enum dpu_dsc dsc_num) >> +{ >> + ctx->pending_dsc_flush_mask |= BIT(dsc_num - DSC_0); >> + ctx->pending_flush_mask |= BIT(DSC_IDX); >> +} >> + >> static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx, >> enum dpu_dspp dspp, u32 dspp_sub_blk) >> { >> @@ -502,9 +514,6 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, >> if ((test_bit(DPU_CTL_VM_CFG, &ctx->caps->features))) >> mode_sel = CTL_DEFAULT_GROUP_ID << 28; >> >> - if (cfg->dsc) >> - DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, cfg->dsc); >> - >> if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD) >> mode_sel |= BIT(17); >> >> @@ -524,10 +533,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, >> if (cfg->merge_3d) >> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, >> BIT(cfg->merge_3d - MERGE_3D_0)); >> - if (cfg->dsc) { >> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > Again, this bugfix of now wrapping DSC_IDX in BIT() should go in a > separate Fixes: patch to have this semantic change documented. (v8 > review) That will be this patch. let me add Fixes at this patch > >> + >> + if (cfg->dsc) >> DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >> - } >> } >> >> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, >> @@ -630,6 +638,8 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops, >> ops->update_pending_flush_merge_3d = >> dpu_hw_ctl_update_pending_flush_merge_3d_v1; >> ops->update_pending_flush_wb = dpu_hw_ctl_update_pending_flush_wb_v1; >> + ops->update_pending_flush_dsc = >> + dpu_hw_ctl_update_pending_flush_dsc_v1; >> } else { >> ops->trigger_flush = dpu_hw_ctl_trigger_flush; >> ops->setup_intf_cfg = dpu_hw_ctl_intf_cfg; >> 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 6292002..d5f3ef8 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >> @@ -158,6 +158,15 @@ struct dpu_hw_ctl_ops { >> enum dpu_dspp blk, u32 dspp_sub_blk); >> >> /** >> + * OR in the given flushbits to the cached pending_(dsc_)flush_mask >> + * No effect on hardware >> + * @ctx: ctl path ctx pointer >> + * @blk: interface block index >> + */ >> + void (*update_pending_flush_dsc)(struct dpu_hw_ctl *ctx, >> + enum dpu_dsc blk); >> + >> + /** >> * Write the value of the pending_flush_mask to hardware >> * @ctx : ctl path ctx pointer >> */ >> @@ -229,6 +238,9 @@ struct dpu_hw_ctl_ops { >> * @pending_flush_mask: storage for pending ctl_flush managed via ops >> * @pending_intf_flush_mask: pending INTF flush >> * @pending_wb_flush_mask: pending WB flush > The above is all capitalized, so...: > >> + * @pending_merge_3d_flush_mask: pending merge_3d flush > MERGE_3D? > >> + * @pending_dspp_flush_mask: pending dspp flush > DSPP > >> + * @pending_dsc_flush_mask: pending dsc flush > DSC > > - Marijn > >> * @ops: operation list >> */ >> struct dpu_hw_ctl { >> @@ -245,6 +257,7 @@ struct dpu_hw_ctl { >> u32 pending_wb_flush_mask; >> u32 pending_merge_3d_flush_mask; >> u32 pending_dspp_flush_mask[DSPP_MAX - DSPP_0]; >> + u32 pending_dsc_flush_mask; >> >> /* ops */ >> struct dpu_hw_ctl_ops ops; >> -- >> 2.7.4 >>
On 19/05/2023 01:09, Kuogee Hsieh wrote: > > On 5/17/2023 3:31 PM, Marijn Suijten wrote: >> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>> @@ -139,6 +139,11 @@ static inline void >>> dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) >>> CTL_DSPP_n_FLUSH(dspp - DSPP_0), >>> ctx->pending_dspp_flush_mask[dspp - DSPP_0]); >>> } >>> + >>> + if (ctx->pending_flush_mask & BIT(DSC_IDX)) >>> + DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, >>> + ctx->pending_dsc_flush_mask); >> Again, when do we reset this mask to 0? (v8 review) > > can not find it. > > let me add a separate patch to fix this. The pending_dsc_flush_mask was added in this patch, so the reset should be a part of this patch too. > >> >>> + >>> DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask); >>> } >>> @@ -285,6 +290,13 @@ static void >>> dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx, >>> ctx->pending_flush_mask |= BIT(MERGE_3D_IDX); >>> } >>> +static void dpu_hw_ctl_update_pending_flush_dsc_v1(struct dpu_hw_ctl >>> *ctx, >>> + enum dpu_dsc dsc_num) >>> +{ >>> + ctx->pending_dsc_flush_mask |= BIT(dsc_num - DSC_0); >>> + ctx->pending_flush_mask |= BIT(DSC_IDX); >>> +} >>> + >>> static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl >>> *ctx, >>> enum dpu_dspp dspp, u32 dspp_sub_blk) >>> { >>> @@ -502,9 +514,6 @@ static void dpu_hw_ctl_intf_cfg_v1(struct >>> dpu_hw_ctl *ctx, >>> if ((test_bit(DPU_CTL_VM_CFG, &ctx->caps->features))) >>> mode_sel = CTL_DEFAULT_GROUP_ID << 28; >>> - if (cfg->dsc) >>> - DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, cfg->dsc); >>> - >>> if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD) >>> mode_sel |= BIT(17); >>> @@ -524,10 +533,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct >>> dpu_hw_ctl *ctx, >>> if (cfg->merge_3d) >>> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, >>> BIT(cfg->merge_3d - MERGE_3D_0)); >>> - if (cfg->dsc) { >>> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); >> Again, this bugfix of now wrapping DSC_IDX in BIT() should go in a >> separate Fixes: patch to have this semantic change documented. (v8 >> review) > That will be this patch. let me add Fixes at this patch _separate_ patch. >> >>> + >>> + if (cfg->dsc) >>> DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >>> - } >>> } >>> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, >>> @@ -630,6 +638,8 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops >>> *ops, >>> ops->update_pending_flush_merge_3d = >>> dpu_hw_ctl_update_pending_flush_merge_3d_v1; >>> ops->update_pending_flush_wb = >>> dpu_hw_ctl_update_pending_flush_wb_v1; >>> + ops->update_pending_flush_dsc = >>> + dpu_hw_ctl_update_pending_flush_dsc_v1; >>> } else { >>> ops->trigger_flush = dpu_hw_ctl_trigger_flush; >>> ops->setup_intf_cfg = dpu_hw_ctl_intf_cfg; >>> 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 6292002..d5f3ef8 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >>> @@ -158,6 +158,15 @@ struct dpu_hw_ctl_ops { >>> enum dpu_dspp blk, u32 dspp_sub_blk); >>> /** >>> + * OR in the given flushbits to the cached pending_(dsc_)flush_mask >>> + * No effect on hardware >>> + * @ctx: ctl path ctx pointer >>> + * @blk: interface block index >>> + */ >>> + void (*update_pending_flush_dsc)(struct dpu_hw_ctl *ctx, >>> + enum dpu_dsc blk); >>> + >>> + /** >>> * Write the value of the pending_flush_mask to hardware >>> * @ctx : ctl path ctx pointer >>> */ >>> @@ -229,6 +238,9 @@ struct dpu_hw_ctl_ops { >>> * @pending_flush_mask: storage for pending ctl_flush managed via ops >>> * @pending_intf_flush_mask: pending INTF flush >>> * @pending_wb_flush_mask: pending WB flush >> The above is all capitalized, so...: >> >>> + * @pending_merge_3d_flush_mask: pending merge_3d flush >> MERGE_3D? >> >>> + * @pending_dspp_flush_mask: pending dspp flush >> DSPP >> >>> + * @pending_dsc_flush_mask: pending dsc flush >> DSC >> >> - Marijn >> >>> * @ops: operation list >>> */ >>> struct dpu_hw_ctl { >>> @@ -245,6 +257,7 @@ struct dpu_hw_ctl { >>> u32 pending_wb_flush_mask; >>> u32 pending_merge_3d_flush_mask; >>> u32 pending_dspp_flush_mask[DSPP_MAX - DSPP_0]; >>> + u32 pending_dsc_flush_mask; >>> /* ops */ >>> struct dpu_hw_ctl_ops ops; >>> -- >>> 2.7.4 >>>
On 2023-05-19 01:40:19, Dmitry Baryshkov wrote: > > On 19/05/2023 01:09, Kuogee Hsieh wrote: > > > > On 5/17/2023 3:31 PM, Marijn Suijten wrote: > >> > >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > >>> @@ -139,6 +139,11 @@ static inline void > >>> dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) > >>> CTL_DSPP_n_FLUSH(dspp - DSPP_0), > >>> ctx->pending_dspp_flush_mask[dspp - DSPP_0]); > >>> } > >>> + > >>> + if (ctx->pending_flush_mask & BIT(DSC_IDX)) > >>> + DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, > >>> + ctx->pending_dsc_flush_mask); > >> Again, when do we reset this mask to 0? (v8 review) > > > > can not find it. > > > > let me add a separate patch to fix this. > > The pending_dsc_flush_mask was added in this patch, so the reset should > be a part of this patch too. Yes, same patch. Related question I asked in v8: only the global pending_flush_mask and pending_dspp_flush_mask are reset in dpu_hw_ctl_clear_pending_flush(). Shall I send a patch to clear the other missing ones (e.g. merge_3d etc) as well? > >>> + > >>> DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask); > >>> } > >>> @@ -285,6 +290,13 @@ static void > >>> dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx, > >>> ctx->pending_flush_mask |= BIT(MERGE_3D_IDX); > >>> } > >>> +static void dpu_hw_ctl_update_pending_flush_dsc_v1(struct dpu_hw_ctl > >>> *ctx, > >>> + enum dpu_dsc dsc_num) > >>> +{ > >>> + ctx->pending_dsc_flush_mask |= BIT(dsc_num - DSC_0); > >>> + ctx->pending_flush_mask |= BIT(DSC_IDX); > >>> +} > >>> + > >>> static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl > >>> *ctx, > >>> enum dpu_dspp dspp, u32 dspp_sub_blk) > >>> { > >>> @@ -502,9 +514,6 @@ static void dpu_hw_ctl_intf_cfg_v1(struct > >>> dpu_hw_ctl *ctx, > >>> if ((test_bit(DPU_CTL_VM_CFG, &ctx->caps->features))) > >>> mode_sel = CTL_DEFAULT_GROUP_ID << 28; > >>> - if (cfg->dsc) > >>> - DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, cfg->dsc); > >>> - > >>> if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD) > >>> mode_sel |= BIT(17); > >>> @@ -524,10 +533,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct > >>> dpu_hw_ctl *ctx, > >>> if (cfg->merge_3d) > >>> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, > >>> BIT(cfg->merge_3d - MERGE_3D_0)); > >>> - if (cfg->dsc) { > >>> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > >> Again, this bugfix of now wrapping DSC_IDX in BIT() should go in a > >> separate Fixes: patch to have this semantic change documented. (v8 > >> review) > > That will be this patch. let me add Fixes at this patch > > _separate_ patch. Separate patch, and documenting clearly what happens and why. Kuogee, I can send this as well if it makes things more clear, since it doesn't seem (from the patch description) that anyone noticed the implication/bugfix in this change as a drive-by effect of porting sde_hw_ctl_update_bitmask_dsc_v1() from downstream. - Marijn <snip>
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index ffa6f04..1957545 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1834,7 +1834,8 @@ dpu_encoder_dsc_initial_line_calc(struct drm_dsc_config *dsc, return DIV_ROUND_UP(total_pixels, dsc->slice_width); } -static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, +static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_ctl *ctl, + struct dpu_hw_dsc *hw_dsc, struct dpu_hw_pingpong *hw_pp, struct drm_dsc_config *dsc, u32 common_mode, @@ -1854,6 +1855,9 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, if (hw_pp->ops.enable_dsc) hw_pp->ops.enable_dsc(hw_pp); + + if (ctl->ops.update_pending_flush_dsc) + ctl->ops.update_pending_flush_dsc(ctl, hw_dsc->idx); } static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, @@ -1861,6 +1865,7 @@ static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, { /* coding only for 2LM, 2enc, 1 dsc config */ struct dpu_encoder_phys *enc_master = dpu_enc->cur_master; + struct dpu_hw_ctl *ctl = enc_master->hw_ctl; struct dpu_hw_dsc *hw_dsc[MAX_CHANNELS_PER_ENC]; struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC]; int this_frame_slices; @@ -1898,7 +1903,8 @@ static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, initial_lines = dpu_encoder_dsc_initial_line_calc(dsc, enc_ip_w); for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) - dpu_encoder_dsc_pipe_cfg(hw_dsc[i], hw_pp[i], dsc, dsc_common_mode, initial_lines); + dpu_encoder_dsc_pipe_cfg(ctl, hw_dsc[i], hw_pp[i], dsc, + dsc_common_mode, initial_lines); } void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) 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 4f7cfa9..4e132d9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c @@ -139,6 +139,11 @@ static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) CTL_DSPP_n_FLUSH(dspp - DSPP_0), ctx->pending_dspp_flush_mask[dspp - DSPP_0]); } + + if (ctx->pending_flush_mask & BIT(DSC_IDX)) + DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, + ctx->pending_dsc_flush_mask); + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask); } @@ -285,6 +290,13 @@ static void dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx, ctx->pending_flush_mask |= BIT(MERGE_3D_IDX); } +static void dpu_hw_ctl_update_pending_flush_dsc_v1(struct dpu_hw_ctl *ctx, + enum dpu_dsc dsc_num) +{ + ctx->pending_dsc_flush_mask |= BIT(dsc_num - DSC_0); + ctx->pending_flush_mask |= BIT(DSC_IDX); +} + static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx, enum dpu_dspp dspp, u32 dspp_sub_blk) { @@ -502,9 +514,6 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, if ((test_bit(DPU_CTL_VM_CFG, &ctx->caps->features))) mode_sel = CTL_DEFAULT_GROUP_ID << 28; - if (cfg->dsc) - DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, cfg->dsc); - if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD) mode_sel |= BIT(17); @@ -524,10 +533,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, if (cfg->merge_3d) DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, BIT(cfg->merge_3d - MERGE_3D_0)); - if (cfg->dsc) { - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); + + if (cfg->dsc) DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); - } } static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, @@ -630,6 +638,8 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops, ops->update_pending_flush_merge_3d = dpu_hw_ctl_update_pending_flush_merge_3d_v1; ops->update_pending_flush_wb = dpu_hw_ctl_update_pending_flush_wb_v1; + ops->update_pending_flush_dsc = + dpu_hw_ctl_update_pending_flush_dsc_v1; } else { ops->trigger_flush = dpu_hw_ctl_trigger_flush; ops->setup_intf_cfg = dpu_hw_ctl_intf_cfg; 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 6292002..d5f3ef8 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h @@ -158,6 +158,15 @@ struct dpu_hw_ctl_ops { enum dpu_dspp blk, u32 dspp_sub_blk); /** + * OR in the given flushbits to the cached pending_(dsc_)flush_mask + * No effect on hardware + * @ctx: ctl path ctx pointer + * @blk: interface block index + */ + void (*update_pending_flush_dsc)(struct dpu_hw_ctl *ctx, + enum dpu_dsc blk); + + /** * Write the value of the pending_flush_mask to hardware * @ctx : ctl path ctx pointer */ @@ -229,6 +238,9 @@ struct dpu_hw_ctl_ops { * @pending_flush_mask: storage for pending ctl_flush managed via ops * @pending_intf_flush_mask: pending INTF flush * @pending_wb_flush_mask: pending WB flush + * @pending_merge_3d_flush_mask: pending merge_3d flush + * @pending_dspp_flush_mask: pending dspp flush + * @pending_dsc_flush_mask: pending dsc flush * @ops: operation list */ struct dpu_hw_ctl { @@ -245,6 +257,7 @@ struct dpu_hw_ctl { u32 pending_wb_flush_mask; u32 pending_merge_3d_flush_mask; u32 pending_dspp_flush_mask[DSPP_MAX - DSPP_0]; + u32 pending_dsc_flush_mask; /* ops */ struct dpu_hw_ctl_ops ops;