[PATCH v2 0/1] bridge/ti-sn65dsi86.c: convert to atomic operations

[PATCH v2 0/1] bridge/ti-sn65dsi86.c: convert to atomic operations

[Sam Ravnborg]

This patch is what is left on a previous attempt to make the
ti-sn65dsi86 support DRM_BRIDGE_ATTACH_NO_CONNECTOR.

I ran into several challenges on the way:
- I never found a way to retreive bpc when the driver
  do not create the connector - and the driver needs it.
  There may be ways to change how bpc is used but as I have no access
  to HW it is not easy to change much.
- My understanding of the driver is limited so I dropped trying to
  do the proper implmentation of format negotiation
- I had made a small macro to encapsulate a few of the common functions
  in drm_bridge_funcs - but looking at it with fresh eyes told me
  this is not a good idea.

I dropped previous a-b / r-b as the driver has changed a bit since
last submission.
Especially to Kieran - sorry for letting you wait so long for almost nothing.

Previous attempt:
https://lore.kernel.org/dri-devel/20220206154405.1243333-1-sam@ravnborg.org/

	Sam


Sam Ravnborg (1):
      drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

[PATCH v2 1/1] drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs

[Sam Ravnborg]

Move away from the deprecated enable/disable operations in
drm_bridge_funcs and enable atomic use.

v3:
 - Drop use of DRM_BRIDGE_STATE_OPS

v2:
 - fix build (kernel test robot <lkp@intel.com>)

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index c2b9227f7042..d6dd4d99a229 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -752,7 +752,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-static void ti_sn_bridge_disable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
+					struct drm_bridge_state *old_bridge_state)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
@@ -1011,7 +1012,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
 	return ret;
 }
 
-static void ti_sn_bridge_enable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
+				       struct drm_bridge_state *old_bridge_state)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 	const char *last_err_str = "No supported DP rate";
@@ -1080,7 +1082,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 			   VSTREAM_ENABLE);
 }
 
-static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *old_bridge_state)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
@@ -1093,7 +1096,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 	usleep_range(100, 110);
 }
 
-static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
+					     struct drm_bridge_state *old_bridge_state)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
@@ -1114,10 +1118,13 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.attach = ti_sn_bridge_attach,
 	.detach = ti_sn_bridge_detach,
 	.mode_valid = ti_sn_bridge_mode_valid,
-	.pre_enable = ti_sn_bridge_pre_enable,
-	.enable = ti_sn_bridge_enable,
-	.disable = ti_sn_bridge_disable,
-	.post_disable = ti_sn_bridge_post_disable,
+	.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
+	.atomic_enable = ti_sn_bridge_atomic_enable,
+	.atomic_disable = ti_sn_bridge_atomic_disable,
+	.atomic_post_disable = ti_sn_bridge_atomic_post_disable,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 };
 
 static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
-- 
2.34.1

Re: [PATCH v2 1/1] drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs

[Robert Foss]

On Sun, 3 Jul 2022 at 20:40, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Move away from the deprecated enable/disable operations in
> drm_bridge_funcs and enable atomic use.
>
> v3:
>  - Drop use of DRM_BRIDGE_STATE_OPS
>
> v2:
>  - fix build (kernel test robot <lkp@intel.com>)
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c2b9227f7042..d6dd4d99a229 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -752,7 +752,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
>         return MODE_OK;
>  }
>
> -static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
> +                                       struct drm_bridge_state *old_bridge_state)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1011,7 +1012,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>         return ret;
>  }
>
> -static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> +                                      struct drm_bridge_state *old_bridge_state)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>         const char *last_err_str = "No supported DP rate";
> @@ -1080,7 +1082,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>                            VSTREAM_ENABLE);
>  }
>
> -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> +                                          struct drm_bridge_state *old_bridge_state)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1093,7 +1096,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
>         usleep_range(100, 110);
>  }
>
> -static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> +                                            struct drm_bridge_state *old_bridge_state)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1114,10 +1118,13 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>         .attach = ti_sn_bridge_attach,
>         .detach = ti_sn_bridge_detach,
>         .mode_valid = ti_sn_bridge_mode_valid,
> -       .pre_enable = ti_sn_bridge_pre_enable,
> -       .enable = ti_sn_bridge_enable,
> -       .disable = ti_sn_bridge_disable,
> -       .post_disable = ti_sn_bridge_post_disable,
> +       .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> +       .atomic_enable = ti_sn_bridge_atomic_enable,
> +       .atomic_disable = ti_sn_bridge_atomic_disable,
> +       .atomic_post_disable = ti_sn_bridge_atomic_post_disable,
> +       .atomic_reset = drm_atomic_helper_bridge_reset,
> +       .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +       .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>  };
>
>  static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> --
> 2.34.1
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Applied to drm-misc-next.

Re: [PATCH v2 1/1] drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs

[Dave Stevenson]

Hi Sam

