Re: [Freedreno] [PATCH v2.5] drm/msm/dsi: switch to DRM_PANEL_BRIDGE

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 15/07/2022 00:54, Abhinav Kumar wrote:
> On 7/12/2022 6:22 AM, Dmitry Baryshkov wrote:
> > Currently the DSI driver has two separate paths: one if the next device
> > in a chain is a bridge and another one if the panel is connected
> > directly to the DSI host. Simplify the code path by using panel-bridge
> > driver (already selected in Kconfig) and dropping support for
> > handling the panel directly.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >
> > I'm not sending this as a separate patchset (I'd like to sort out mdp5
> > first), but more of a preview of changes related to
> > msm_dsi_manager_ext_bridge_init().
> >
> > ---
> >   drivers/gpu/drm/msm/dsi/dsi.c         |  35 +---
> >   drivers/gpu/drm/msm/dsi/dsi.h         |  16 +-
> >   drivers/gpu/drm/msm/dsi/dsi_host.c    |  25 ---
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 283 +++-----------------------
> >   4 files changed, 36 insertions(+), 323 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi.c 
> > b/drivers/gpu/drm/msm/dsi/dsi.c
> > index 1625328fa430..4edb9167e600 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> > @@ -6,14 +6,6 @@
> >   #include "dsi.h"
> >   #include "dsi_cfg.h"
> > -struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi)
> > -{
> > -    if (!msm_dsi || !msm_dsi_device_connected(msm_dsi))
> > -        return NULL;
> > -
> > -    return msm_dsi->encoder;
> > -}
> > -
> >   bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi)
> >   {
> >       unsigned long host_flags = 
> > msm_dsi_host_get_mode_flags(msm_dsi->host);
> > @@ -220,7 +212,6 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, 
> > struct drm_device *dev,
> >                struct drm_encoder *encoder)
> >   {
> >       struct msm_drm_private *priv;
> > -    struct drm_bridge *ext_bridge;
> >       int ret;
> >       if (WARN_ON(!encoder) || WARN_ON(!msm_dsi) || WARN_ON(!dev))
> > @@ -254,26 +245,10 @@ int msm_dsi_modeset_init(struct msm_dsi 
> > *msm_dsi, struct drm_device *dev,
> >           goto fail;
> >       }
> > -    /*
> > -     * check if the dsi encoder output is connected to a panel or an
> > -     * external bridge. We create a connector only if we're connected 
> > to a
> > -     * drm_panel device. When we're connected to an external bridge, we
> > -     * assume that the drm_bridge driver will create the connector 
> > itself.
> > -     */
> > -    ext_bridge = msm_dsi_host_get_bridge(msm_dsi->host);
> > -
> > -    if (ext_bridge)
> > -        msm_dsi->connector =
> > -            msm_dsi_manager_ext_bridge_init(msm_dsi->id);
> > -    else
> > -        msm_dsi->connector =
> > -            msm_dsi_manager_connector_init(msm_dsi->id);
> > -
> > -    if (IS_ERR(msm_dsi->connector)) {
> > -        ret = PTR_ERR(msm_dsi->connector);
> > +    ret = msm_dsi_manager_ext_bridge_init(msm_dsi->id);
> > +    if (ret) {
> >           DRM_DEV_ERROR(dev->dev,
> >               "failed to create dsi connector: %d\n", ret);
> > -        msm_dsi->connector = NULL;
> >           goto fail;
> >       }
> > @@ -287,12 +262,6 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, 
> > struct drm_device *dev,
> >           msm_dsi->bridge = NULL;
> >       }
> > -    /* don't destroy connector if we didn't make it */
> > -    if (msm_dsi->connector && !msm_dsi->external_bridge)
> > -        msm_dsi->connector->funcs->destroy(msm_dsi->connector);
> > -
> > -    msm_dsi->connector = NULL;
>
>  From what i can see all the usages of msm_dsi->connector are removed 
> after this change. So can we drop that?

The connector field is dropped from the msm_dsi struct. If you are 
asking about the msm_dsi_modeset_init(), we can not drop it since we 
require the DRM device with GEM being initialized in order to allocate 
DSI DMA buffer. We can think about moving DMA buffer allocation towards 
the usage point, however this is definitely a separate commit.

[skipped]

> > *msm_dsi_manager_ext_bridge_init(u8 id)
> >       ret = drm_bridge_attach(encoder, ext_bridge, int_bridge,
> >               DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >       if (ret == -EINVAL) {
> > -        struct drm_connector *connector;
> > -        struct list_head *connector_list;
> > -
> > -        /* link the internal dsi bridge to the external bridge */
> > -        drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
> > -
> >           /*
> > -         * we need the drm_connector created by the external bridge
> > -         * driver (or someone else) to feed it to our driver's
> > -         * priv->connector[] list, mainly for msm_fbdev_init()
> > +         * link the internal dsi bridge to the external bridge,
> > +         * connector is created by the next bridge.
> >            */
> > -        connector_list = &dev->mode_config.connector_list;
> > +        ret = drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
> > +        if (ret < 0)
> > +            return ret;
> > +    } else {
> > +        struct drm_connector *connector;
> > -        list_for_each_entry(connector, connector_list, head) {
> > -            if (drm_connector_has_possible_encoder(connector, encoder))
> > -                return connector;
> > +        /* We are in charge of the connector, create one now. */
> > +        connector = drm_bridge_connector_init(dev, encoder);
> > +        if (IS_ERR(connector)) {
> > +            DRM_ERROR("Unable to create bridge connector\n");
> > +            return PTR_ERR(connector);
> >           }
>
> Ok, I understood now. We create the connector using 
> drm_bridge_connector_init() only when the brige doesnt create one already.
>
> In both cases since now we are leaving the hpd handling to the next 
> bridge, like I was suggesting, the dsi_hpd_worker() etc can be dropped 
> now. Because anyway without setting the DRM_CONNECTOR_POLL_HPD, event 
> will not be sent to usermode.

I've submitted https://patchwork.freedesktop.org/series/107564/ as a 
separate change.

> > -        return ERR_PTR(-ENODEV);
> > -    }
> > -
> > -    connector = drm_bridge_connector_init(dev, encoder);
> > -    if (IS_ERR(connector)) {
> > -        DRM_ERROR("Unable to create bridge connector\n");
> > -        return ERR_CAST(connector);
> > +        ret = drm_connector_attach_encoder(connector, encoder);
> > +        if (ret < 0)
> > +            return ret;
> >       }
> > -    drm_connector_attach_encoder(connector, encoder);
> > +    /* The pipeline is ready, ping encoders if necessary */
> > +    msm_dsi_manager_set_split_display(id);
> > -    return connector;
> > +    return 0;
> >   }
> >   void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)

--
With best wishes
Dmitry