Re: [PATCH v2 3/5] drm/msm/dp: Support up to 3 DP controllers

From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Stephen Boyd <swboyd@chromium.org>,
	Abhinav Kumar <abhinavk@codeaurora.org>,
	Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Kalyan Thota <kalyan_t@codeaurora.org>,
	Kuogee Hsieh <khsieh@codeaurora.org>,
	Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
	Rob Herring <robh+dt@kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	freedreno <freedreno@lists.freedesktop.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/5] drm/msm/dp: Support up to 3 DP controllers
Date: Fri, 1 Oct 2021 07:18:50 -0700	[thread overview]
Message-ID: <YVcYyvtVfOx977an@ripper> (raw)
In-Reply-To: <CAD=FV=UWu4CBC8S8C9P7r=q+F=JqWMzTVAPyzWb+AT4QHbULZQ@mail.gmail.com>

On Fri 01 Oct 06:58 PDT 2021, Doug Anderson wrote:

> Hi,
> 
> On Thu, Aug 26, 2021 at 10:20 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Bjorn Andersson (2021-08-25 16:42:31)
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index 2c7de43f655a..4a6132c18e57 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -78,6 +78,8 @@ struct dp_display_private {
> > >         char *name;
> > >         int irq;
> > >
> > > +       int id;
> > > +
> > >         /* state variables */
> > >         bool core_initialized;
> > >         bool hpd_irq_on;
> > > @@ -115,8 +117,19 @@ struct dp_display_private {
> > >         struct dp_audio *audio;
> > >  };
> > >
> > > +
> > > +struct msm_dp_config {
> > > +       phys_addr_t io_start[3];
> >
> > Can this be made into another struct, like msm_dp_desc, that also
> > indicates what type of DP connector it is, i.e. eDP vs DP? That would
> > help me understand in modetest and /sys/class/drm what sort of connector
> > is probing. dp_drm_connector_init() would need to pass the type of
> > connector appropriately. Right now, eDP connectors still show up as DP
> > instead of eDP in sysfs.
> 
> I'm not convinced that's the right way to do it. I think the right way
> forward here is to look at whether drm_of_find_panel_or_bridge() finds
> a panel. If it finds a panel then we're eDP. If it doesn't then we're
> DP. That matches roughly what Laurent was planning to do for
> ti-sn65dsi86:
> 
> https://lore.kernel.org/all/20210322030128.2283-11-laurent.pinchart+renesas@ideasonboard.com/
> 

When we spoke about this earlier I got the impression that there was
interest in representing the DP display as a panel at some point in the
future. Did I misunderstand you or would we simply update the scheme
when that day comes?

I updated the patch based on Stephen's input and it looks nice, but I
could certainly respin it again to simply rely on us having an explicit
panel or not.

> I know technically an eDP and DP controller can have different sets of
> features but I think what we really are trying to communicate to
> modetest is whether an internal panel or external display is
> connected, right?
> 

For me Stephen's suggestion resulted in the monitor names in Wayland
suddenly making more sense, i.e. it makes more sense to say that the lid
on my laptop should control eDP-1, rather than DP-3 on this machine...

Regards,
Bjorn