On Mon, 4 Jul 2022 at 17:18, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Move away from the deprecated enable/disable operations in
> drm_bridge_funcs and enable atomic use.

If you're moving to the atomic API, shouldn't you also be getting the
mode from the provided state, rather than
ti_sn_bridge_set_video_timings() having a rummage to find the crtc and
encoder state via &pdata->bridge.encoder->crtc->state->adjusted_mode?

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/ti-sn65dsi86.c#L904

  Dave

> v3:
>  - Drop use of DRM_BRIDGE_STATE_OPS
>
> v2:
>  - fix build (kernel test robot <lkp@intel.com>)
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c2b9227f7042..d6dd4d99a229 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -752,7 +752,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
>         return MODE_OK;
>  }
>
> -static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
> +                                       struct drm_bridge_state *old_bridge_state)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1011,7 +1012,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
>         return ret;
>  }
>
> -static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> +                                      struct drm_bridge_state *old_bridge_state)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>         const char *last_err_str = "No supported DP rate";
> @@ -1080,7 +1082,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>                            VSTREAM_ENABLE);
>  }
>
> -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> +                                          struct drm_bridge_state *old_bridge_state)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1093,7 +1096,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
>         usleep_range(100, 110);
>  }
>
> -static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> +                                            struct drm_bridge_state *old_bridge_state)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1114,10 +1118,13 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>         .attach = ti_sn_bridge_attach,
>         .detach = ti_sn_bridge_detach,
>         .mode_valid = ti_sn_bridge_mode_valid,
> -       .pre_enable = ti_sn_bridge_pre_enable,
> -       .enable = ti_sn_bridge_enable,
> -       .disable = ti_sn_bridge_disable,
> -       .post_disable = ti_sn_bridge_post_disable,
> +       .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> +       .atomic_enable = ti_sn_bridge_atomic_enable,
> +       .atomic_disable = ti_sn_bridge_atomic_disable,
> +       .atomic_post_disable = ti_sn_bridge_atomic_post_disable,
> +       .atomic_reset = drm_atomic_helper_bridge_reset,
> +       .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +       .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>  };
>
>  static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> --
> 2.34.1
>

Re: [PATCH v2 0/1] bridge/ti-sn65dsi86.c: convert to atomic operations

[Kieran Bingham]

(Dropping CC/lists)

Quoting Sam Ravnborg (2022-07-03 21:27:23)
> This patch is what is left on a previous attempt to make the
> ti-sn65dsi86 support DRM_BRIDGE_ATTACH_NO_CONNECTOR.
> 
> I ran into several challenges on the way:
> - I never found a way to retreive bpc when the driver
>   do not create the connector - and the driver needs it.
>   There may be ways to change how bpc is used but as I have no access
>   to HW it is not easy to change much.
> - My understanding of the driver is limited so I dropped trying to
>   do the proper implmentation of format negotiation
> - I had made a small macro to encapsulate a few of the common functions
>   in drm_bridge_funcs - but looking at it with fresh eyes told me
>   this is not a good idea.
> 
> I dropped previous a-b / r-b as the driver has changed a bit since
> last submission.
> Especially to Kieran - sorry for letting you wait so long for almost nothing.

No worries, thanks for digging in.

I'll get my branch back up and rebase on top of this and resume from
there. I can't remember all the details right now - so it will be
interesting to see how to handle the issues you've mentioned above - but
I think some of it is what I was trying to work on. (dealing with
getting the BPC at least..., so I don't know if I considered the issue
you reference about case of the driver not creating a connector).

I don't think I'll be able to resume my DRM work for more than a couple
of weeks now though so I'm going to be a bit longer. Hopefully your
patch here looks like it could be merged independently though.

--
Kieran


> 
> Sorry if this is a duplicate - something went wrong when I sent this
> mail the first time.
> 
>         Sam
> 
> 
> Sam Ravnborg (1):
>       drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
>

[PATCH v2 0/1] bridge/ti-sn65dsi86.c: convert to atomic operations

[Sam Ravnborg]

This patch is what is left on a previous attempt to make the
ti-sn65dsi86 support DRM_BRIDGE_ATTACH_NO_CONNECTOR.

I ran into several challenges on the way:
- I never found a way to retreive bpc when the driver
  do not create the connector - and the driver needs it.
  There may be ways to change how bpc is used but as I have no access
  to HW it is not easy to change much.
- My understanding of the driver is limited so I dropped trying to
  do the proper implmentation of format negotiation
- I had made a small macro to encapsulate a few of the common functions
  in drm_bridge_funcs - but looking at it with fresh eyes told me
  this is not a good idea.

I dropped previous a-b / r-b as the driver has changed a bit since
last submission.
Especially to Kieran - sorry for letting you wait so long for almost nothing.

Sorry if this is a duplicate - something went wrong when I sent this
mail the first time.

	Sam


Sam Ravnborg (1):
      drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)