[Freedreno] [PATCH v10 6/8] drm/msm/dpu: separate DSC flush update out of interface

[Kuogee Hsieh]

On 5/19/2023 5:04 AM, Marijn Suijten wrote:
> On 2023-05-19 01:40:19, Dmitry Baryshkov wrote:
>> On 19/05/2023 01:09, Kuogee Hsieh wrote:
>>> On 5/17/2023 3:31 PM, Marijn Suijten wrote:
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>>>> @@ -139,6 +139,11 @@ static inline void
>>>>> dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
>>>>>                    CTL_DSPP_n_FLUSH(dspp - DSPP_0),
>>>>>                    ctx->pending_dspp_flush_mask[dspp - DSPP_0]);
>>>>>            }
>>>>> +
>>>>> +    if (ctx->pending_flush_mask & BIT(DSC_IDX))
>>>>> +        DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH,
>>>>> +                  ctx->pending_dsc_flush_mask);
>>>> Again, when do we reset this mask to 0?  (v8 review)
>>> can not find it.
>>>
>>> let me add a separate  patch to fix this.
>> The pending_dsc_flush_mask was added in this patch, so the reset should
>> be a part of this patch too.
> Yes, same patch.
yes, i keep pending_dsc_flush_mask = 0; at this patch at V11
>
> Related question I asked in v8: only the global pending_flush_mask and
> pending_dspp_flush_mask are reset in dpu_hw_ctl_clear_pending_flush().
> Shall I send a patch to clear the other missing ones (e.g. merge_3d etc)
> as well?
at v11, I had add separate  patch to clear missing ones.
>>>>> +
>>>>>        DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
>>>>>    }
>>>>> @@ -285,6 +290,13 @@ static void
>>>>> dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx,
>>>>>        ctx->pending_flush_mask |= BIT(MERGE_3D_IDX);
>>>>>    }
>>>>> +static void dpu_hw_ctl_update_pending_flush_dsc_v1(struct dpu_hw_ctl
>>>>> *ctx,
>>>>> +                           enum dpu_dsc dsc_num)
>>>>> +{
>>>>> +    ctx->pending_dsc_flush_mask |= BIT(dsc_num - DSC_0);
>>>>> +    ctx->pending_flush_mask |= BIT(DSC_IDX);
>>>>> +}
>>>>> +
>>>>>    static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl
>>>>> *ctx,
>>>>>        enum dpu_dspp dspp, u32 dspp_sub_blk)
>>>>>    {
>>>>> @@ -502,9 +514,6 @@ static void dpu_hw_ctl_intf_cfg_v1(struct
>>>>> dpu_hw_ctl *ctx,
>>>>>        if ((test_bit(DPU_CTL_VM_CFG, &ctx->caps->features)))
>>>>>            mode_sel = CTL_DEFAULT_GROUP_ID  << 28;
>>>>> -    if (cfg->dsc)
>>>>> -        DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, cfg->dsc);
>>>>> -
>>>>>        if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD)
>>>>>            mode_sel |= BIT(17);
>>>>> @@ -524,10 +533,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct
>>>>> dpu_hw_ctl *ctx,
>>>>>        if (cfg->merge_3d)
>>>>>            DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
>>>>>                      BIT(cfg->merge_3d - MERGE_3D_0));
>>>>> -    if (cfg->dsc) {
>>>>> -        DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
>>>> Again, this bugfix of now wrapping DSC_IDX in BIT() should go in a
>>>> separate Fixes: patch to have this semantic change documented.  (v8
>>>> review)
>>> That will be this patch. let me add Fixes at this patch
>> _separate_ patch.
> Separate patch, and documenting clearly what happens and why.  Kuogee, I
> can send this as well if it makes things more clear, since it doesn't
> seem (from the patch description) that anyone noticed the
> implication/bugfix in this change as a drive-by effect of porting
> sde_hw_ctl_update_bitmask_dsc_v1() from downstream.
>
> - Marijn

The problem is a create a separate patch to delete 
DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX), then this patch will break 
dsc function.

So that I keep this within same patch.

please confirm you still want a separate patch to delete 
DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX).

>
> <snip>