Re: [RFC PATCH 08/11] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Laurent Pinchart <laurent.pinchart@ideasonboard.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