diff mbox series

[v7,10/12] drm/bridge: cdns-dsi: Move DSI mode check to _atomic_check()

Message ID 20250114055626.18816-11-aradhya.bhatia@linux.dev (mailing list archive)
State New
Headers show
Series drm/bridge: cdns-dsi: Fix the color-shift issue | expand

Commit Message

Aradhya Bhatia Jan. 14, 2025, 5:56 a.m. UTC 
From: Aradhya Bhatia <a-bhatia1@ti.com>

At present, the DSI mode configuration check happens during the
_atomic_enable() phase, which is not really the best place for this.
Moreover, if the mode is not valid, the driver gives a warning and
continues the hardware configuration.

Move the DSI mode configuration check to _atomic_check() instead, which
can properly report back any invalid mode, before the _enable phase even
begins.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 87 +++++++++++++++++--
 1 file changed, 82 insertions(+), 5 deletions(-)

Comments

Dmitry Baryshkov Jan. 14, 2025, 11:15 a.m. UTC | #1 
On Tue, Jan 14, 2025 at 11:26:24AM +0530, Aradhya Bhatia wrote:
> From: Aradhya Bhatia <a-bhatia1@ti.com>
> 
> At present, the DSI mode configuration check happens during the
> _atomic_enable() phase, which is not really the best place for this.
> Moreover, if the mode is not valid, the driver gives a warning and
> continues the hardware configuration.
> 
> Move the DSI mode configuration check to _atomic_check() instead, which
> can properly report back any invalid mode, before the _enable phase even
> begins.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> ---
>  .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 87 +++++++++++++++++--
>  1 file changed, 82 insertions(+), 5 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tomi Valkeinen Jan. 14, 2025, 12:47 p.m. UTC | #2 
Hi,

