On 2/18/2022 1:21 PM, Dmitry Baryshkov wrote:
> 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 <[email protected]>
>>>>>>
>>>>>> 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.
>
num_intf is 1 in this case as only single interface is used. But even 4
dsc encoders going to 2 interfaces its DSC merge. So assuming num_intf
as 1 always is not right. Thats why I suggested a generalized confition
like above.
> 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
>>>>
>>>
>>>
>
>
On 19/02/2022 00:29, Abhinav Kumar wrote:
>
>
> On 2/18/2022 1:21 PM, Dmitry Baryshkov wrote:
>> 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 <[email protected]>
>>>>>>>
>>>>>>> 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.
>>
> num_intf is 1 in this case as only single interface is used. But even 4
> dsc encoders going to 2 interfaces its DSC merge. So assuming num_intf
> as 1 always is not right. Thats why I suggested a generalized confition
> like above.
Ah, quadpipe. Yes, then I was incorrect.
>
>> 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