[PATCH v5 0/3] SII902X HDMI Bridge fixups

[PATCH v5 0/3] SII902X HDMI Bridge fixups

[Jayesh Choudhary]

- Move the mode_valid hook to drm_bridge_funcs structure to take care
  of the case when the encoder attaches the bridge chain with the
  DRM_BRIDGE_ATTACH_NO_CONNECTOR flag in which case, the connector is
  not initialized in the bridge's attach call and mode_valid is not
  called.

- Also add this check to the atomic_check call as suggested by Maxime in
  v1 patch.

- Move from deprecated enable()/disable() hooks to atomic hooks as
  suggested in v4 by Sam

Testing has been done on AM62X platform which have SII902X HDMI bridge.
From the logs we can see the propagated modes and flip test for 1080p
resolution.

Testlog:
<https://gist.github.com/Jayesh2000/9bea6840672869337039296bc1145df4>

Changelog v4->v5:
- Add followup patch to replace deprecated bridge hooks
- Fix atomic check to return negative value
- Made commit message for [1/2] in v4 more clear while keeping the R-by and
  Acked-by tags.
- Fix commit header for [2/2] in v4

v4 patch:
<https://lore.kernel.org/all/20240530092930.434026-1-j-choudhary@ti.com/>

Changelog v3->v4:
- Remove mode_valid hook from connector_helper_funcs as it is not needed.

v3 patch:
<https://lore.kernel.org/all/20240524093509.127189-1-j-choudhary@ti.com/>

Changelog v2->v3:
- Remove newline that was introduced in [1/2] and later deleted in [2/2]
  in v2

v2 patch:
<https://lore.kernel.org/all/20240524073305.107293-1-j-choudhary@ti.com/>

Changelog v1->v2:
- Add KHZ suffix in the macros to be more clear
- Add the hook for drm_bridge_funcs as well
- Add check in atomic_check dunction call (in a separate patch)

v1 patch:
<https://lore.kernel.org/all/20240408081435.216927-1-j-choudhary@ti.com/>

Jayesh Choudhary (3):
  drm/bridge: sii902x: Fix mode_valid hook
  drm/bridge: sii902x: Support atomic bridge APIs
  drm/bridge: sii902x: Add pixel clock check in atomic_check

 drivers/gpu/drm/bridge/sii902x.c | 46 +++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH v5 1/3] drm/bridge: sii902x: Fix mode_valid hook

[Jayesh Choudhary]

Currently, mode_valid is defined only in drm_connector_helper_funcs.
When the bridge is attached with the 'DRM_BRIDGE_ATTACH_NO_CONNECTOR'
flag, the connector is not initialized, and so is the mode_valid
hook under connector helper funcs.
It also returns MODE_OK for all modes without actually checking the
modes.
So move the mode_valid hook to drm_bridge_funcs with proper clock
checks for maximum and minimum pixel clock supported by the bridge.

Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/bridge/sii902x.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 2fbeda9025bf..6a6055a4ccf9 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -163,6 +163,14 @@
 
 #define SII902X_AUDIO_PORT_INDEX		3
 
+/*
+ * The maximum resolution supported by the HDMI bridge is 1080p@60Hz
+ * and 1920x1200 requiring a pixel clock of 165MHz and the minimum
+ * resolution supported is 480p@60Hz requiring a pixel clock of 25MHz
+ */
+#define SII902X_MIN_PIXEL_CLOCK_KHZ		25000
+#define SII902X_MAX_PIXEL_CLOCK_KHZ		165000
+
 struct sii902x {
 	struct i2c_client *i2c;
 	struct regmap *regmap;
@@ -310,17 +318,8 @@ static int sii902x_get_modes(struct drm_connector *connector)
 	return num;
 }
 
-static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector,
-					       struct drm_display_mode *mode)
-{
-	/* TODO: check mode */
-
-	return MODE_OK;
-}
-
 static const struct drm_connector_helper_funcs sii902x_connector_helper_funcs = {
 	.get_modes = sii902x_get_modes,
-	.mode_valid = sii902x_mode_valid,
 };
 
 static void sii902x_bridge_disable(struct drm_bridge *bridge)
