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

[Dmitry Baryshkov]

On 19/05/2023 19:21, Kuogee Hsieh wrote:
> 
> 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).

I'd prefer to see:

- DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
+ DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, BIT(DSC_IDX));

This a Fixes patch and in theory it can be backported to stable kernel.

Then the rest can go into this patch, including the movement of 
BIT(DSC_IDX) to proper function, etc.

> 
>>
>> <snip>

-- 
With best wishes
Dmitry