On 14/01/2025 07:56, Aradhya Bhatia wrote:
> From: Aradhya Bhatia <a-bhatia1@ti.com>
> 
> At present, the DSI mode configuration check happens during the
> _atomic_enable() phase, which is not really the best place for this.
> Moreover, if the mode is not valid, the driver gives a warning and
> continues the hardware configuration.
> 
> Move the DSI mode configuration check to _atomic_check() instead, which
> can properly report back any invalid mode, before the _enable phase even
> begins.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> ---
>   .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 87 +++++++++++++++++--
>   1 file changed, 82 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index b8db984ea9fa..d60254e1270c 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -425,6 +425,17 @@
>   #define DSI_NULL_FRAME_OVERHEAD		6
>   #define DSI_EOT_PKT_SIZE		4
>   
> +struct cdns_dsi_bridge_state {
> +	struct drm_bridge_state base;
> +	struct cdns_dsi_cfg dsi_cfg;
> +};
> +
> +static inline struct cdns_dsi_bridge_state *
> +to_cdns_dsi_bridge_state(struct drm_bridge_state *bridge_state)
> +{
> +	return container_of(bridge_state, struct cdns_dsi_bridge_state, base);
> +}
> +
>   static inline struct cdns_dsi *input_to_dsi(struct cdns_dsi_input *input)
>   {
>   	return container_of(input, struct cdns_dsi, input);
> @@ -768,6 +779,9 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
>   	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>   	struct cdns_dsi *dsi = input_to_dsi(input);
>   	struct cdns_dsi_output *output = &dsi->output;
> +	struct drm_atomic_state *state = old_bridge_state->base.state;
> +	struct cdns_dsi_bridge_state *dsi_state;
> +	struct drm_bridge_state *new_bridge_state;
>   	struct drm_display_mode *mode;
>   	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
>   	unsigned long tx_byte_period;
> @@ -778,14 +792,19 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
>   	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
>   		return;
>   
> +	new_bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
> +	if (WARN_ON(!new_bridge_state))
> +		return;
> +
> +	dsi_state = to_cdns_dsi_bridge_state(new_bridge_state);
> +	dsi_cfg = dsi_state->dsi_cfg;
> +
>   	if (dsi->platform_ops && dsi->platform_ops->enable)
>   		dsi->platform_ops->enable(dsi);
>   
>   	mode = &bridge->encoder->crtc->state->adjusted_mode;
>   	nlanes = output->dev->lanes;
>   
> -	WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, &dsi_cfg, false));
> -
>   	cdns_dsi_init_link(dsi);
>   	cdns_dsi_hs_init(dsi);
>   
> @@ -956,6 +975,63 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
>   	return input_fmts;
>   }
>   
> +static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
> +					struct drm_bridge_state *bridge_state,
> +					struct drm_crtc_state *crtc_state,
> +					struct drm_connector_state *conn_state)
> +{
> +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> +	struct cdns_dsi *dsi = input_to_dsi(input);
> +	struct cdns_dsi_bridge_state *dsi_state = to_cdns_dsi_bridge_state(bridge_state);
> +	struct drm_display_mode *mode = &crtc_state->mode;

const

> +	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
> +
> +	return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
> +}
> +
> +static struct drm_bridge_state *
> +cdns_dsi_bridge_atomic_duplicate_state(struct drm_bridge *bridge)
> +{
> +	struct cdns_dsi_bridge_state *dsi_state;
> +
> +	if (WARN_ON(!bridge->base.state))
> +		return NULL;
> +
> +	dsi_state = kzalloc(sizeof(*dsi_state), GFP_KERNEL);
> +	if (!dsi_state)
> +		return NULL;
> +
> +	__drm_atomic_helper_bridge_duplicate_state(bridge, &dsi_state->base);

Hmm, if I'm not mistaken, you should copy the private state here 
(cdns_dsi_cfg).

> +	return &dsi_state->base;
> +}
> +
> +static void
> +cdns_dsi_bridge_atomic_destroy_state(struct drm_bridge *bridge,
> +				     struct drm_bridge_state *state)
> +{
> +	struct cdns_dsi_bridge_state *dsi_state;
> +
> +	dsi_state = to_cdns_dsi_bridge_state(state);
> +
> +	kfree(dsi_state);
> +}
> +
> +static struct drm_bridge_state *
> +cdns_dsi_bridge_atomic_reset(struct drm_bridge *bridge)
> +{
> +	struct cdns_dsi_bridge_state *dsi_state;
> +
> +	dsi_state = kzalloc(sizeof(*dsi_state), GFP_KERNEL);
> +	if (!dsi_state)
> +		return NULL;
> +
> +	memset(dsi_state, 0, sizeof(*dsi_state));
> +	dsi_state->base.bridge = bridge;
> +
> +	return &dsi_state->base;
> +}
> +
>   static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
>   	.attach = cdns_dsi_bridge_attach,
>   	.mode_valid = cdns_dsi_bridge_mode_valid,
> @@ -963,9 +1039,10 @@ static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
>   	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
>   	.atomic_enable = cdns_dsi_bridge_atomic_enable,
>   	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
> -	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> -	.atomic_reset = drm_atomic_helper_bridge_reset,
> +	.atomic_check = cdns_dsi_bridge_atomic_check,
> +	.atomic_duplicate_state = cdns_dsi_bridge_atomic_duplicate_state,
> +	.atomic_destroy_state = cdns_dsi_bridge_atomic_destroy_state,
> +	.atomic_reset = cdns_dsi_bridge_atomic_reset,
>   	.atomic_get_input_bus_fmts = cdns_dsi_bridge_get_input_bus_fmts,
>   };
>   

Other than the two small comments:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index b8db984ea9fa..d60254e1270c 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -425,6 +425,17 @@ 
 #define DSI_NULL_FRAME_OVERHEAD		6
 #define DSI_EOT_PKT_SIZE		4
 
