Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <56453228-d4b2-c7e4-7b72-6de8637f2def@linaro.org>
Date:   Fri, 15 Apr 2022 00:16:31 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus
Content-Language: en-GB
To:     Doug Anderson <dianders@chromium.org>,
        Stephen Boyd <swboyd@chromium.org>
Cc:     Sankeerth Billakanti <quic_sbillaka@quicinc.com>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        freedreno <freedreno@lists.freedesktop.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, Rob Clark <robdclark@gmail.com>,
        Sean Paul <seanpaul@chromium.org>,
        quic_kalyant <quic_kalyant@quicinc.com>,
        Abhinav Kumar <quic_abhinavk@quicinc.com>,
        Kuogee Hsieh <quic_khsieh@quicinc.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        quic_vproddut <quic_vproddut@quicinc.com>,
        Aravind Venkateswaran <quic_aravindh@quicinc.com>,
        Steev Klimaszewski <steev@kali.org>
References: <1649938766-6768-1-git-send-email-quic_sbillaka@quicinc.com>
 <1649938766-6768-2-git-send-email-quic_sbillaka@quicinc.com>
 <CAD=FV=Wmiv2WGhFCLYmXbWESNOh5FfobjNme85aU6YtN1SLVDA@mail.gmail.com>
 <81c3a9fb-4c92-6969-c715-ca085322f9c6@linaro.org>
 <CAE-0n50obe_aqzwQY-X1yH4emjjOErOJ_wj9sQe=HoWEZ3vjTw@mail.gmail.com>
 <CAD=FV=U4qtst5q--_1794Pdjsc7b_JMRAh+X_vr-9qJx5NtOrw@mail.gmail.com>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
In-Reply-To: <CAD=FV=U4qtst5q--_1794Pdjsc7b_JMRAh+X_vr-9qJx5NtOrw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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