@@ -504,6 +503,20 @@ static int sii902x_bridge_atomic_check(struct drm_bridge *bridge,
 	return 0;
 }
 
+static enum drm_mode_status
+sii902x_bridge_mode_valid(struct drm_bridge *bridge,
+			  const struct drm_display_info *info,
+			  const struct drm_display_mode *mode)
+{
+	if (mode->clock < SII902X_MIN_PIXEL_CLOCK_KHZ)
+		return MODE_CLOCK_LOW;
+
+	if (mode->clock > SII902X_MAX_PIXEL_CLOCK_KHZ)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
 static const struct drm_bridge_funcs sii902x_bridge_funcs = {
 	.attach = sii902x_bridge_attach,
 	.mode_set = sii902x_bridge_mode_set,
@@ -516,6 +529,7 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_get_input_bus_fmts = sii902x_bridge_atomic_get_input_bus_fmts,
 	.atomic_check = sii902x_bridge_atomic_check,
+	.mode_valid = sii902x_bridge_mode_valid,
 };
 
 static int sii902x_mute(struct sii902x *sii902x, bool mute)
-- 
2.25.1

Re: [PATCH v5 1/3] drm/bridge: sii902x: Fix mode_valid hook

[Aradhya Bhatia]

Hi Jayesh,

Thanks for the patches!

On 13-Jun-24 14:08, Jayesh Choudhary wrote:
> Currently, mode_valid is defined only in drm_connector_helper_funcs.
> When the bridge is attached with the 'DRM_BRIDGE_ATTACH_NO_CONNECTOR'
> flag, the connector is not initialized, and so is the mode_valid
> hook under connector helper funcs.
> It also returns MODE_OK for all modes without actually checking the
> modes.
> So move the mode_valid hook to drm_bridge_funcs with proper clock
> checks for maximum and minimum pixel clock supported by the bridge.
> 
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/bridge/sii902x.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 2fbeda9025bf..6a6055a4ccf9 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -163,6 +163,14 @@
>  
>  #define SII902X_AUDIO_PORT_INDEX		3
>  
> +/*
> + * The maximum resolution supported by the HDMI bridge is 1080p@60Hz
> + * and 1920x1200 requiring a pixel clock of 165MHz and the minimum
> + * resolution supported is 480p@60Hz requiring a pixel clock of 25MHz
> + */
> +#define SII902X_MIN_PIXEL_CLOCK_KHZ		25000
> +#define SII902X_MAX_PIXEL_CLOCK_KHZ		165000
> +
>  struct sii902x {
>  	struct i2c_client *i2c;
>  	struct regmap *regmap;
> @@ -310,17 +318,8 @@ static int sii902x_get_modes(struct drm_connector *connector)
>  	return num;
>  }
>  
> -static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector,
> -					       struct drm_display_mode *mode)
> -{
> -	/* TODO: check mode */
> -
> -	return MODE_OK;
> -}
> -
>  static const struct drm_connector_helper_funcs sii902x_connector_helper_funcs = {
>  	.get_modes = sii902x_get_modes,
> -	.mode_valid = sii902x_mode_valid,
>  };
>  
>  static void sii902x_bridge_disable(struct drm_bridge *bridge)
> @@ -504,6 +503,20 @@ static int sii902x_bridge_atomic_check(struct drm_bridge *bridge,
>  	return 0;
>  }
>  
> +static enum drm_mode_status
> +sii902x_bridge_mode_valid(struct drm_bridge *bridge,
> +			  const struct drm_display_info *info,
> +			  const struct drm_display_mode *mode)
> +{
> +	if (mode->clock < SII902X_MIN_PIXEL_CLOCK_KHZ)
> +		return MODE_CLOCK_LOW;
> +
> +	if (mode->clock > SII902X_MAX_PIXEL_CLOCK_KHZ)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +
>  static const struct drm_bridge_funcs sii902x_bridge_funcs = {
>  	.attach = sii902x_bridge_attach,
>  	.mode_set = sii902x_bridge_mode_set,
> @@ -516,6 +529,7 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = {
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>  	.atomic_get_input_bus_fmts = sii902x_bridge_atomic_get_input_bus_fmts,
>  	.atomic_check = sii902x_bridge_atomic_check,
> +	.mode_valid = sii902x_bridge_mode_valid,
>  };
>  
>  static int sii902x_mute(struct sii902x *sii902x, bool mute)

Reviewed-by: Aradhya Bhatia <a-bhatia1@ti.com>

-- 
Regards
Aradhya

[PATCH v5 2/3] drm/bridge: sii902x: Support atomic bridge APIs

[Jayesh Choudhary]

Change exisitig enable() and disable() bridge hooks to their atomic
counterparts as the former hooks are deprecated.

Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
 drivers/gpu/drm/bridge/sii902x.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 6a6055a4ccf9..00a8c469f553 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -322,7 +322,8 @@ static const struct drm_connector_helper_funcs sii902x_connector_helper_funcs =
 	.get_modes = sii902x_get_modes,
 };
 
-static void sii902x_bridge_disable(struct drm_bridge *bridge)
+static void sii902x_bridge_atomic_disable(struct drm_bridge *bridge,
+					  struct drm_bridge_state *old_bridge_state)
 {
 	struct sii902x *sii902x = bridge_to_sii902x(bridge);
 
@@ -335,7 +336,8 @@ static void sii902x_bridge_disable(struct drm_bridge *bridge)
 	mutex_unlock(&sii902x->mutex);
 }
 
-static void sii902x_bridge_enable(struct drm_bridge *bridge)
+static void sii902x_bridge_atomic_enable(struct drm_bridge *bridge,
+					 struct drm_bridge_state *old_bridge_state)
 {
 	struct sii902x *sii902x = bridge_to_sii902x(bridge);
 
@@ -520,8 +522,8 @@ sii902x_bridge_mode_valid(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs sii902x_bridge_funcs = {
 	.attach = sii902x_bridge_attach,
 	.mode_set = sii902x_bridge_mode_set,
-	.disable = sii902x_bridge_disable,
-	.enable = sii902x_bridge_enable,
+	.atomic_disable = sii902x_bridge_atomic_disable,
+	.atomic_enable = sii902x_bridge_atomic_enable,
 	.detect = sii902x_bridge_detect,
 	.edid_read = sii902x_bridge_edid_read,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
-- 
2.25.1

Re: [PATCH v5 2/3] drm/bridge: sii902x: Support atomic bridge APIs

[Aradhya Bhatia]

On 13-Jun-24 14:08, Jayesh Choudhary wrote:
> Change exisitig enable() and disable() bridge hooks to their atomic
> counterparts as the former hooks are deprecated.
> 
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
>  drivers/gpu/drm/bridge/sii902x.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 6a6055a4ccf9..00a8c469f553 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -322,7 +322,8 @@ static const struct drm_connector_helper_funcs sii902x_connector_helper_funcs =
>  	.get_modes = sii902x_get_modes,
>  };
>  
> -static void sii902x_bridge_disable(struct drm_bridge *bridge)
> +static void sii902x_bridge_atomic_disable(struct drm_bridge *bridge,
> +					  struct drm_bridge_state *old_bridge_state)
>  {
>  	struct sii902x *sii902x = bridge_to_sii902x(bridge);
>  
> @@ -335,7 +336,8 @@ static void sii902x_bridge_disable(struct drm_bridge *bridge)
>  	mutex_unlock(&sii902x->mutex);
>  }
>  
> -static void sii902x_bridge_enable(struct drm_bridge *bridge)
> +static void sii902x_bridge_atomic_enable(struct drm_bridge *bridge,
> +					 struct drm_bridge_state *old_bridge_state)
>  {
>  	struct sii902x *sii902x = bridge_to_sii902x(bridge);
>  
> @@ -520,8 +522,8 @@ sii902x_bridge_mode_valid(struct drm_bridge *bridge,
>  static const struct drm_bridge_funcs sii902x_bridge_funcs = {
>  	.attach = sii902x_bridge_attach,
>  	.mode_set = sii902x_bridge_mode_set,
> -	.disable = sii902x_bridge_disable,
> -	.enable = sii902x_bridge_enable,
> +	.atomic_disable = sii902x_bridge_atomic_disable,
> +	.atomic_enable = sii902x_bridge_atomic_enable,
>  	.detect = sii902x_bridge_detect,
>  	.edid_read = sii902x_bridge_edid_read,
>  	.atomic_reset = drm_atomic_helper_bridge_reset,

Reviewed-by: Aradhya Bhatia <a-bhatia1@ti.com>

-- 
Regards
Aradhya

Re: [PATCH v5 2/3] drm/bridge: sii902x: Support atomic bridge APIs

[Dmitry Baryshkov]

On Thu, Jun 13, 2024 at 02:08:04PM +0530, Jayesh Choudhary wrote:
> Change exisitig enable() and disable() bridge hooks to their atomic
> counterparts as the former hooks are deprecated.
> 
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
>  drivers/gpu/drm/bridge/sii902x.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 

Suggested-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH v5 3/3] drm/bridge: sii902x: Add pixel clock check in atomic_check

[Jayesh Choudhary]

Check the pixel clock for the mode in atomic_check and ensure that
it is within the range supported by the bridge.

Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
 drivers/gpu/drm/bridge/sii902x.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 00a8c469f553..7f91b0db161e 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -496,6 +496,10 @@ static int sii902x_bridge_atomic_check(struct drm_bridge *bridge,
 				       struct drm_crtc_state *crtc_state,
 				       struct drm_connector_state *conn_state)
 {
+	if (crtc_state->mode.clock < SII902X_MIN_PIXEL_CLOCK_KHZ ||
+	    crtc_state->mode.clock > SII902X_MAX_PIXEL_CLOCK_KHZ)
+		return -EINVAL;
+
 	/*
 	 * There might be flags negotiation supported in future but
 	 * set the bus flags in atomic_check statically for now.
-- 
2.25.1

Re: [PATCH v5 3/3] drm/bridge: sii902x: Add pixel clock check in atomic_check

[Aradhya Bhatia]

On 13-Jun-24 14:08, Jayesh Choudhary wrote:
> Check the pixel clock for the mode in atomic_check and ensure that
> it is within the range supported by the bridge.
> 
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
>  drivers/gpu/drm/bridge/sii902x.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 00a8c469f553..7f91b0db161e 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -496,6 +496,10 @@ static int sii902x_bridge_atomic_check(struct drm_bridge *bridge,
>  				       struct drm_crtc_state *crtc_state,
>  				       struct drm_connector_state *conn_state)
>  {
> +	if (crtc_state->mode.clock < SII902X_MIN_PIXEL_CLOCK_KHZ ||
> +	    crtc_state->mode.clock > SII902X_MAX_PIXEL_CLOCK_KHZ)
> +		return -EINVAL;
> +
>  	/*
>  	 * There might be flags negotiation supported in future but
>  	 * set the bus flags in atomic_check statically for now.

Reviewed-by: Aradhya Bhatia <a-bhatia1@ti.com>

-- 
Regards
Aradhya

Re: [PATCH v5 3/3] drm/bridge: sii902x: Add pixel clock check in atomic_check

[Dmitry Baryshkov]

On Thu, Jun 13, 2024 at 02:08:05PM +0530, Jayesh Choudhary wrote:
> Check the pixel clock for the mode in atomic_check and ensure that
> it is within the range supported by the bridge.
> 
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
>  drivers/gpu/drm/bridge/sii902x.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

Re: [PATCH v5 0/3] SII902X HDMI Bridge fixups

[Maxime Ripard]

On Thu, 13 Jun 2024 14:08:02 +0530, Jayesh Choudhary wrote:
> - Move the mode_valid hook to drm_bridge_funcs structure to take care
>   of the case when the encoder attaches the bridge chain with the
>   DRM_BRIDGE_ATTACH_NO_CONNECTOR flag in which case, the connector is
>   not initialized in the bridge's attach call and mode_valid is not
>   called.
> 
> - Also add this check to the atomic_check call as suggested by Maxime in
>   v1 patch.
> 
> [...]

Applied to misc/kernel.git (drm-misc-next).

Thanks!
Maxime