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

[Abhinav Kumar]

On 8/22/2022 10:53 AM, Dmitry Baryshkov wrote:
> 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 at linaro.org>
>>> ---
>>>
>>> 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]

Yes, got it.
> 
>>> *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.
> 
Thanks for pushing this change, I have R-bed that too.

For this one, now I can

Reviewed-by: Abhinav Kumar <quic_abhinavk at quicinc.com>

>>
>>> -        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)
>