On 2022-02-18 01:12:02, Dmitry Baryshkov wrote: > On 18/02/2022 00:54, 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 <[email protected]> > >> Reviewed-by: Dmitry Baryshkov <[email protected]> > >> Signed-off-by: Vinod Koul <[email protected]> > >> --- > >> .../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? > > Just my 2c. I personally hate the unified functions with the boolean > argument. One of the reasons being the return value. Typically you do > not expect that the disable function can fail (or return an error). But > the unified function provides an error (to be handled) even in the > disable case. > > Last, but not least, overall the kernel API is biased towards separate > enable and disable calls.
Fair enough, we should replace the other functions then. Or perhaps drop the return argument entirely, it's always zero for enable_dsc anyway. I doubt we'll ever add additional checks here? If we do, things can be split again. - Marijn
