Messages in this thread | | | Date | Fri, 18 Feb 2022 13:29:41 -0800 | Subject | Re: [Freedreno] [REPOST PATCH v4 08/13] drm/msm/disp/dpu1: Don't use DSC with mode_3d | From | Abhinav Kumar <> |
| |
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 <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. > 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 >>>> >>> >>> > >
| |