Re: [REPOST PATCH v4 08/13] drm/msm/disp/dpu1: Don't use DSC with mode_3d

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 18/02/2022 23:46, Abhinav Kumar wrote:
> 
> 
> On 2/16/2022 11:12 PM, Dmitry Baryshkov wrote:
>> On 17/02/2022 09:33, Abhinav Kumar wrote:
>>>
>>>
>>> On 2/16/2022 10:10 PM, Vinod Koul wrote:
>>>> On 16-02-22, 19:11, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 2/10/2022 2:34 AM, Vinod Koul wrote:
>>>>>> We cannot enable mode_3d when we are using the DSC. So pass
>>>>>> configuration to detect DSC is enabled and not enable mode_3d
>>>>>> when we are using DSC
>>>>>>
>>>>>> We add a helper dpu_encoder_helper_get_dsc() to detect dsc
>>>>>> enabled and pass this to .setup_intf_cfg()
>>>>>>
>>>>>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>>>>>
>>>>> We should not use 3D mux only when we use DSC merge topology.
>>>>> I agree that today we use only 2-2-1 topology for DSC which means 
>>>>> its using
>>>>> DSC merge.
>>>>>
>>>>> But generalizing that 3D mux should not be used for DSC is not right.
>>>>>
>>>>> You can detect DSC merge by checking if there are two encoders and one
>>>>> interface in the topology and if so, you can disable 3D mux.
>>>>
>>>> Right now with DSC we disable that as suggested by Dmitry last time.
>>>> Whenever we introduce merge we should revisit this, for now this should
>>>> suffice
>>>>
>>>
>>> Sorry I didnt follow.
>>>
>>> The topology which you are supporting today IS DSC merge 2-2-1. I 
>>> didnt get what you mean by "whenever we introduce".
>>>
>>> I didnt follow Dmitry's comment either.
>>>
>>> "anybody adding support for SDE_RM_TOPOLOGY_DUALPIPE_3DMERGE_DSC 
>>> handle this."
>>>
>>> 3D mux shouldnt be used when DSC merge is used.
>>>
>>> The topology Dmitry is referring to will not use DSC merge but you 
>>> are using it here and thats why you had to make this patch in the 
>>> first place. So I am not sure why would someone who uses 3D merge 
>>> topology worry about DSC merge. Your patch is the one which deals 
>>> with the topology in question.
>>>
>>> What I am suggesting is a small but necessary improvement to this patch.
>>
>> It seems that we can replace this patch by changing 
>> dpu_encoder_helper_get_3d_blend_mode() to contain the following 
>> condition (instead of the one present there). Does the following seem 
>> correct to you:
>>
>> static inline enum dpu_3d_blend_mode 
>> dpu_encoder_helper_get_3d_blend_mode(
>>                  struct dpu_encoder_phys *phys_enc)
>> {
>>          struct dpu_crtc_state *dpu_cstate;
>>
>>          if (!phys_enc || phys_enc->enable_state == DPU_ENC_DISABLING)
>>                  return BLEND_3D_NONE;
>>
>>          dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
>>
>> +    /* Use merge_3d unless DSCMERGE topology is used */
>>          if (phys_enc->split_role == ENC_ROLE_SOLO &&
>> +           hweight(dpu_encoder_helper_get_dsc(phys_enc)) != 1 &&

Yes, the correct should be:
hweight(...) == 2

>>              dpu_cstate->num_mixers == CRTC_DUAL_MIXERS)
>>                  return BLEND_3D_H_ROW_INT;
>>
>>          return BLEND_3D_NONE;
>> }
> 
> This will not be enough. To detect whether DSC merge is enabled you need 
> to query the topology. The above condition only checks if DSC is enabled 
> not DSC merge.
> 
> So the above function can be modified to use a helper like below instead 
> of the hweight.
> 
> bool dpu_encoder_get_dsc_merge_info(struct dpu_encoder_virt *dpu_enc)
> {
>      struct msm_display_topology topology = {0};
> 
>      topology = dpu_encoder_get_topology(...);
> 
>      if (topology.num_dsc > topology.num_intf)

num_intf is 1 or 2. If it's one, the split_role is SOLO
hweight would return a num of bits in the DSC mask. It's 0, 1 or 2.
So, if the split_role is SOLO and hweight is 2, we get exactly your 
condition.

Does that sound correct?

>          return true;
>      else
>          return false;
> }
> 
> if (!dpu_encoder_get_dsc_merge_info() && other conditions listed above)
>      return BLEND_3D_H_ROW_INT;
> else
>      BLEND_3D_NONE;
>>
>>
>>>
>>> All that you have to do is in query whether DSC merge is used from 
>>> the topology. You can do it in multiple ways:
>>>
>>> 1) Either query this from the encoder
>>> 2) Store a bool "dsc_merge" in the intf_cfg
>>>
>>
>>


-- 
With best wishes
Dmitry