From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Douglas Anderson <dianders@chromium.org>
Cc: dri-devel@lists.freedesktop.org, Daniel Vetter <daniel@ffwll.ch>,
Javier Martinez Canillas <javierm@redhat.com>,
robert.foss@linaro.org, lschyi@chromium.org,
Sam Ravnborg <sam@ravnborg.org>,
jjsu@chromium.org, Andrzej Hajda <andrzej.hajda@intel.com>,
David Airlie <airlied@linux.ie>,
Jernej Skrabec <jernej.skrabec@gmail.com>,
Jonas Karlman <jonas@kwiboo.se>,
Neil Armstrong <narmstrong@baylibre.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] drm/bridge: ti-sn65dsi86: Use drm_bridge_connector
Date: Tue, 8 Feb 2022 03:50:14 +0200 [thread overview]
Message-ID: <YgHMVneO/Y/qopEX@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20220204161245.v2.1.I3ab26b7f197cc56c874246a43e57913e9c2c1028@changeid>
Hi Doug,
Thank you for the patch.
On Fri, Feb 04, 2022 at 04:13:40PM -0800, Douglas Anderson wrote:
> The ti-sn65dsi86 driver shouldn't hand-roll its own bridge
> connector. It should use the normal drm_bridge_connector. Let's switch
> to do that, removing all of the custom code.
>
> NOTE: this still _doesn't_ implement DRM_BRIDGE_ATTACH_NO_CONNECTOR
> support for ti-sn65dsi86 and that would still be a useful thing to do
> in the future. It was attempted in the past [1] but put on the back
> burner. However, unless we instantly change ti-sn65dsi86 fully from
> not supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR at all to _only_
> supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR then we'll still need a bit
> of time when we support both. This is a better way to support the old
> way where the driver hand rolls things itself.
>
> A new notes about the implementation here:
> * When using the drm_bridge_connector the connector should be created
> after all the bridges, so we change the ordering a bit.
> * I'm reasonably certain that we don't need to do anything to "free"
> the new drm_bridge_connector. If drm_bridge_connector_init() returns
> success then we know drm_connector_init() was called with the
> `drm_bridge_connector_funcs`. The `drm_bridge_connector_funcs` has a
> .destroy() that does all the cleanup. drm_connector_init() calls
> __drm_mode_object_add() with a drm_connector_free() that will call
> the .destroy().
> * I'm also reasonably certain that I don't need to "undo" the
> drm_bridge_attach() if drm_bridge_connector_init() fails. The
> "detach" function is private and other similar code doesn't try to
> undo the drm_bridge_attach() in error cases. There's also a comment
> indicating the lack of balance at the top of drm_bridge_attach().
>
> [1] https://lore.kernel.org/r/20210920225801.227211-4-robdclark@gmail.com
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>
> Changes in v2:
> - ("ti-sn65dsi86: Use drm_bridge_connector") new for v2.
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 72 ++++++---------------------
> 1 file changed, 14 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index ba136a188be7..38616aab12ac 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -26,6 +26,7 @@
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
> #include <drm/dp/drm_dp_aux_bus.h>
> #include <drm/dp/drm_dp_helper.h>
> #include <drm/drm_mipi_dsi.h>
> @@ -174,7 +175,7 @@ struct ti_sn65dsi86 {
> struct regmap *regmap;
> struct drm_dp_aux aux;
> struct drm_bridge bridge;
> - struct drm_connector connector;
> + struct drm_connector *connector;
> struct device_node *host_node;
> struct mipi_dsi_device *dsi;
> struct clk *refclk;
> @@ -646,54 +647,6 @@ static struct auxiliary_driver ti_sn_aux_driver = {
> .id_table = ti_sn_aux_id_table,
> };
>
> -/* -----------------------------------------------------------------------------
> - * DRM Connector Operations
> - */
> -
> -static struct ti_sn65dsi86 *
> -connector_to_ti_sn65dsi86(struct drm_connector *connector)
> -{
> - return container_of(connector, struct ti_sn65dsi86, connector);
> -}
> -
> -static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> -{
> - struct ti_sn65dsi86 *pdata = connector_to_ti_sn65dsi86(connector);
> -
> - return drm_bridge_get_modes(pdata->next_bridge, connector);
> -}
> -
> -static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
> - .get_modes = ti_sn_bridge_connector_get_modes,
> -};
> -
> -static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
> - .fill_modes = drm_helper_probe_single_connector_modes,
> - .destroy = drm_connector_cleanup,
> - .reset = drm_atomic_helper_connector_reset,
> - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -};
> -
> -static int ti_sn_bridge_connector_init(struct ti_sn65dsi86 *pdata)
> -{
> - int ret;
> -
> - ret = drm_connector_init(pdata->bridge.dev, &pdata->connector,
> - &ti_sn_bridge_connector_funcs,
> - DRM_MODE_CONNECTOR_eDP);
> - if (ret) {
> - DRM_ERROR("Failed to initialize connector with drm\n");
> - return ret;
> - }
> -
> - drm_connector_helper_add(&pdata->connector,
> - &ti_sn_bridge_connector_helper_funcs);
> - drm_connector_attach_encoder(&pdata->connector, pdata->bridge.encoder);
> -
> - return 0;
> -}
> -
> /*------------------------------------------------------------------------------
> * DRM Bridge
> */
> @@ -757,10 +710,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> return ret;
> }
>
> - ret = ti_sn_bridge_connector_init(pdata);
> - if (ret < 0)
> - goto err_conn_init;
> -
> /* We never want the next bridge to *also* create a connector: */
> flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
>
> @@ -768,13 +717,20 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
> &pdata->bridge, flags);
> if (ret < 0)
> - goto err_dsi_host;
> + goto err_initted_aux;
> +
> + pdata->connector = drm_bridge_connector_init(pdata->bridge.dev,
> + pdata->bridge.encoder);
> + if (IS_ERR(pdata->connector)) {
> + ret = PTR_ERR(pdata->connector);
> + goto err_initted_aux;
> + }
> +
> + drm_connector_attach_encoder(pdata->connector, pdata->bridge.encoder);
>
> return 0;
>
> -err_dsi_host:
> - drm_connector_cleanup(&pdata->connector);
> -err_conn_init:
> +err_initted_aux:
> drm_dp_aux_unregister(&pdata->aux);
> return ret;
> }
> @@ -824,7 +780,7 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
>
> static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> {
> - if (pdata->connector.display_info.bpc <= 6)
> + if (pdata->connector->display_info.bpc <= 6)
> return 18;
> else
> return 24;
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2022-02-08 1:53 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-05 0:13 [PATCH v2 0/3] drm/panel-edp: Debugfs for panel-edp Douglas Anderson
2022-02-05 0:13 ` [PATCH v2 1/3] drm/bridge: ti-sn65dsi86: Use drm_bridge_connector Douglas Anderson
2022-02-08 1:50 ` Laurent Pinchart [this message]
2022-02-05 0:13 ` [PATCH v2 2/3] drm: Plumb debugfs_init through to panels Douglas Anderson
2022-02-08 1:53 ` Laurent Pinchart
2022-02-15 22:09 ` Javier Martinez Canillas
2022-02-15 22:20 ` Andrzej Hajda
2022-02-15 23:11 ` Doug Anderson
2022-02-16 9:25 ` Jani Nikula
2022-02-16 9:35 ` Javier Martinez Canillas
2022-02-16 11:42 ` Andrzej Hajda
2022-02-22 23:47 ` Doug Anderson
2022-02-05 0:13 ` [PATCH v2 3/3] drm/panel-edp: Allow querying the detected panel via debugfs Douglas Anderson
2022-02-15 22:10 ` Javier Martinez Canillas
2022-02-15 23:31 ` [PATCH v2 0/3] drm/panel-edp: Debugfs for panel-edp Doug Anderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YgHMVneO/Y/qopEX@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=airlied@linux.ie \
--cc=andrzej.hajda@intel.com \
--cc=daniel@ffwll.ch \
--cc=dianders@chromium.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=javierm@redhat.com \
--cc=jernej.skrabec@gmail.com \
--cc=jjsu@chromium.org \
--cc=jonas@kwiboo.se \
--cc=linux-kernel@vger.kernel.org \
--cc=lschyi@chromium.org \
--cc=narmstrong@baylibre.com \
--cc=robert.foss@linaro.org \
--cc=sam@ravnborg.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).