LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: 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@vger.kernel.org, dri-devel@lists.freedesktop.org,
	freedreno@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers
Date: Tue, 5 Oct 2021 21:26:22 -0700	[thread overview]
Message-ID: <CAE-0n53TwEyycpAaWVpRUKPpos4z-gqwrvyUdgobh1V88VUsXg@mail.gmail.com> (raw)
In-Reply-To: <YV0MAF/Y5BR1e6My@ripper>

Quoting Bjorn Andersson (2021-10-05 19:37:52)
> On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> > >
> > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > > >         if (!dp)
> > > > >                 return -ENOMEM;
> > > > >
> > > > > -       desc = dp_display_get_desc(pdev);
> > > > > +       desc = dp_display_get_desc(pdev, &dp->id);
> > > >
> > > > I'm sad that dp->id has to match the number in the SoC specific
> > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > > already have INTF_DP macros, which makes me think that it may be better
> > > > to connect this to those arrays instead of making an msm_dp_desc
> > > > structure and then make sure the 'type' member matches a connector
> > > > type number. Otherwise this code is super fragile.
> > > >
> > >
> > > I'm afraid I don't understand what you're proposing. Or which part you
> > > consider fragile, the indices of the INTF_DP instances aren't going to
> > > move around...
> > >
> > > I have N instances of the DP driver that I need to match to N entries
> > > from the platform specific intf array, I need some stable reference
> > > between them. When I started this journey I figured I could rely on the
> > > of_graph between the DPU and the interface controllers, but the values
> > > used there today are just bogus, so that was a no go.
> > >
> > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > > come up with an identifier to put in h_tile_instance[0] so that
> > > dpu_encoder_setup_display() can find the relevant INTF.
> > >
> >
> > To make it more concrete we can look at sc7180
> >
> > static const struct dpu_intf_cfg sc7180_intf[] = {
> >         INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> >                                                      ^
> >                                                      |
> >
> > intf0 is irrelevant. Also the address is irrelevant. But here we have a
> > zero, the number after INTF_DP, and that is very relevant. That number
> > needs to match the dp->id. Somewhere we have a match between
> > controller_id and dp->id in the code.
>
> That number (the 0, not INTF_0) is what the code matches against dp->id
> in _dpu_kms_initialize_displayport(), in order to figure out that this
> is INTF_0 in dpu_encoder_setup_display().
>
> I.e. look at the sc8180x patch:
>
> INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
> INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
> INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
>
> Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
> which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
>

Yep. I'm saying that having to make that number in this intf array match
the order of the register mapping descriptor array is fragile. Why can't
we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
map from the descriptor array to this intf array somehow so that the
order of the descriptor array doesn't matter? Then we don't have to put
the connector type in the descriptor array, and we don't have to keep
the order of the array a certain way to match this intf descriptor.

Maybe

	struct msm_dp_desc {
		phys_addr_t io_start;
		unsigned int id;
	};

and then have msm_dp_desc::id equal INTF_<N> and then look through the
intf from DPU here in the DP driver to find the id and type of connector
that should be used by default? Still sort of fragile because the only
connection is an unsigned int which isn't great, but at least it's
explicit instead of implicit based on the array order.

  reply	other threads:[~2021-10-06  4:26 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-05 23:13 [PATCH v4 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
2021-10-05 23:13 ` [PATCH v4 1/7] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
2021-10-05 23:13 ` [PATCH v4 2/7] drm/msm/dp: Modify prototype of encoder based API Bjorn Andersson
2021-10-05 23:13 ` [PATCH v4 3/7] drm/msm/dp: Allow specifying connector_type per controller Bjorn Andersson
2021-10-06  0:15   ` Stephen Boyd
2021-10-06  0:29   ` Stephen Boyd
2021-10-06  3:35     ` Bjorn Andersson
2021-10-06  0:31   ` Stephen Boyd
2021-10-05 23:13 ` [PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel Bjorn Andersson
2021-10-06  0:35   ` [Freedreno] " abhinavk
2021-10-06  2:09     ` Bjorn Andersson
2021-10-06  3:09       ` abhinavk
2021-10-08 15:01     ` Doug Anderson
2021-10-05 23:13 ` [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers Bjorn Andersson
2021-10-06  0:43   ` Stephen Boyd
2021-10-06  1:43     ` Bjorn Andersson
2021-10-06  2:06       ` Stephen Boyd
2021-10-06  2:37         ` Bjorn Andersson
2021-10-06  4:26           ` Stephen Boyd [this message]
2021-10-06  6:10             ` Dmitry Baryshkov
2021-10-06  7:06               ` Stephen Boyd
2021-10-06 11:35                 ` Dmitry Baryshkov
2021-10-06 17:07             ` Bjorn Andersson
2021-10-06 17:19               ` Stephen Boyd
2021-10-06 18:05                 ` Bjorn Andersson
2021-10-06 18:59                   ` Stephen Boyd
2021-10-06 20:39                     ` Bjorn Andersson
2021-10-07 22:29                       ` abhinavk
2021-10-06 17:19               ` Dmitry Baryshkov
2021-10-06 18:37                 ` Bjorn Andersson
2021-10-05 23:13 ` [PATCH v4 6/7] dt-bindings: msm/dp: Add SC8180x compatibles Bjorn Andersson
2021-10-05 23:13 ` [PATCH v4 7/7] drm/msm/dp: Add sc8180x DP controllers Bjorn Andersson
2021-10-06  0:36   ` Stephen Boyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAE-0n53TwEyycpAaWVpRUKPpos4z-gqwrvyUdgobh1V88VUsXg@mail.gmail.com \
    --to=swboyd@chromium.org \
    --cc=abhinavk@codeaurora.org \
    --cc=airlied@linux.ie \
    --cc=bjorn.andersson@linaro.org \
    --cc=daniel@ffwll.ch \
    --cc=dmitry.baryshkov@linaro.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=kalyan_t@codeaurora.org \
    --cc=khsieh@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robdclark@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=sean@poorly.run \
    --subject='Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).