diff mbox series

[v2,08/17] drm/msm/dpu: add changes to support writeback in hw_ctl

Message ID 1650419169-13760-9-git-send-email-quic_abhinavk@quicinc.com (mailing list archive)
State New, archived
Headers show
Series Add writeback block support for DPU | expand

Commit Message

Abhinav Kumar April 20, 2022, 1:46 a.m. UTC
Add changes to support writeback module in the dpu_hw_ctl
interface.

changes in v2:
	- keep only the wb specific changes to reset_intf_cfg
	- use cfg->intf / cfg->wb to identify intf or wb
	- use bit-wise OR for the wb bits while programming

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 43 +++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 15 ++++++++++-
 2 files changed, 53 insertions(+), 5 deletions(-)

Comments

Dmitry Baryshkov April 20, 2022, 6:59 a.m. UTC | #1
On 20/04/2022 04:46, Abhinav Kumar wrote:
> Add changes to support writeback module in the dpu_hw_ctl
> interface.
> 
> changes in v2:
> 	- keep only the wb specific changes to reset_intf_cfg
> 	- use cfg->intf / cfg->wb to identify intf or wb
> 	- use bit-wise OR for the wb bits while programming
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 43 +++++++++++++++++++++++++++---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 15 ++++++++++-
>   2 files changed, 53 insertions(+), 5 deletions(-)
> 
> 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 524f024..495a9cd 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> @@ -1,5 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> +/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>    */
>   
>   #include <linux/delay.h>
> @@ -23,10 +24,12 @@
>   #define   CTL_SW_RESET                  0x030
>   #define   CTL_LAYER_EXTN_OFFSET         0x40
>   #define   CTL_MERGE_3D_ACTIVE           0x0E4
> +#define   CTL_WB_ACTIVE                 0x0EC
>   #define   CTL_INTF_ACTIVE               0x0F4
>   #define   CTL_MERGE_3D_FLUSH            0x100
>   #define   CTL_DSC_ACTIVE                0x0E8
>   #define   CTL_DSC_FLUSH                0x104
> +#define   CTL_WB_FLUSH                  0x108
>   #define   CTL_INTF_FLUSH                0x110
>   #define   CTL_INTF_MASTER               0x134
>   #define   CTL_FETCH_PIPE_ACTIVE         0x0FC
> @@ -38,6 +41,7 @@
>   #define  MERGE_3D_IDX   23
>   #define  DSC_IDX        22
>   #define  INTF_IDX       31
> +#define WB_IDX          16
>   #define CTL_INVALID_BIT                 0xffff
>   #define CTL_DEFAULT_GROUP_ID		0xf
>   
> @@ -135,6 +139,9 @@ static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
>   	if (ctx->pending_flush_mask & BIT(INTF_IDX))
>   		DPU_REG_WRITE(&ctx->hw, CTL_INTF_FLUSH,
>   				ctx->pending_intf_flush_mask);
> +	if (ctx->pending_flush_mask & BIT(WB_IDX))
> +		DPU_REG_WRITE(&ctx->hw, CTL_WB_FLUSH,
> +				ctx->pending_wb_flush_mask);
>   
>   	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
>   }
> @@ -255,6 +262,13 @@ static void dpu_hw_ctl_update_pending_flush_intf(struct dpu_hw_ctl *ctx,
>   	}
>   }
>   
> +static void dpu_hw_ctl_update_pending_flush_wb_v1(struct dpu_hw_ctl *ctx,
> +		enum dpu_wb wb)
> +{
> +	ctx->pending_wb_flush_mask |= BIT(wb - WB_0);
> +	ctx->pending_flush_mask |= BIT(WB_IDX);
> +}
> +
>   static void dpu_hw_ctl_update_pending_flush_intf_v1(struct dpu_hw_ctl *ctx,
>   		enum dpu_intf intf)
>   {
> @@ -504,6 +518,7 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
>   {
>   	struct dpu_hw_blk_reg_map *c = &ctx->hw;
>   	u32 intf_active = 0;
> +	u32 wb_active = 0;
>   	u32 mode_sel = 0;
>   
>   	/* CTL_TOP[31:28] carries group_id to collate CTL paths
> @@ -519,11 +534,20 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
>   	if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD)
>   		mode_sel |= BIT(17);
>   
> -	intf_active = DPU_REG_READ(c, CTL_INTF_ACTIVE);
> -	intf_active |= BIT(cfg->intf - INTF_0);
> +	if (cfg->intf) {
> +		intf_active = DPU_REG_READ(c, CTL_INTF_ACTIVE);
> +		intf_active |= BIT(cfg->intf - INTF_0);
> +	}
> +
> +	if (cfg->wb) {
> +		wb_active = DPU_REG_READ(c, CTL_WB_ACTIVE);
> +		wb_active |= BIT(cfg->wb - WB_0);
> +	}
>   
>   	DPU_REG_WRITE(c, CTL_TOP, mode_sel);
>   	DPU_REG_WRITE(c, CTL_INTF_ACTIVE, intf_active);
> +	DPU_REG_WRITE(c, CTL_WB_ACTIVE, wb_active);

This will not work as expected. If cfg->intf is not set, CTL_INTF_ACTIVE 
will be reset to 0 (while it should have been retained). Please change 
this to always read CTL_INTF_ACTIVE/CTL_WB_ACTIVE.

> +
>   	if (cfg->merge_3d)
>   		DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
>   			      BIT(cfg->merge_3d - MERGE_3D_0));
> @@ -546,6 +570,9 @@ static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
>   		intf_cfg |= (cfg->mode_3d - 0x1) << 20;
>   	}
>   
> +	if (cfg->wb)
> +		intf_cfg |= (cfg->wb & 0x3) + 2;
> +

Ugh. I see that we have the same code in downstream driver. And that we 
do not support WB_0 at all. But maybe we should be more explicit here.

>   	switch (cfg->intf_mode_sel) {
>   	case DPU_CTL_MODE_SEL_VID:
>   		intf_cfg &= ~BIT(17);
> @@ -568,12 +595,13 @@ static void dpu_hw_ctl_reset_intf_cfg_v1(struct dpu_hw_ctl *ctx,
>   {
>   	struct dpu_hw_blk_reg_map *c = &ctx->hw;
>   	u32 intf_active = 0;
> +	u32 wb_active = 0;
>   	u32 merge3d_active = 0;
>   
>   	/*
>   	 * This API resets each portion of the CTL path namely,
>   	 * clearing the sspps staged on the lm, merge_3d block,
> -	 * interfaces etc to ensure clean teardown of the pipeline.
> +	 * interfaces , writeback etc to ensure clean teardown of the pipeline.
>   	 * This will be used for writeback to begin with to have a
>   	 * proper teardown of the writeback session but upon further
>   	 * validation, this can be extended to all interfaces.
> @@ -592,6 +620,12 @@ static void dpu_hw_ctl_reset_intf_cfg_v1(struct dpu_hw_ctl *ctx,
>   		intf_active &= ~BIT(cfg->intf - INTF_0);
>   		DPU_REG_WRITE(c, CTL_INTF_ACTIVE, intf_active);
>   	}
> +
> +	if (cfg->wb) {
> +		wb_active = DPU_REG_READ(c, CTL_WB_ACTIVE);
> +		wb_active &= ~BIT(cfg->wb - WB_0);
> +		DPU_REG_WRITE(c, CTL_WB_ACTIVE, wb_active);
> +	}
>   }
>   
>   static void dpu_hw_ctl_set_fetch_pipe_active(struct dpu_hw_ctl *ctx,
> @@ -622,6 +656,7 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops,
>   			dpu_hw_ctl_update_pending_flush_intf_v1;
>   		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;

Do we also need the update_pending_flush_wb for non-ACTIVE_CTL case? I 
think we do.

>   	} 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 c61a8fd..df8f8e9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> @@ -1,5 +1,6 @@
>   /* SPDX-License-Identifier: GPL-2.0-only */
> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> +/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>    */
>   
>   #ifndef _DPU_HW_CTL_H
> @@ -44,6 +45,7 @@ struct dpu_hw_stage_cfg {
>    */
>   struct dpu_hw_intf_cfg {
>   	enum dpu_intf intf;
> +	enum dpu_wb wb;
>   	enum dpu_3d_blend_mode mode_3d;
>   	enum dpu_merge_3d merge_3d;
>   	enum dpu_ctl_mode_sel intf_mode_sel;
> @@ -102,6 +104,15 @@ struct dpu_hw_ctl_ops {
>   		u32 flushbits);
>   
>   	/**
> +	 * OR in the given flushbits to the cached pending_(wb_)flush_mask
> +	 * No effect on hardware
> +	 * @ctx       : ctl path ctx pointer
> +	 * @blk       : writeback block index
> +	 */
> +	void (*update_pending_flush_wb)(struct dpu_hw_ctl *ctx,
> +		enum dpu_wb blk);
> +
> +	/**
>   	 * OR in the given flushbits to the cached pending_(intf_)flush_mask
>   	 * No effect on hardware
>   	 * @ctx       : ctl path ctx pointer
> @@ -199,6 +210,7 @@ struct dpu_hw_ctl_ops {
>    * @mixer_hw_caps: mixer hardware capabilities
>    * @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
>    * @ops: operation list
>    */
>   struct dpu_hw_ctl {
> @@ -212,6 +224,7 @@ struct dpu_hw_ctl {
>   	const struct dpu_lm_cfg *mixer_hw_caps;
>   	u32 pending_flush_mask;
>   	u32 pending_intf_flush_mask;
> +	u32 pending_wb_flush_mask;
>   	u32 pending_merge_3d_flush_mask;
>   
>   	/* ops */
Abhinav Kumar April 20, 2022, 5:16 p.m. UTC | #2
On 4/19/2022 11:59 PM, Dmitry Baryshkov wrote:
> On 20/04/2022 04:46, Abhinav Kumar wrote:
>> Add changes to support writeback module in the dpu_hw_ctl
>> interface.
>>
>> changes in v2:
>>     - keep only the wb specific changes to reset_intf_cfg
>>     - use cfg->intf / cfg->wb to identify intf or wb
>>     - use bit-wise OR for the wb bits while programming
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 43 
>> +++++++++++++++++++++++++++---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 15 ++++++++++-
>>   2 files changed, 53 insertions(+), 5 deletions(-)
>>
>> 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 524f024..495a9cd 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> @@ -1,5 +1,6 @@
>>   // SPDX-License-Identifier: GPL-2.0-only
>> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>> +/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>>    */
>>   #include <linux/delay.h>
>> @@ -23,10 +24,12 @@
>>   #define   CTL_SW_RESET                  0x030
>>   #define   CTL_LAYER_EXTN_OFFSET         0x40
>>   #define   CTL_MERGE_3D_ACTIVE           0x0E4
>> +#define   CTL_WB_ACTIVE                 0x0EC
>>   #define   CTL_INTF_ACTIVE               0x0F4
>>   #define   CTL_MERGE_3D_FLUSH            0x100
>>   #define   CTL_DSC_ACTIVE                0x0E8
>>   #define   CTL_DSC_FLUSH                0x104
>> +#define   CTL_WB_FLUSH                  0x108
>>   #define   CTL_INTF_FLUSH                0x110
>>   #define   CTL_INTF_MASTER               0x134
>>   #define   CTL_FETCH_PIPE_ACTIVE         0x0FC
>> @@ -38,6 +41,7 @@
>>   #define  MERGE_3D_IDX   23
>>   #define  DSC_IDX        22
>>   #define  INTF_IDX       31
>> +#define WB_IDX          16
>>   #define CTL_INVALID_BIT                 0xffff
>>   #define CTL_DEFAULT_GROUP_ID        0xf
>> @@ -135,6 +139,9 @@ static inline void 
>> dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
>>       if (ctx->pending_flush_mask & BIT(INTF_IDX))
>>           DPU_REG_WRITE(&ctx->hw, CTL_INTF_FLUSH,
>>                   ctx->pending_intf_flush_mask);
>> +    if (ctx->pending_flush_mask & BIT(WB_IDX))
>> +        DPU_REG_WRITE(&ctx->hw, CTL_WB_FLUSH,
>> +                ctx->pending_wb_flush_mask);
>>       DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
>>   }
>> @@ -255,6 +262,13 @@ static void 
>> dpu_hw_ctl_update_pending_flush_intf(struct dpu_hw_ctl *ctx,
>>       }
>>   }
>> +static void dpu_hw_ctl_update_pending_flush_wb_v1(struct dpu_hw_ctl 
>> *ctx,
>> +        enum dpu_wb wb)
>> +{
>> +    ctx->pending_wb_flush_mask |= BIT(wb - WB_0);
>> +    ctx->pending_flush_mask |= BIT(WB_IDX);
>> +}
>> +
>>   static void dpu_hw_ctl_update_pending_flush_intf_v1(struct 
>> dpu_hw_ctl *ctx,
>>           enum dpu_intf intf)
>>   {
>> @@ -504,6 +518,7 @@ static void dpu_hw_ctl_intf_cfg_v1(struct 
>> dpu_hw_ctl *ctx,
>>   {
>>       struct dpu_hw_blk_reg_map *c = &ctx->hw;
>>       u32 intf_active = 0;
>> +    u32 wb_active = 0;
>>       u32 mode_sel = 0;
>>       /* CTL_TOP[31:28] carries group_id to collate CTL paths
>> @@ -519,11 +534,20 @@ static void dpu_hw_ctl_intf_cfg_v1(struct 
>> dpu_hw_ctl *ctx,
>>       if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD)
>>           mode_sel |= BIT(17);
>> -    intf_active = DPU_REG_READ(c, CTL_INTF_ACTIVE);
>> -    intf_active |= BIT(cfg->intf - INTF_0);
>> +    if (cfg->intf) {
>> +        intf_active = DPU_REG_READ(c, CTL_INTF_ACTIVE);
>> +        intf_active |= BIT(cfg->intf - INTF_0);
>> +    }
>> +
>> +    if (cfg->wb) {
>> +        wb_active = DPU_REG_READ(c, CTL_WB_ACTIVE);
>> +        wb_active |= BIT(cfg->wb - WB_0);
>> +    }
>>       DPU_REG_WRITE(c, CTL_TOP, mode_sel);
>>       DPU_REG_WRITE(c, CTL_INTF_ACTIVE, intf_active);
>> +    DPU_REG_WRITE(c, CTL_WB_ACTIVE, wb_active);
> 
> This will not work as expected. If cfg->intf is not set, CTL_INTF_ACTIVE 
> will be reset to 0 (while it should have been retained). Please change 
> this to always read CTL_INTF_ACTIVE/CTL_WB_ACTIVE.

ack, and thanks for catching this.
Yes, i need to add the always read part back.

> 
>> +
>>       if (cfg->merge_3d)
>>           DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
>>                     BIT(cfg->merge_3d - MERGE_3D_0));
>> @@ -546,6 +570,9 @@ static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl 
>> *ctx,
>>           intf_cfg |= (cfg->mode_3d - 0x1) << 20;
>>       }
>> +    if (cfg->wb)
>> +        intf_cfg |= (cfg->wb & 0x3) + 2;
>> +
> 
> Ugh. I see that we have the same code in downstream driver. And that we 
> do not support WB_0 at all. But maybe we should be more explicit here.

Sorry, I didnt follow this comment. Why is this related to WB_0?

All this code is doing is that its programming the lower bits of CTL_TOP 
register to be used for WB mode.

The correct value of this register for linear wb mode which we use is 0x5.

Which will still be correct now because cfg->wb will be 0x3.

Coming to other non-WB_2 values, this code is still correct.

Lets say cfg->wb was 0x1 ( for WB_0), then the register will be 
programmed to 0x3 which is the correct value to use because then we will 
be using rotation and not linear writeback.

Perhaps, you need a comment here to explain this?

> 
>>       switch (cfg->intf_mode_sel) {
>>       case DPU_CTL_MODE_SEL_VID:
>>           intf_cfg &= ~BIT(17);
>> @@ -568,12 +595,13 @@ static void dpu_hw_ctl_reset_intf_cfg_v1(struct 
>> dpu_hw_ctl *ctx,
>>   {
>>       struct dpu_hw_blk_reg_map *c = &ctx->hw;
>>       u32 intf_active = 0;
>> +    u32 wb_active = 0;
>>       u32 merge3d_active = 0;
>>       /*
>>        * This API resets each portion of the CTL path namely,
>>        * clearing the sspps staged on the lm, merge_3d block,
>> -     * interfaces etc to ensure clean teardown of the pipeline.
>> +     * interfaces , writeback etc to ensure clean teardown of the 
>> pipeline.
>>        * This will be used for writeback to begin with to have a
>>        * proper teardown of the writeback session but upon further
>>        * validation, this can be extended to all interfaces.
>> @@ -592,6 +620,12 @@ static void dpu_hw_ctl_reset_intf_cfg_v1(struct 
>> dpu_hw_ctl *ctx,
>>           intf_active &= ~BIT(cfg->intf - INTF_0);
>>           DPU_REG_WRITE(c, CTL_INTF_ACTIVE, intf_active);
>>       }
>> +
>> +    if (cfg->wb) {
>> +        wb_active = DPU_REG_READ(c, CTL_WB_ACTIVE);
>> +        wb_active &= ~BIT(cfg->wb - WB_0);
>> +        DPU_REG_WRITE(c, CTL_WB_ACTIVE, wb_active);
>> +    }
>>   }
>>   static void dpu_hw_ctl_set_fetch_pipe_active(struct dpu_hw_ctl *ctx,
>> @@ -622,6 +656,7 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops 
>> *ops,
>>               dpu_hw_ctl_update_pending_flush_intf_v1;
>>           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;
> 
> Do we also need the update_pending_flush_wb for non-ACTIVE_CTL case? I 
> think we do.

Yes, but the bits will be different. I can update it.

> 
>>       } 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 c61a8fd..df8f8e9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
>> @@ -1,5 +1,6 @@
>>   /* SPDX-License-Identifier: GPL-2.0-only */
>> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>> +/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>>    */
>>   #ifndef _DPU_HW_CTL_H
>> @@ -44,6 +45,7 @@ struct dpu_hw_stage_cfg {
>>    */
>>   struct dpu_hw_intf_cfg {
>>       enum dpu_intf intf;
>> +    enum dpu_wb wb;
>>       enum dpu_3d_blend_mode mode_3d;
>>       enum dpu_merge_3d merge_3d;
>>       enum dpu_ctl_mode_sel intf_mode_sel;
>> @@ -102,6 +104,15 @@ struct dpu_hw_ctl_ops {
>>           u32 flushbits);
>>       /**
>> +     * OR in the given flushbits to the cached pending_(wb_)flush_mask
>> +     * No effect on hardware
>> +     * @ctx       : ctl path ctx pointer
>> +     * @blk       : writeback block index
>> +     */
>> +    void (*update_pending_flush_wb)(struct dpu_hw_ctl *ctx,
>> +        enum dpu_wb blk);
>> +
>> +    /**
>>        * OR in the given flushbits to the cached 
>> pending_(intf_)flush_mask
>>        * No effect on hardware
>>        * @ctx       : ctl path ctx pointer
>> @@ -199,6 +210,7 @@ struct dpu_hw_ctl_ops {
>>    * @mixer_hw_caps: mixer hardware capabilities
>>    * @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
>>    * @ops: operation list
>>    */
>>   struct dpu_hw_ctl {
>> @@ -212,6 +224,7 @@ struct dpu_hw_ctl {
>>       const struct dpu_lm_cfg *mixer_hw_caps;
>>       u32 pending_flush_mask;
>>       u32 pending_intf_flush_mask;
>> +    u32 pending_wb_flush_mask;
>>       u32 pending_merge_3d_flush_mask;
>>       /* ops */
> 
>
Dmitry Baryshkov April 20, 2022, 6:48 p.m. UTC | #3
On Wed, 20 Apr 2022 at 20:16, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 4/19/2022 11:59 PM, Dmitry Baryshkov wrote:
> > On 20/04/2022 04:46, Abhinav Kumar wrote:
> >> Add changes to support writeback module in the dpu_hw_ctl
> >> interface.
> >>
> >> changes in v2:
> >>     - keep only the wb specific changes to reset_intf_cfg
> >>     - use cfg->intf / cfg->wb to identify intf or wb
> >>     - use bit-wise OR for the wb bits while programming
> >>
> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 43
> >> +++++++++++++++++++++++++++---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 15 ++++++++++-
> >>   2 files changed, 53 insertions(+), 5 deletions(-)
> >>
> >> 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 524f024..495a9cd 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> >> @@ -1,5 +1,6 @@
> >>   // SPDX-License-Identifier: GPL-2.0-only
> >> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> >> +/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
> >> reserved.
> >> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> >>    */
> >>   #include <linux/delay.h>
> >> @@ -23,10 +24,12 @@
> >>   #define   CTL_SW_RESET                  0x030
> >>   #define   CTL_LAYER_EXTN_OFFSET         0x40
> >>   #define   CTL_MERGE_3D_ACTIVE           0x0E4
> >> +#define   CTL_WB_ACTIVE                 0x0EC
> >>   #define   CTL_INTF_ACTIVE               0x0F4
> >>   #define   CTL_MERGE_3D_FLUSH            0x100
> >>   #define   CTL_DSC_ACTIVE                0x0E8
> >>   #define   CTL_DSC_FLUSH                0x104
> >> +#define   CTL_WB_FLUSH                  0x108
> >>   #define   CTL_INTF_FLUSH                0x110
> >>   #define   CTL_INTF_MASTER               0x134
> >>   #define   CTL_FETCH_PIPE_ACTIVE         0x0FC
> >> @@ -38,6 +41,7 @@
> >>   #define  MERGE_3D_IDX   23
> >>   #define  DSC_IDX        22
> >>   #define  INTF_IDX       31
> >> +#define WB_IDX          16
> >>   #define CTL_INVALID_BIT                 0xffff
> >>   #define CTL_DEFAULT_GROUP_ID        0xf
> >> @@ -135,6 +139,9 @@ static inline void
> >> dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
> >>       if (ctx->pending_flush_mask & BIT(INTF_IDX))
> >>           DPU_REG_WRITE(&ctx->hw, CTL_INTF_FLUSH,
> >>                   ctx->pending_intf_flush_mask);
> >> +    if (ctx->pending_flush_mask & BIT(WB_IDX))
> >> +        DPU_REG_WRITE(&ctx->hw, CTL_WB_FLUSH,
> >> +                ctx->pending_wb_flush_mask);
> >>       DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
> >>   }
> >> @@ -255,6 +262,13 @@ static void
> >> dpu_hw_ctl_update_pending_flush_intf(struct dpu_hw_ctl *ctx,
> >>       }
> >>   }
> >> +static void dpu_hw_ctl_update_pending_flush_wb_v1(struct dpu_hw_ctl
> >> *ctx,
> >> +        enum dpu_wb wb)
> >> +{
> >> +    ctx->pending_wb_flush_mask |= BIT(wb - WB_0);
> >> +    ctx->pending_flush_mask |= BIT(WB_IDX);
> >> +}
> >> +
> >>   static void dpu_hw_ctl_update_pending_flush_intf_v1(struct
> >> dpu_hw_ctl *ctx,
> >>           enum dpu_intf intf)
> >>   {
> >> @@ -504,6 +518,7 @@ static void dpu_hw_ctl_intf_cfg_v1(struct
> >> dpu_hw_ctl *ctx,
> >>   {
> >>       struct dpu_hw_blk_reg_map *c = &ctx->hw;
> >>       u32 intf_active = 0;
> >> +    u32 wb_active = 0;
> >>       u32 mode_sel = 0;
> >>       /* CTL_TOP[31:28] carries group_id to collate CTL paths
> >> @@ -519,11 +534,20 @@ static void dpu_hw_ctl_intf_cfg_v1(struct
> >> dpu_hw_ctl *ctx,
> >>       if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD)
> >>           mode_sel |= BIT(17);
> >> -    intf_active = DPU_REG_READ(c, CTL_INTF_ACTIVE);
> >> -    intf_active |= BIT(cfg->intf - INTF_0);
> >> +    if (cfg->intf) {
> >> +        intf_active = DPU_REG_READ(c, CTL_INTF_ACTIVE);
> >> +        intf_active |= BIT(cfg->intf - INTF_0);
> >> +    }
> >> +
> >> +    if (cfg->wb) {
> >> +        wb_active = DPU_REG_READ(c, CTL_WB_ACTIVE);
> >> +        wb_active |= BIT(cfg->wb - WB_0);
> >> +    }
> >>       DPU_REG_WRITE(c, CTL_TOP, mode_sel);
> >>       DPU_REG_WRITE(c, CTL_INTF_ACTIVE, intf_active);
> >> +    DPU_REG_WRITE(c, CTL_WB_ACTIVE, wb_active);
> >
> > This will not work as expected. If cfg->intf is not set, CTL_INTF_ACTIVE
> > will be reset to 0 (while it should have been retained). Please change
> > this to always read CTL_INTF_ACTIVE/CTL_WB_ACTIVE.
>
> ack, and thanks for catching this.
> Yes, i need to add the always read part back.
>
> >
> >> +
> >>       if (cfg->merge_3d)
> >>           DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
> >>                     BIT(cfg->merge_3d - MERGE_3D_0));
> >> @@ -546,6 +570,9 @@ static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl
> >> *ctx,
> >>           intf_cfg |= (cfg->mode_3d - 0x1) << 20;
> >>       }
> >> +    if (cfg->wb)
> >> +        intf_cfg |= (cfg->wb & 0x3) + 2;
> >> +
> >
> > Ugh. I see that we have the same code in downstream driver. And that we
> > do not support WB_0 at all. But maybe we should be more explicit here.
>
> Sorry, I didnt follow this comment. Why is this related to WB_0?
>
> All this code is doing is that its programming the lower bits of CTL_TOP
> register to be used for WB mode.
>
> The correct value of this register for linear wb mode which we use is 0x5.
>
> Which will still be correct now because cfg->wb will be 0x3.
>
> Coming to other non-WB_2 values, this code is still correct.
>
> Lets say cfg->wb was 0x1 ( for WB_0), then the register will be
> programmed to 0x3 which is the correct value to use because then we will
> be using rotation and not linear writeback.
>
> Perhaps, you need a comment here to explain this?

IIRC, at least for 8916 WB_0 must be used with this field set to 0x1
or 0x3 depending on other settings.
Thus I thought it might be better to be explicit here.

As a second thought, let's keep it as is (and if somebody works on
WB_0/rotation support, he will know what to set here anyway).

>
> >
> >>       switch (cfg->intf_mode_sel) {
> >>       case DPU_CTL_MODE_SEL_VID:
> >>           intf_cfg &= ~BIT(17);
> >> @@ -568,12 +595,13 @@ static void dpu_hw_ctl_reset_intf_cfg_v1(struct
> >> dpu_hw_ctl *ctx,
> >>   {
> >>       struct dpu_hw_blk_reg_map *c = &ctx->hw;
> >>       u32 intf_active = 0;
> >> +    u32 wb_active = 0;
> >>       u32 merge3d_active = 0;
> >>       /*
> >>        * This API resets each portion of the CTL path namely,
> >>        * clearing the sspps staged on the lm, merge_3d block,
> >> -     * interfaces etc to ensure clean teardown of the pipeline.
> >> +     * interfaces , writeback etc to ensure clean teardown of the
> >> pipeline.
> >>        * This will be used for writeback to begin with to have a
> >>        * proper teardown of the writeback session but upon further
> >>        * validation, this can be extended to all interfaces.
> >> @@ -592,6 +620,12 @@ static void dpu_hw_ctl_reset_intf_cfg_v1(struct
> >> dpu_hw_ctl *ctx,
> >>           intf_active &= ~BIT(cfg->intf - INTF_0);
> >>           DPU_REG_WRITE(c, CTL_INTF_ACTIVE, intf_active);
> >>       }
> >> +
> >> +    if (cfg->wb) {
> >> +        wb_active = DPU_REG_READ(c, CTL_WB_ACTIVE);
> >> +        wb_active &= ~BIT(cfg->wb - WB_0);
> >> +        DPU_REG_WRITE(c, CTL_WB_ACTIVE, wb_active);
> >> +    }
> >>   }
> >>   static void dpu_hw_ctl_set_fetch_pipe_active(struct dpu_hw_ctl *ctx,
> >> @@ -622,6 +656,7 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops
> >> *ops,
> >>               dpu_hw_ctl_update_pending_flush_intf_v1;
> >>           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;
> >
> > Do we also need the update_pending_flush_wb for non-ACTIVE_CTL case? I
> > think we do.
>
> Yes, but the bits will be different. I can update it.

Yes, please.

>
> >
> >>       } 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 c61a8fd..df8f8e9 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> >> @@ -1,5 +1,6 @@
> >>   /* SPDX-License-Identifier: GPL-2.0-only */
> >> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> >> +/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
> >> reserved.
> >> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> >>    */
> >>   #ifndef _DPU_HW_CTL_H
> >> @@ -44,6 +45,7 @@ struct dpu_hw_stage_cfg {
> >>    */
> >>   struct dpu_hw_intf_cfg {
> >>       enum dpu_intf intf;
> >> +    enum dpu_wb wb;
> >>       enum dpu_3d_blend_mode mode_3d;
> >>       enum dpu_merge_3d merge_3d;
> >>       enum dpu_ctl_mode_sel intf_mode_sel;
> >> @@ -102,6 +104,15 @@ struct dpu_hw_ctl_ops {
> >>           u32 flushbits);
> >>       /**
> >> +     * OR in the given flushbits to the cached pending_(wb_)flush_mask
> >> +     * No effect on hardware
> >> +     * @ctx       : ctl path ctx pointer
> >> +     * @blk       : writeback block index
> >> +     */
> >> +    void (*update_pending_flush_wb)(struct dpu_hw_ctl *ctx,
> >> +        enum dpu_wb blk);
> >> +
> >> +    /**
> >>        * OR in the given flushbits to the cached
> >> pending_(intf_)flush_mask
> >>        * No effect on hardware
> >>        * @ctx       : ctl path ctx pointer
> >> @@ -199,6 +210,7 @@ struct dpu_hw_ctl_ops {
> >>    * @mixer_hw_caps: mixer hardware capabilities
> >>    * @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
> >>    * @ops: operation list
> >>    */
> >>   struct dpu_hw_ctl {
> >> @@ -212,6 +224,7 @@ struct dpu_hw_ctl {
> >>       const struct dpu_lm_cfg *mixer_hw_caps;
> >>       u32 pending_flush_mask;
> >>       u32 pending_intf_flush_mask;
> >> +    u32 pending_wb_flush_mask;
> >>       u32 pending_merge_3d_flush_mask;
> >>       /* ops */
> >
> >
diff mbox series

Patch

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 524f024..495a9cd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -1,5 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
  */
 
 #include <linux/delay.h>
@@ -23,10 +24,12 @@ 
 #define   CTL_SW_RESET                  0x030
 #define   CTL_LAYER_EXTN_OFFSET         0x40
 #define   CTL_MERGE_3D_ACTIVE           0x0E4
+#define   CTL_WB_ACTIVE                 0x0EC
 #define   CTL_INTF_ACTIVE               0x0F4
 #define   CTL_MERGE_3D_FLUSH            0x100
 #define   CTL_DSC_ACTIVE                0x0E8
 #define   CTL_DSC_FLUSH                0x104
+#define   CTL_WB_FLUSH                  0x108
 #define   CTL_INTF_FLUSH                0x110
 #define   CTL_INTF_MASTER               0x134
 #define   CTL_FETCH_PIPE_ACTIVE         0x0FC
@@ -38,6 +41,7 @@ 
 #define  MERGE_3D_IDX   23
 #define  DSC_IDX        22
 #define  INTF_IDX       31
+#define WB_IDX          16
 #define CTL_INVALID_BIT                 0xffff
 #define CTL_DEFAULT_GROUP_ID		0xf
 
@@ -135,6 +139,9 @@  static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
 	if (ctx->pending_flush_mask & BIT(INTF_IDX))
 		DPU_REG_WRITE(&ctx->hw, CTL_INTF_FLUSH,
 				ctx->pending_intf_flush_mask);
+	if (ctx->pending_flush_mask & BIT(WB_IDX))
+		DPU_REG_WRITE(&ctx->hw, CTL_WB_FLUSH,
+				ctx->pending_wb_flush_mask);
 
 	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
 }
@@ -255,6 +262,13 @@  static void dpu_hw_ctl_update_pending_flush_intf(struct dpu_hw_ctl *ctx,
 	}
 }
 
+static void dpu_hw_ctl_update_pending_flush_wb_v1(struct dpu_hw_ctl *ctx,
+		enum dpu_wb wb)
+{
+	ctx->pending_wb_flush_mask |= BIT(wb - WB_0);
+	ctx->pending_flush_mask |= BIT(WB_IDX);
+}
+
 static void dpu_hw_ctl_update_pending_flush_intf_v1(struct dpu_hw_ctl *ctx,
 		enum dpu_intf intf)
 {
@@ -504,6 +518,7 @@  static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
 {
 	struct dpu_hw_blk_reg_map *c = &ctx->hw;
 	u32 intf_active = 0;
+	u32 wb_active = 0;
 	u32 mode_sel = 0;
 
 	/* CTL_TOP[31:28] carries group_id to collate CTL paths
@@ -519,11 +534,20 @@  static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
 	if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD)
 		mode_sel |= BIT(17);
 
-	intf_active = DPU_REG_READ(c, CTL_INTF_ACTIVE);
-	intf_active |= BIT(cfg->intf - INTF_0);
+	if (cfg->intf) {
+		intf_active = DPU_REG_READ(c, CTL_INTF_ACTIVE);
+		intf_active |= BIT(cfg->intf - INTF_0);
+	}
+
+	if (cfg->wb) {
+		wb_active = DPU_REG_READ(c, CTL_WB_ACTIVE);
+		wb_active |= BIT(cfg->wb - WB_0);
+	}
 
 	DPU_REG_WRITE(c, CTL_TOP, mode_sel);
 	DPU_REG_WRITE(c, CTL_INTF_ACTIVE, intf_active);
+	DPU_REG_WRITE(c, CTL_WB_ACTIVE, wb_active);
+
 	if (cfg->merge_3d)
 		DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
 			      BIT(cfg->merge_3d - MERGE_3D_0));
@@ -546,6 +570,9 @@  static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
 		intf_cfg |= (cfg->mode_3d - 0x1) << 20;
 	}
 
+	if (cfg->wb)
+		intf_cfg |= (cfg->wb & 0x3) + 2;
+
 	switch (cfg->intf_mode_sel) {
 	case DPU_CTL_MODE_SEL_VID:
 		intf_cfg &= ~BIT(17);
@@ -568,12 +595,13 @@  static void dpu_hw_ctl_reset_intf_cfg_v1(struct dpu_hw_ctl *ctx,
 {
 	struct dpu_hw_blk_reg_map *c = &ctx->hw;
 	u32 intf_active = 0;
+	u32 wb_active = 0;
 	u32 merge3d_active = 0;
 
 	/*
 	 * This API resets each portion of the CTL path namely,
 	 * clearing the sspps staged on the lm, merge_3d block,
-	 * interfaces etc to ensure clean teardown of the pipeline.
+	 * interfaces , writeback etc to ensure clean teardown of the pipeline.
 	 * This will be used for writeback to begin with to have a
 	 * proper teardown of the writeback session but upon further
 	 * validation, this can be extended to all interfaces.
@@ -592,6 +620,12 @@  static void dpu_hw_ctl_reset_intf_cfg_v1(struct dpu_hw_ctl *ctx,
 		intf_active &= ~BIT(cfg->intf - INTF_0);
 		DPU_REG_WRITE(c, CTL_INTF_ACTIVE, intf_active);
 	}
+
+	if (cfg->wb) {
+		wb_active = DPU_REG_READ(c, CTL_WB_ACTIVE);
+		wb_active &= ~BIT(cfg->wb - WB_0);
+		DPU_REG_WRITE(c, CTL_WB_ACTIVE, wb_active);
+	}
 }
 
 static void dpu_hw_ctl_set_fetch_pipe_active(struct dpu_hw_ctl *ctx,
@@ -622,6 +656,7 @@  static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops,
 			dpu_hw_ctl_update_pending_flush_intf_v1;
 		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;
 	} 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 c61a8fd..df8f8e9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
@@ -1,5 +1,6 @@ 
 /* SPDX-License-Identifier: GPL-2.0-only */
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/* Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
  */
 
 #ifndef _DPU_HW_CTL_H
@@ -44,6 +45,7 @@  struct dpu_hw_stage_cfg {
  */
 struct dpu_hw_intf_cfg {
 	enum dpu_intf intf;
+	enum dpu_wb wb;
 	enum dpu_3d_blend_mode mode_3d;
 	enum dpu_merge_3d merge_3d;
 	enum dpu_ctl_mode_sel intf_mode_sel;
@@ -102,6 +104,15 @@  struct dpu_hw_ctl_ops {
 		u32 flushbits);
 
 	/**
+	 * OR in the given flushbits to the cached pending_(wb_)flush_mask
+	 * No effect on hardware
+	 * @ctx       : ctl path ctx pointer
+	 * @blk       : writeback block index
+	 */
+	void (*update_pending_flush_wb)(struct dpu_hw_ctl *ctx,
+		enum dpu_wb blk);
+
+	/**
 	 * OR in the given flushbits to the cached pending_(intf_)flush_mask
 	 * No effect on hardware
 	 * @ctx       : ctl path ctx pointer
@@ -199,6 +210,7 @@  struct dpu_hw_ctl_ops {
  * @mixer_hw_caps: mixer hardware capabilities
  * @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
  * @ops: operation list
  */
 struct dpu_hw_ctl {
@@ -212,6 +224,7 @@  struct dpu_hw_ctl {
 	const struct dpu_lm_cfg *mixer_hw_caps;
 	u32 pending_flush_mask;
 	u32 pending_intf_flush_mask;
+	u32 pending_wb_flush_mask;
 	u32 pending_merge_3d_flush_mask;
 
 	/* ops */