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.
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.
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.
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.
Regards,
Bjorn
