From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Doug Anderson <dianders@chromium.org>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>,
Jonas Karlman <jonas@kwiboo.se>,
Neil Armstrong <narmstrong@baylibre.com>,
dri-devel <dri-devel@lists.freedesktop.org>,
Stephen Boyd <swboyd@chromium.org>,
linux-renesas-soc@vger.kernel.org,
Andrzej Hajda <a.hajda@samsung.com>
Subject: Re: [RFC PATCH 08/11] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
Date: Fri, 26 Mar 2021 03:40:04 +0200 [thread overview]
Message-ID: <YF07dHgNlK1RqVUA@pendragon.ideasonboard.com> (raw)
In-Reply-To: <CAD=FV=UPqg0CnA1ZFR70Ym+m6ROemdFbYwk_=C3+SemP1X9hYw@mail.gmail.com>
Hi Doug,
On Wed, Mar 24, 2021 at 03:46:28PM -0700, Doug Anderson wrote:
> On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote:
> >
> > Implement the bridge connector-related .get_edid() operation, and report
> > the related bridge capabilities and type. The .get_edid() operation is
> > implemented with the same backend as the EDID retrieval from the
> > connector .get_modes() operation.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 30 ++++++++++++++++++++++-----
> > 1 file changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index dc300fab4319..6f6e075544e8 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -261,6 +261,18 @@ static void ti_sn_debugfs_remove(struct ti_sn_bridge *pdata)
> > pdata->debugfs = NULL;
> > }
> >
> > +static struct edid *__ti_sn_bridge_get_edid(struct ti_sn_bridge *pdata,
> > + struct drm_connector *connector)
> > +{
> > + struct edid *edid;
> > +
> > + pm_runtime_get_sync(pdata->dev);
> > + edid = drm_get_edid(connector, &pdata->aux.ddc);
> > + pm_runtime_put(pdata->dev);
>
> As mentioned in my patch [1], the above is a bit iffy for eDP.
> Specifically just doing a pm_runtime_get_sync() isn't enough to
> actually be able to read the EDID. We also need to do any panel power
> sequencing and potentially set the "ignore HPD" bit in the bridge to
> actually read the EDID.
>
> I'll try to find some time to make this better--let's see if I get
> distracted before I manage it.
I've replied to your review of 05/11 with a possible solution. Fingers
crossed :-)
> > +
> > + return edid;
> > +}
> > +
> > /* -----------------------------------------------------------------------------
> > * DRM Connector Operations
> > */
> > @@ -277,11 +289,8 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> > struct edid *edid = pdata->edid;
> > int num, ret;
> >
> > - if (!edid) {
> > - pm_runtime_get_sync(pdata->dev);
> > - edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
> > - pm_runtime_put(pdata->dev);
> > - }
> > + if (!edid)
> > + edid = pdata->edid = __ti_sn_bridge_get_edid(pdata, connector);
>
> It feels weird to me that we are now exposing the get_edid() function
> directly but we still need to implement get_modes(). I guess this is
> because we might need to fallback to the hardcoded modes? ...but that
> seems like it would be a common pattern for eDP bridges...
Bridges are moving from creating the connector internally to providing a
set of bridge operations to support connector creation externally (by
the drm_bridge_connector helper, or by display drivers directly if
needed). During the transition, both need to be implemented, hence the
bridge .get_edid() operation for the new model, and the connector
.get_modes() operation for the old model.
> > if (edid && drm_edid_is_valid(edid)) {
> > ret = drm_connector_update_edid_property(connector, edid);
> > @@ -871,12 +880,21 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> > pm_runtime_put_sync(pdata->dev);
> > }
> >
> > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > + struct drm_connector *connector)
> > +{
> > + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> > +
> > + return __ti_sn_bridge_get_edid(pdata, connector);
> > +}
> > +
> > static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> > .attach = ti_sn_bridge_attach,
> > .pre_enable = ti_sn_bridge_pre_enable,
> > .enable = ti_sn_bridge_enable,
> > .disable = ti_sn_bridge_disable,
> > .post_disable = ti_sn_bridge_post_disable,
> > + .get_edid = ti_sn_bridge_get_edid,
> > };
> >
> > /* -----------------------------------------------------------------------------
> > @@ -1335,6 +1353,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> >
> > pdata->bridge.funcs = &ti_sn_bridge_funcs;
> > pdata->bridge.of_node = client->dev.of_node;
> > + pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > + pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
>
> Even with the few comments above, I think this is still fine to move
> us in the right direction. Unless I manage to fix up the EDID reading
> (in which case your patch would conflict and would need to be
> tweaked), then:
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
>
> [1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-03-26 1:40 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-22 3:01 [RFC PATCH 00/11] drm/bridge: ti-sn65dsi86: Support DisplayPort mode Laurent Pinchart
2021-03-22 3:01 ` [RFC PATCH 01/11] dt-bindings: drm/bridge: ti-sn65dsi8: Make enable GPIO optional Laurent Pinchart
2021-03-22 10:29 ` Jagan Teki
2021-03-23 7:10 ` Stephen Boyd
2021-03-23 21:08 ` Doug Anderson
2021-03-27 16:42 ` Rob Herring
2021-03-22 3:01 ` [RFC PATCH 02/11] drm/bridge: ti-sn65dsi86: " Laurent Pinchart
2021-03-22 10:29 ` Jagan Teki
2021-03-23 7:10 ` Stephen Boyd
2021-03-23 21:08 ` Doug Anderson
2021-03-22 3:01 ` [RFC PATCH 03/11] drm/bridge: ti-sn65dsi86: Unregister AUX adapter in remove() Laurent Pinchart
2021-03-23 7:10 ` Stephen Boyd
2021-03-23 21:08 ` Doug Anderson
2021-03-23 21:41 ` Laurent Pinchart
2021-03-23 22:55 ` Doug Anderson
2021-03-23 23:02 ` Laurent Pinchart
2021-03-26 0:43 ` Doug Anderson
2021-03-26 1:01 ` Laurent Pinchart
2021-03-22 3:01 ` [RFC PATCH 04/11] drm/bridge: ti-sn65dsi86: Use bitmask to store valid rates Laurent Pinchart
2021-03-23 7:11 ` Stephen Boyd
2021-03-23 21:08 ` Doug Anderson
2021-03-23 21:45 ` Laurent Pinchart
2021-03-23 22:45 ` Doug Anderson
2021-03-24 8:47 ` Geert Uytterhoeven
2021-03-22 3:01 ` [RFC PATCH 05/11] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge Laurent Pinchart
2021-03-22 10:19 ` Jagan Teki
2021-03-23 7:14 ` Stephen Boyd
2021-03-23 21:50 ` Laurent Pinchart
2021-03-24 22:44 ` Doug Anderson
2021-03-26 1:06 ` Laurent Pinchart
2021-03-22 3:01 ` [RFC PATCH 06/11] drm/bridge: ti-sn65dsi86: Group code in sections Laurent Pinchart
2021-03-23 7:14 ` Stephen Boyd
2021-03-24 22:44 ` Doug Anderson
2021-03-22 3:01 ` [RFC PATCH 07/11] drm/bridge: ti-sn65dsi86: Split connector creation to a function Laurent Pinchart
2021-03-23 7:15 ` Stephen Boyd
2021-03-24 22:44 ` Doug Anderson
2021-03-22 3:01 ` [RFC PATCH 08/11] drm/bridge: ti-sn65dsi86: Implement bridge connector operations Laurent Pinchart
2021-03-23 7:15 ` Stephen Boyd
2021-03-24 22:46 ` Doug Anderson
2021-03-26 1:40 ` Laurent Pinchart [this message]
2021-03-22 3:01 ` [RFC PATCH 09/11] drm/bridge: ti-sn65dsi86: Make connector creation optional Laurent Pinchart
2021-03-22 3:01 ` [RFC PATCH 10/11] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode Laurent Pinchart
2021-03-24 22:47 ` Doug Anderson
2021-06-23 13:59 ` Laurent Pinchart
2022-02-23 18:04 ` Kieran Bingham
2022-02-23 18:20 ` Doug Anderson
2022-03-04 15:49 ` Laurent Pinchart
2021-03-22 3:01 ` [RFC PATCH 11/11] drm/bridge: ti-sn65dsi86: Support hotplug detection Laurent Pinchart
2021-03-23 7:21 ` Stephen Boyd
2021-03-24 22:47 ` Doug Anderson
2021-06-23 23:25 ` Laurent Pinchart
2021-06-23 23:51 ` Doug Anderson
2022-02-23 17:43 ` Kieran Bingham
2022-02-23 18:25 ` Doug Anderson
2022-03-04 15:45 ` Kieran Bingham
2022-03-04 16:30 ` Doug Anderson
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=YF07dHgNlK1RqVUA@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=a.hajda@samsung.com \
--cc=dianders@chromium.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=jernej.skrabec@siol.net \
--cc=jonas@kwiboo.se \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=narmstrong@baylibre.com \
--cc=swboyd@chromium.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).