Messages in this thread | | | Date | Sat, 19 Feb 2022 00:36:45 +0300 | Subject | Re: [Freedreno] [REPOST PATCH v4 08/13] drm/msm/disp/dpu1: Don't use DSC with mode_3d | From | Dmitry Baryshkov <> |
| |
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 <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.
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
| |