[Freedreno] [PATCH v2.5] drm/msm/dsi: switch to DRM_PANEL_BRIDGE
Abhinav Kumar
quic_abhinavk at quicinc.com
Sat Aug 27 21:34:56 UTC 2022
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)
>
More information about the dri-devel
mailing list