Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

From: Abhinav Kumar
Date: Thu Apr 07 2022 - 16:29:19 EST

Hi Doug and Dmitry

Sorry, but I caught up on this email just now.

Some comments below.

Thanks

Abhinav
On 4/7/2022 10:07 AM, Doug Anderson wrote:
Hi,

On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
<quic_sbillaka@xxxxxxxxxxx> wrote:

Hi Dmitry and Doug,

Hi,

On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:

On 05/04/2022 20:02, Doug Anderson wrote:
Hi,

On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
3. For DP and eDP HPD means something a little different.
Essentially there are two concepts: a) is a display physically
connected and b) is the display powered up and ready. For DP, the
two are really tied together. From the kernel's point of view you
never "power down" a DP display and you can't detect that it's
physically connected until it's ready. Said another way, on you
tie "is a display there" to the HPD line and the moment a display
is there it's ready for you to do AUX transfers. For eDP, in the
lowest power state of a display it _won't_ assert its "HPD"
signal. However, it's still physically present. For eDP you simply
have to _assume_ it's present without any actual proof since you
can't get proof until you power it up. Thus for eDP, you report
that the display is there as soon as we're asked. We can't _talk_
to the display yet, though. So in get_modes() we need to be able
to power the display on enough to talk over the AUX channel to it.
As part of this, we wait for the signal named "HPD" which really means
"panel finished powering on" in this context.

NOTE: for aux transfer, we don't have the _display_ pipe and
clocks running. We only have enough stuff running to do the AUX
transfer.
We're not clocking out pixels. We haven't fully powered on the
display. The AUX transfer is designed to be something that can be
done early _before_ you turn on the display.


OK, so basically that was a longwinded way of saying: yes, we
could avoid the AUX transfer in probe, but we can't wait all the
way to enable. We have to be able to transfer in get_modes(). If
you think that's helpful I think it'd be a pretty easy patch to
write even if it would look a tad bit awkward IMO. Let me know if
you want me to post it up.

I think it would be a good idea. At least it will allow us to
judge, which is the more correct way.

I'm still happy to prototype this, but the more I think about it the
more it feels like a workaround for the Qualcomm driver. The eDP
panel driver is actually given a pointer to the AUX bus at probe
time. It's really weird to say that we can't do a transfer on it
yet... As you said, this is a little sideband bus. It should be able
to be used without all the full blown infra of the rest of the driver.

Yes, I have that feeling too. However I also have a feeling that just
powering up the PHY before the bus probe is ... a hack. There are no
obvious stopgaps for the driver not to power it down later.


Lets go back to why we need to power up the PHY before the bus probe.

We need to power up PHY before bus probe because panel-eDP tries to read the EDID in probe() for the panel_id. Not get_modes().

So doug, I didnt follow your comment that panel-eDP only does EDID read in get_modes()

panel_id = drm_edid_get_panel_id(panel->ddc);
if (!panel_id) {
dev_err(dev, "Couldn't identify panel via EDID\n");
ret = -EIO;
goto exit;
}

If we do not need this part, we really dont need to power up the PHY before the probe(). The hack which dmitry was referring to.

So this is boiling down to why or how panel-eDP was originally designed.

This is why I think we need to move to Runtime PM to manage this. Basically:

1. When an AUX transfer happens, you grab a PM runtime reference that
_that_ powers up the PHY.

This will not be trivial and needs to be scoped out as sankeerth said but if the above is the only concern, why do we need to do this? There seems to be an explanation why we are doing this and its not a hack.

How would Dmitry's rework address this? We need some RFC to conclude on that first.


2. At the end of the AUX transfer function, you do a "put_autosuspend".

Then it becomes not a hack, right?



pm runtime ops needs to be implemented for both eDP and DP. This change
take good amount of planning and code changes as it affects DP also.

Because this patch series consist of basic eDP changes for SC7280 bootup,
shall we take this pm_runtime implementation in subsequent patch series?

Dmitry is the real decision maker here, but in my opinion it would be
OK to get something landed first that worked OK and wasn't taking us
too far in the wrong direction and then we could get a follow up patch
to move to pm_runtime.

I would say the discussion changed into a direction of implementing pm-runtime because the current patch series does what it takes to adhere to panel-eDP's design along with aux bus requirements of PHY needing to be on.

So doug, to answer your questions here:

"So I guess the net result is maybe we should just keep it where it is.
Long term I'd be interested in knowing if there's a reason why we
can't structure the driver so that AUX transfers can happen with less
intertwining with the rest of the code, but that can happen later. I
would expect that you'd basically just need clocks and regulators on
and maybe your PHY on."

Yes PHY needs to be absolutely on and configured before aux transfers.

If we want to change that up to stop reading the panel_id in the panel probe() and do it later, perhaps some of the changes done here are not needed.

It only seems reasonable that we first prototype that in a separate patch even a RFC perhaps and take this further as these set of changes are needed for basic display functionality on sc7280 chromebooks.

Let us know what are the concerns with doing it in a follow up change.

Thanks

Abhinav