Hi Vinod,
I seem to have sent this review to v3 instead of the repost of v4. It
should still apply the same, hope that's no issue.
- Marijn
On 2022-02-17 22:54:38, Marijn Suijten wrote:
> On 2021-11-16 11:52:46, Vinod Koul wrote:
> > In SDM845, DSC can be enabled by writing to pingpong block registers, so
> > add support for DSC in hw_pp
>
> Nit: I don't think the ", so add support for DSC in XXX" part in this
> and other commit messages add anything. You've already stated that in
> the title, the commit body is just extra justification (and can perhaps
> be filled with extra details about the patch contents instead).
>
> > Reviewed-by: Abhinav Kumar <abhinavk@xxxxxxxxxxxxxx>
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > Signed-off-by: Vinod Koul <vkoul@xxxxxxxxxx>
> > ---
> > .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 32 +++++++++++++++++++
> > .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h | 14 ++++++++
> > 2 files changed, 46 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > index 55766c97c4c8..47c6ab6caf95 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> > @@ -28,6 +28,9 @@
> > #define PP_FBC_MODE 0x034
> > #define PP_FBC_BUDGET_CTL 0x038
> > #define PP_FBC_LOSSY_MODE 0x03C
> > +#define PP_DSC_MODE 0x0a0
> > +#define PP_DCE_DATA_IN_SWAP 0x0ac
>
> This enum does not seem used here, is it used in another patch?
>
> > +#define PP_DCE_DATA_OUT_SWAP 0x0c8
> >
> > #define PP_DITHER_EN 0x000
> > #define PP_DITHER_BITDEPTH 0x004
> > @@ -245,6 +248,32 @@ static u32 dpu_hw_pp_get_line_count(struct dpu_hw_pingpong *pp)
> > return line;
> > }
> >
> > +static int dpu_hw_pp_dsc_enable(struct dpu_hw_pingpong *pp)
> > +{
> > + struct dpu_hw_blk_reg_map *c = &pp->hw;
> > +
> > + DPU_REG_WRITE(c, PP_DSC_MODE, 1);
> > + return 0;
> > +}
> > +
> > +static void dpu_hw_pp_dsc_disable(struct dpu_hw_pingpong *pp)
> > +{
> > + struct dpu_hw_blk_reg_map *c = &pp->hw;
> > +
> > + DPU_REG_WRITE(c, PP_DSC_MODE, 0);
> > +}
> > +
> > +static int dpu_hw_pp_setup_dsc(struct dpu_hw_pingpong *pp)
> > +{
> > + struct dpu_hw_blk_reg_map *pp_c = &pp->hw;
> > + int data;
> > +
> > + data = DPU_REG_READ(pp_c, PP_DCE_DATA_OUT_SWAP);
> > + data |= BIT(18); /* endian flip */
> > + DPU_REG_WRITE(pp_c, PP_DCE_DATA_OUT_SWAP, data);
> > + return 0;
> > +}
> > +
> > static void _setup_pingpong_ops(struct dpu_hw_pingpong *c,
> > unsigned long features)
> > {
> > @@ -256,6 +285,9 @@ static void _setup_pingpong_ops(struct dpu_hw_pingpong *c,
> > c->ops.get_autorefresh = dpu_hw_pp_get_autorefresh_config;
> > c->ops.poll_timeout_wr_ptr = dpu_hw_pp_poll_timeout_wr_ptr;
> > c->ops.get_line_count = dpu_hw_pp_get_line_count;
> > + c->ops.setup_dsc = dpu_hw_pp_setup_dsc;
> > + c->ops.enable_dsc = dpu_hw_pp_dsc_enable;
> > + c->ops.disable_dsc = dpu_hw_pp_dsc_disable;
> >
> > if (test_bit(DPU_PINGPONG_DITHER, &features))
> > c->ops.setup_dither = dpu_hw_pp_setup_dither;
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h
> > index 89d08a715c16..12758468d9ca 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h
> > @@ -124,6 +124,20 @@ struct dpu_hw_pingpong_ops {
> > */
> > void (*setup_dither)(struct dpu_hw_pingpong *pp,
> > struct dpu_hw_dither_cfg *cfg);
> > + /**
> > + * Enable DSC
> > + */
> > + int (*enable_dsc)(struct dpu_hw_pingpong *pp);
> > +
> > + /**
> > + * Disable DSC
> > + */
> > + void (*disable_dsc)(struct dpu_hw_pingpong *pp);
>
> It looks like most other callbacks in dpu1 use an `enable` function with
> a boolean, instead of having a separate disable function. That should
> simplify the implementation down to a single ternary-if, too. Would
> that be desired to use here?
>
> - Marijn
>
> > +
> > + /**
> > + * Setup DSC
> > + */
> > + int (*setup_dsc)(struct dpu_hw_pingpong *pp);
> > };
> >
> > struct dpu_hw_merge_3d;
> > --
> > 2.31.1
> >
