Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 14/04/2022 23:09, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 14, 2022 at 12:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
>>
>> Quoting Dmitry Baryshkov (2022-04-14 12:16:14)
>>>
>>> I think it's too verbose and a bit incorrect.
> 
> Not sure which part you're asserting is incorrect, but shorter is OK w/ me too.

I was referring to the "If we don't implement the ops..." part. For some 
reason I thought that panel implements detect() callback (and thus the 
DRM will not care because the next bridge takes precedence).

However I was mistaken, please excuse me. Your description was correct 
and I was wrong. The panel bridge doesn't implement callback. Most 
probably I mixed it with the display_connector bridge.

So... your description is more correct.

> 
> 
>>> This is a bit saner:
>>> /*
>>>    * These ops do not make sense for eDP, since they are provided
>>>    * by the panel-bridge corresponding to the attached eDP panel.
>>>    */
>>>
>>> My question was whether we really need to disable them for eDP since for
>>> eDP the detect and and get_modes will be overridden anyway.
> 
> Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work?
> It's definitely worth confirming but from my reading of the code it
> _probably_ wouldn't hurt.
> 
> One thing someone would want to confirm would be what would happen if
> we move this code and the panel code to implement DRM_BRIDGE_OP_EDID
> properly. It looks as if both actually ought to be implementing that
> instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a
> future day. Could we get into trouble if one moved before the other?
> Then the panel would no longer override the eDP controller and the eDP
> controller would try to read from a possibly unpowered panel?

That would depend on the way the get_edid would be implemented in DP 
driver. Currently the edid is cached via the 
dp_display_process_hpd_high() -> dp_panel_read_sink_caps() call chain.

With this patchset, the dp_hpd_plug_handle() -> 
dp_display_usbpd_configure_cb() -> dp_display_process_hpd_high() will be 
called too late for the get_modes/get_edid (from dp_bridge's enable() op).

There is another issue. drm_panel has only get_modes() callback, so 
panel_bridge can not implement get_edid() unless we extend the panel 
interface (which might be a good idea).

> 
> So I guess in the end my preference would be that we know that driving
> the EDID read from the controller isn't a great idea for eDP (since we
> have no way to ensure that the panel is powered) so why risk it and
> set the bit saying we can do it?

Yep.


> For hotplug/detect I'm even less confident that setting the bits would
> be harmless. I haven't sat down and traced everything, but from what I
> can see the panel _doesn't_ set these bits, does it? I believe that
> the rule is that when every bridge in the chain _doesn't_ implement
> detect/hotplug that the panel is always present. The moment someone
> says "hey, I can detect" then it suddenly becomes _not_ always
> present. Yes, I guess we could have the panel implement "detect" and
> return true, but I'm not convinced that's actually better...

I think it makes sense to implement OP_DETECT in panel bridge (that 
always returns connector_status_connected) at least to override the 
possible detect ops in previous bridges.

>> And to go further, I'd expect that a bridge should expose the
>> functionality that it supports, regardless of what is connected down the
>> chain. Otherwise we won't be able to mix and match bridges because the
>> code is brittle, making assumptions about what is connected.
> 
>  From my point of view the bridge simply doesn't support any of the
> three things when we're in eDP mode. Yes, it's the same driver. Yes,
> eDP and DP share nearly the same signalling on the wire. Yes, it's
> easily possible to make a single controller that supports DP and eDP.
> ...but the rules around detection and power sequencing are simply
> different for the two cases.

I just hope that during refactoring this can be expressed in a more 
natural way.

> The controller simply _cannot_ detect
> whether the panel is connected in the eDP case and it _must_ assume
> that the panel is always connected. Yes, it has an HPD pin. No, that
> HPD pin doesn't tell when the panel is present. The panel is always
> present. The panel is always present.

Yep, I remember regarding the HPD pin. And I still think that panel-edp 
(and panel bridge) should provide an overriding OP_DETECT.

> So, IMO, it is _incorrect_ to say we can support HPD and DETECT if we
> know we're in eDP mode.

I see your point. Let's do it this way. Maybe (hopefully) it will become 
more logical during refactoring. Or maybe I'll just tune myself into the 
DP/eDP logic :D

-- 
With best wishes
Dmitry