Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230807061115.3244501-1-victor.liu@nxp.com>
In-Reply-To: <20230807061115.3244501-1-victor.liu@nxp.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Wed, 9 Aug 2023 15:53:51 +0200
Message-ID: <CAPDyKFpyQqojAFtmY5NP1gQjL9ag1V-kHLUBRAx2UHBZ6rgVSg@mail.gmail.com>
Subject: Re: [PATCH v3] drm/bridge: panel: Add a device link between drm
 device and panel device
To:     Liu Ying <victor.liu@nxp.com>
Cc:     dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        andrzej.hajda@intel.com, neil.armstrong@linaro.org,
        rfoss@kernel.org, Laurent.pinchart@ideasonboard.com,
        jonas@kwiboo.se, jernej.skrabec@gmail.com, airlied@gmail.com,
        daniel@ffwll.ch
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 7 Aug 2023 at 08:06, Liu Ying <victor.liu@nxp.com> wrote:
>
> Add the device link when panel bridge is attached and delete the link
> when panel bridge is detached.  The drm device is the consumer while
> the panel device is the supplier.  This makes sure that the drm device
> suspends eariler and resumes later than the panel device, hence resolves
> problems where the order is reversed, like the problematic case mentioned
> in the below link.
>
> Link: https://lore.kernel.org/lkml/CAPDyKFr0XjrU_udKoUKQ_q8RWaUkyqL+8fV-7s1CTMqi7u3-Rg@mail.gmail.com/T/
> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>

Looks good to me! Just a minor question though, don't we need to
manage runtime PM too - or this is solely for system wide
suspend/resume?

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
> v2->v3:
> * Improve commit message s/swapped/reversed/.
>
> v1->v2:
> * Fix bailout for panel_bridge_attach() in case device_link_add() fails.
>
>  drivers/gpu/drm/bridge/panel.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 9316384b4474..a6587d233505 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -4,6 +4,8 @@
>   * Copyright (C) 2017 Broadcom
>   */
>
> +#include <linux/device.h>
> +
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_connector.h>
> @@ -19,6 +21,7 @@ struct panel_bridge {
>         struct drm_bridge bridge;
>         struct drm_connector connector;
>         struct drm_panel *panel;
> +       struct device_link *link;
>         u32 connector_type;
>  };
>
> @@ -60,6 +63,8 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>  {
>         struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>         struct drm_connector *connector = &panel_bridge->connector;
> +       struct drm_panel *panel = panel_bridge->panel;
> +       struct drm_device *drm_dev = bridge->dev;
>         int ret;
>
>         if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> @@ -70,6 +75,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>                 return -ENODEV;
>         }
>
> +       panel_bridge->link = device_link_add(drm_dev->dev, panel->dev,
> +                                            DL_FLAG_STATELESS);
> +       if (!panel_bridge->link) {
> +               DRM_ERROR("Failed to add device link between %s and %s\n",
> +                         dev_name(drm_dev->dev), dev_name(panel->dev));
> +               return -EINVAL;
> +       }
> +
>         drm_connector_helper_add(connector,
>                                  &panel_bridge_connector_helper_funcs);
>
> @@ -78,6 +91,7 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>                                  panel_bridge->connector_type);
>         if (ret) {
>                 DRM_ERROR("Failed to initialize connector\n");
> +               device_link_del(panel_bridge->link);
>                 return ret;
>         }
>
> @@ -100,6 +114,8 @@ static void panel_bridge_detach(struct drm_bridge *bridge)
>         struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>         struct drm_connector *connector = &panel_bridge->connector;
>
> +       device_link_del(panel_bridge->link);
> +
>         /*
>          * Cleanup the connector if we know it was initialized.
>          *
> --
> 2.37.1
>