diff mbox series

[v3,03/13] drm/msm/disp/dpu1: Add support for DSC in pingpong block

Message ID 20211116062256.2417186-4-vkoul@kernel.org (mailing list archive)
State Superseded
Headers show
Series drm/msm: Add Display Stream Compression Support | expand

Commit Message

Vinod Koul Nov. 16, 2021, 6:22 a.m. UTC 
In SDM845, DSC can be enabled by writing to pingpong block registers, so
add support for DSC in hw_pp

Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 .../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(+)

Comments

Marijn Suijten Feb. 17, 2022, 9:54 p.m. UTC | #1 
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@codeaurora.org>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  .../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
>
Marijn Suijten Feb. 17, 2022, 10:06 p.m. UTC | #2 
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@codeaurora.org>
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  .../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
> >
Dmitry Baryshkov Feb. 17, 2022, 10:12 p.m. UTC | #3 
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 <abhinavk@codeaurora.org>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>> ---
>>   .../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.

> 
> - Marijn
> 
>> +
>> +	/**
>> +	 * Setup DSC
>> +	 */
>> +	int (*setup_dsc)(struct dpu_hw_pingpong *pp);
>>   };
>>   
>>   struct dpu_hw_merge_3d;
>> -- 
>> 2.31.1
>>
Marijn Suijten Feb. 17, 2022, 10:39 p.m. UTC | #4 
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 <abhinavk@codeaurora.org>
> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> >> ---
> >>   .../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
diff mbox series

Patch

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
+#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);
+
+	/**
+	 * Setup DSC
+	 */
+	int (*setup_dsc)(struct dpu_hw_pingpong *pp);
 };
 
 struct dpu_hw_merge_3d;