Re: [PATCH v3 2/7] drm/msm/dpu: add DPU_PINGPONG_DSC feature bit

[Marijn Suijten]

On 2023-05-02 14:02:57, Kuogee Hsieh wrote:
> Legacy DPU requires PP block to be involved during DSC setting up.

This patch should clarify that "legacy" means DPU < 7.0.0, as we found
out in [1] that PINGPONG has no more register remaining in 7.x.  In
addition, it seems that the DCE enable register/flag moved to INTF, as
added by Jessica in [2].  Perhaps those patches should be part of this
series too, and this patch should mention that it was moved?

[1]: https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-7-27ce1a5ab5c6@xxxxxxxxxxxxxx/
[2]: https://lore.kernel.org/linux-arm-msm/20230405-add-dsc-support-v1-0-6bc6f03ae735@xxxxxxxxxxx/T/#t

> This patch adds DDPU_PINGPONG_DSC feature bit to indicate that both
> dpu_hw_pp_setup_dsc() and dpu_hw_pp_dsc_enable() pingpong ops
> functions are required to complete DSC data path set up and start

datapath setup*

As already suggested by Dmitry's review in a different way, this patch
doesn't "indicate that both ops are required to complete DSC datapath
setup", this patch removes those callbacks from PP since DPU 7.0.0 as
the registers are no longer present (and have been moved to the INTF in
some form).

The *implementation* is good though, and I'd r-b it after addressing the
nits - thanks!

> DSC engine.
> 
> Reported-by : Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>

drop the space before :.

> Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h  | 2 ++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 9 ++++++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> 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 71584cd..c07a6b6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -144,6 +144,7 @@ enum {
>   * @DPU_PINGPONG_SPLIT      PP block supports split fifo
>   * @DPU_PINGPONG_SLAVE      PP block is a suitable slave for split fifo
>   * @DPU_PINGPONG_DITHER,    Dither blocks
> + * @DPU_PINGPONG_DSC,	    PP ops functions required for DSC

Mixing tab indentation, and the comma shouldn't be there.

- Marijn

>   * @DPU_PINGPONG_MAX
>   */
>  enum {
> @@ -152,6 +153,7 @@ enum {
>  	DPU_PINGPONG_SPLIT,
>  	DPU_PINGPONG_SLAVE,
>  	DPU_PINGPONG_DITHER,
> +	DPU_PINGPONG_DSC,
>  	DPU_PINGPONG_MAX
>  };
>  
> 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 3822e06..f255a04 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> @@ -264,9 +264,12 @@ 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 (features & BIT(DPU_PINGPONG_DSC)) {
> +		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;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>