+struct cdns_dsi_bridge_state {
+	struct drm_bridge_state base;
+	struct cdns_dsi_cfg dsi_cfg;
+};
+
+static inline struct cdns_dsi_bridge_state *
+to_cdns_dsi_bridge_state(struct drm_bridge_state *bridge_state)
+{
+	return container_of(bridge_state, struct cdns_dsi_bridge_state, base);
+}
+
 static inline struct cdns_dsi *input_to_dsi(struct cdns_dsi_input *input)
 {
 	return container_of(input, struct cdns_dsi, input);
@@ -768,6 +779,9 @@  static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 	struct cdns_dsi_output *output = &dsi->output;
+	struct drm_atomic_state *state = old_bridge_state->base.state;
+	struct cdns_dsi_bridge_state *dsi_state;
+	struct drm_bridge_state *new_bridge_state;
 	struct drm_display_mode *mode;
 	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
 	unsigned long tx_byte_period;
@@ -778,14 +792,19 @@  static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
 		return;
 
+	new_bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
+	if (WARN_ON(!new_bridge_state))
+		return;
+
+	dsi_state = to_cdns_dsi_bridge_state(new_bridge_state);
+	dsi_cfg = dsi_state->dsi_cfg;
+
 	if (dsi->platform_ops && dsi->platform_ops->enable)
 		dsi->platform_ops->enable(dsi);
 
 	mode = &bridge->encoder->crtc->state->adjusted_mode;
 	nlanes = output->dev->lanes;
 
-	WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, &dsi_cfg, false));
-
 	cdns_dsi_init_link(dsi);
 	cdns_dsi_hs_init(dsi);
 
@@ -956,6 +975,63 @@  static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 	return input_fmts;
 }
 
+static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
+					struct drm_bridge_state *bridge_state,
+					struct drm_crtc_state *crtc_state,
+					struct drm_connector_state *conn_state)
+{
+	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+	struct cdns_dsi *dsi = input_to_dsi(input);
+	struct cdns_dsi_bridge_state *dsi_state = to_cdns_dsi_bridge_state(bridge_state);
+	struct drm_display_mode *mode = &crtc_state->mode;
+	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
+
+	return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
+}
+
+static struct drm_bridge_state *
+cdns_dsi_bridge_atomic_duplicate_state(struct drm_bridge *bridge)
+{
+	struct cdns_dsi_bridge_state *dsi_state;
+
+	if (WARN_ON(!bridge->base.state))
+		return NULL;
+
+	dsi_state = kzalloc(sizeof(*dsi_state), GFP_KERNEL);
+	if (!dsi_state)
+		return NULL;
+
+	__drm_atomic_helper_bridge_duplicate_state(bridge, &dsi_state->base);
+
+	return &dsi_state->base;
+}
+
+static void
+cdns_dsi_bridge_atomic_destroy_state(struct drm_bridge *bridge,
+				     struct drm_bridge_state *state)
+{
+	struct cdns_dsi_bridge_state *dsi_state;
+
+	dsi_state = to_cdns_dsi_bridge_state(state);
+
+	kfree(dsi_state);
+}
+
+static struct drm_bridge_state *
+cdns_dsi_bridge_atomic_reset(struct drm_bridge *bridge)
+{
+	struct cdns_dsi_bridge_state *dsi_state;
+
+	dsi_state = kzalloc(sizeof(*dsi_state), GFP_KERNEL);
+	if (!dsi_state)
+		return NULL;
+
+	memset(dsi_state, 0, sizeof(*dsi_state));
+	dsi_state->base.bridge = bridge;
+
+	return &dsi_state->base;
+}
+
 static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.attach = cdns_dsi_bridge_attach,
 	.mode_valid = cdns_dsi_bridge_mode_valid,
@@ -963,9 +1039,10 @@  static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
 	.atomic_enable = cdns_dsi_bridge_atomic_enable,
 	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
-	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
-	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.atomic_check = cdns_dsi_bridge_atomic_check,
+	.atomic_duplicate_state = cdns_dsi_bridge_atomic_duplicate_state,
+	.atomic_destroy_state = cdns_dsi_bridge_atomic_destroy_state,
+	.atomic_reset = cdns_dsi_bridge_atomic_reset,
 	.atomic_get_input_bus_fmts = cdns_dsi_bridge_get_input_bus_fmts,
 };