[RFC] drm/msm/dp: Allow attaching a drm_panel
Bjorn Andersson
bjorn.andersson at linaro.org
Tue Oct 5 02:11:57 UTC 2021
On Mon 04 Oct 20:50 CDT 2021, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2021-10-04 18:11:11)
> > On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > > <bjorn.andersson at linaro.org> wrote:
> > > >
> > > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > > <bjorn.andersson at linaro.org> wrote:
> > > > > >
> > > > > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > > > > +{
> > > > > > + struct device_node *np = parser->pdev->dev.of_node;
> > > > > > + int rc;
> > > > > > +
> > > > > > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> > > > >
> > > > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > > > port 1 is "Output endpoint of the controller". We should just use port
> > > > > 1 here, right?
> > > > >
> > > >
> > > > Finally got back to this, changed it to 1 and figured out why I left it
> > > > at 2.
> > > >
> > > > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > > > reference to the USB-C controller, scan through the registered panels
> > > > and conclude that the of_node of the USB-C controller isn't a registered
> > > > panel and return -EPROBE_DEFER.
> > >
> > > I'm confused, but maybe it would help if I could see something
> > > concrete. Is there a specific board this was happening on?
> > >
> >
> > Right, let's make this more concrete with a snippet from the actual
> > SC8180x DT.
>
> Where is this DT? Is it in the kernel tree?
>
Still missing a bunch of driver pieces, so I haven't yet pushed any of
this upstream.
But if you're interested you can find some work-in-progress here:
https://github.com/andersson/kernel/commits/wip/sc8180x-next-20210819
> >
> > > Under the DP node in the device tree I expect:
> > >
> > > ports {
> > > port at 1 {
> > > reg = <1>;
> > > edp_out: endpoint {
> > > remote-endpoint = <&edp_panel_in>;
> > > };
> > > };
> > > };
> > >
> >
> > /* We got a panel */
> > panel {
> > ...
> > ports {
> > port {
> > auo_b133han05_in: endpoint {
> > remote-endpoint = <&mdss_edp_out>;
> > };
> > };
> > };
> > };
> >
> > /* And a 2-port USB-C controller */
> > type-c-controller {
> > ...
> > connector at 0 {
> > ports {
> > port at 0 {
> > reg = <0>;
> > ucsi_port_0_dp: endpoint {
> > remote-endpoint = <&dp0_mode>;
> > };
> > };
> >
> > port at 1 {
> > reg = <1>;
> > ucsi_port_0_switch: endpoint {
> > remote-endpoint = <&primary_qmp_phy>;
> > };
> > };
> > };
> > };
> >
> > connector at 1 {
> > ports {
> > port at 0 {
> > reg = <0>;
> > ucsi_port_1_dp: endpoint {
> > remote-endpoint = <&dp1_mode>;
> > };
> > };
> >
> > port at 1 {
> > reg = <1>;
> > ucsi_port_1_switch: endpoint {
> > remote-endpoint = <&second_qmp_phy>;
> > };
> > };
> > };
> > };
> > };
> >
> > /* And then our 2 DP and single eDP controllers */
> > &mdss_dp0 {
> > ports {
> > port at 1 {
> > reg = <1>;
> > dp0_mode: endpoint {
> > remote-endpoint = <&ucsi_port_0_dp>;
> > };
> > };
> > };
> > };
> >
> > &mdss_dp1 {
> > ports {
> > port at 1 {
> > reg = <1>;
> > dp1_mode: endpoint {
> > remote-endpoint = <&ucsi_port_1_dp>;
> > };
> > };
> > };
> > };
> >
> > &mdss_edp {
> > ports {
> > port at 1 {
> > reg = <1>;
> > mdss_edp_out: endpoint {
> > remote-endpoint = <&auo_b133han05_in>;
> > };
> > };
> > };
> > };
> >
> > > If you have "port at 1" pointing to a USB-C controller but this instance
> > > of the DP controller is actually hooked up straight to a panel then
> > > you should simply delete the "port at 1" that points to the typeC and
> > > replace it with one that points to a panel, right?
> > >
> >
> > As you can see, port 1 on &mdss_dp0 and &mdss_dp1 points to the two UCSI
> > connectors and the eDP points to the panel, exactly like we agreed.
> >
> > So now I call:
> > drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> >
> > which for the two DP nodes will pass respective UCSI connector to
> > drm_find_panel() and get EPROBE_DEFER back - because they are not on
> > panel_list.
>
> That's "good" right?
>
Well, it's expected that the connectors aren't panels...
> >
> > There's nothing indicating in the of_graph that the USB connectors
> > aren't panels (or bridges), so I don't see a way to distinguish the two
> > types remotes.
> >
>
> I'd like to create a bridge, not panel, for USB connectors, so that we
> can push sideband HPD signaling through to the DP driver. But either way
> this should work, right? If drm_of_find_panel_or_bridge() returns
> -EPROBE_DEFER, then assume the connector is DP. Otherwise if there's a
> valid pointer then treat it as eDP. We can't go too crazy though because
> once we attach a bridge we're assuming eDP which may not actually be
> true.
>
How will I be able to distinguish this from "the eDP panel is not yet
probed"? Unless we first implement the rest of this suggestion to make
sure drm_of_find_panel_or_bridge() has something to find in both cases.
> If we make a bridge for type-C USB connectors then we'll be able to use
> the drm_bridge_connector code to automatically figure out the connector
> type (eDP vs. DP vs. whatever else is chained onto the end of the DP
> connector). That would require updating the bridge connector code to
> treat DP as a connector type though. And then the eDP path would need to
> be handled when there's no bridge really involved, like in your case
> where the eDP hardware is directly connected to the eDP panel.
>
> In this case I think we're supposed to make a bridge in this DP driver
> itself that does pretty basic stuff and assumes the connector is eDP or
> DP based on the hardware type it is. Then if we wire a type-c connector
> up to the eDP hardware the eDP bridge we make in this driver will see a
> type-c connector that makes a bridge saying "I'm a DP connector" and the
> drm_bridge_connector code will look at the last bridge in the chain to
> see that it's actually a DP connector.
This is rather far from how I do handle USB, and its HPD interrupts
today. But perhaps I'm missing something there...
Let me get that patch on the list as well then.
Regards,
Bjorn
More information about the dri-devel
mailing list