On Thu 17 Feb 19:10 CST 2022, Dmitry Baryshkov wrote: > On 16/02/2022 05:16, Abhinav Kumar wrote: > > > > > > On 2/15/2022 6:03 PM, Bjorn Andersson wrote: > > > On Tue 15 Feb 19:34 CST 2022, Abhinav Kumar wrote: > > > > > > > > > > > > > > > On 2/15/2022 4:20 PM, Dmitry Baryshkov wrote: > > > > > On Tue, 15 Feb 2022 at 23:21, Abhinav Kumar > > > > > <quic_abhinavk@xxxxxxxxxxx> wrote: > > > > > > On 2/15/2022 10:42 AM, Dmitry Baryshkov wrote: > > > > > > > On Tue, 15 Feb 2022 at 20:42, Abhinav Kumar > > > > > > > <quic_abhinavk@xxxxxxxxxxx> wrote: > > > > > > > > On 2/15/2022 9:28 AM, Bjorn Andersson wrote: > > > > > > > > > On Tue 15 Feb 11:14 CST 2022, Abhinav Kumar wrote: > > > > > > > > > > On 2/14/2022 8:33 PM, Bjorn Andersson wrote: > > > > > > > > > > > From: Rob Clark <robdclark@xxxxxxxxxxxx> > > > [..] > > > > > > > (thus leading us to cases when someone would forget to add INTF_EDP > > > > > > > next to INTF_DP) > > > > > > > > > > > > > > Also, if we are switching from INTF_DP to INTF_EDP, should we stop > > > > > > > using end-to-end numbering (like MSM_DP_CONTROLLER_2 for INTF_5) and > > > > > > > add a separate numbering scheme for INTF_EDP? > > > > > > > > > > > > > We should change the controller ID to match what it actually is. > > > > > > > > > > > > Now that you pointed this out, this looks even more confusing to me to > > > > > > say that MSM_DP_CONTROLLER_2 is actually a EDP controller because > > > > > > fundamentally and even hardware block wise they are different. > > > > > > > > > > So, do we split msm_priv->dp too? It's indexed using > > > > > MSM_DP_CONTROLLER_n entries. > > > > > Do we want to teach drm/msm/dp code that there are priv->dp[] and > > > > > priv->edp arrays? > > > > > > > > ok so now priv->dp and priv->edp arrays are also in the picture here :) > > > > > > > > Actually all these questions should have probably come when we > > > > were figuring > > > > out how best to re-use eDP and DP driver. > > Well, these questions were evaluated. And this resulted in our suggestion to > reuse DP driver, INTF_DP type and priv->dp array. > > > > > > > > > Either way atleast, its good we are documenting all these > > > > questions on this > > > > thread so that anyone can refer this to know what all was missed out :) > > > > > > > > priv->dp is of type msm_dp. When re-using DP driver for eDP and since > > > > struct msm_dp is the shared struct between dpu and the msm/dp, I > > > > get your > > > > point of re-using MSM_DP_CONTROLLER_* as thats being use to index. > > > > > > > > So MSM_DP_CONTROLLER_* is more of an index into the DP driver > > > > and not really > > > > a hardware indexing scheme. > > > > > > > > If we split into two arrays, we need more changes to dpu_encoder too. > > > > > > > > Too instrusive a change at this point, even though probably correct. > > > > > > > > > > I'm sorry, but performing such a split would create a whole bunch of > > > duplication and I don't see the reasons yet. Can you please give me an > > > example of when the DPU _code_ would benefit from being specifically > > > written for EDP vs DP? > > > > > > Things where it doesn't make sense to enable certain features in > > > runtime - but really have different implementation for the two interface > > > types. > > > > > > > Like I have mentioned in my previous comment, this would be a big change > > and I am also not in favor of this big change. > I'm also not in favour of splitting priv->dp into ->dp and ->edp. > > One of the reasons, pointed out by Bjorn, is that some of interfaces can be > used for both DP and eDP. Adding them to either of arrays would create > confusion. > > Second reason being that introducing the split would bring in extra code for > no additional benefits. From the DPU point of view both DP and eDP > interfaces look the same. > > > > > But are you seeing more changes required even if we just change > > > > INTF_DP to > > > > INTF_eDP for the eDP entries? What are the challenges there? > > > > > > > > > > What are the benefits? > > > > In terms of current code, again like I said before in my previous > > comments several times I do not have an example. > > > > I was keeping the separation in case in future for some features we do > > need to differentiate eDP and DP. > > And we also might need to separte eDP-behind msm/dp and old-8x74-eDP. > It the same "possible" future that we might face. > > > > > Somehow I also feel this change and below are interlinked that way. > > > > https://patchwork.freedesktop.org/patch/473871/ > > > > The only reason we need this change is because both eDP and DP use > > DRM_MODE_ENCODER_TMDS and specifying the intf_type directly will clear > > the confusion because DRM_MODE_ENCODER_DSI means DSI and > > DRM_MODE_ENCODER_VIRTUAL means Writeback but DRM_MODE_ENCODER_TMDS can > > mean DP OR eDP interface. > > > > The ambiguity was always for eDP and DP. > > > > That led to the discussion about the INTF_* we are specifying in the > > dpu_hw_catalog only to find the discrepancy. > > > > So now by clearing that ambiguity that change makes sense. That > > discussion trickled into this one. > > I did some research for the INTF_*. As you probably remember (I didn't) on > mdp4 and mdp5 chipsets we program the DISP_INTF_SEL registers, telling the > hardware which hardware is to be driven by each of INTFs. > The freely available 410E HRD demands that this register is written. > > At some point this became unnecessary, but the DPU driver kept INTF_* > intact. Including INTF_EDP, INTF_LCDC, INTF_HDMI, etc. However from my > understanding INTF_EDP would correspond to older eDP interfaces, not to eDP > panels being connected by the contemporary DP/eDP ports. > > Oh, and last but not least, I'd suggest to follow downstream, which uses > "dp" to name all of DP/EDP ports. See https://github.com/TheXPerienceProject/android_kernel_xiaomi_courbet/blob/xpe-16.0/arch/arm64/boot/dts/qcom/sdmshrike-sde.dtsi#L89 > > So, to summarize my proposal: > - Keep INTF_EDP reserved for 8x74/8x84 > - Use INTF_DP for all contemporary DP and eDP ports > - Documet this in dpu_hw_mdss.h > - Remove INTF_EDP usage in dpu1 driver. > I'm in favour of this. Regards, Bjorn
