Re: [Freedreno] [PATCH v2 2/2] drm/msm/dpu: Add SC8180x to hw catalog

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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@quicinc.com> wrote:
> > > > > > On 2/15/2022 10:42 AM, Dmitry Baryshkov wrote:
> > > > > > > On Tue, 15 Feb 2022 at 20:42, Abhinav Kumar
> > > > > > > <quic_abhinavk@quicinc.com> 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@chromium.org>
> > > [..]
> > > > > > > (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