[PATCH v4 0/2] Add mode_valid and atomic_check hooks for sii902x bridge

[PATCH v4 0/2] Add mode_valid and atomic_check hooks for sii902x bridge

[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.

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 (2):
  drm/bridge: sii902x: Fix mode_valid hook
  drm/bridge: Add pixel clock check in atomic_check

 drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH v4 1/2] drm/bridge: sii902x: Fix mode_valid hook

[Jayesh Choudhary]

Currently, mode_valid hook returns all mode as valid and it is
defined only in drm_connector_helper_funcs. With the introduction of
'DRM_BRIDGE_ATTACH_NO_CONNECTOR', connector is not initialized in
bridge_attach call for cases when the encoder has this flag enabled.
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>
---
 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 v4 1/2] drm/bridge: sii902x: Fix mode_valid hook

[Dmitry Baryshkov]

On Thu, May 30, 2024 at 02:59:29PM +0530, Jayesh Choudhary wrote:
> Currently, mode_valid hook returns all mode as valid and it is
> defined only in drm_connector_helper_funcs. With the introduction of
> 'DRM_BRIDGE_ATTACH_NO_CONNECTOR', connector is not initialized in
> bridge_attach call for cases when the encoder has this flag enabled.
> 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>
> ---
>  drivers/gpu/drm/bridge/sii902x.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 

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

Chris, you might be interested in testing this series.

-- 
With best wishes
Dmitry

Re: [v4,1/2] drm/bridge: sii902x: Fix mode_valid hook

[Sui Jingfeng]

Hi,


On 5/30/24 17:29, Jayesh Choudhary wrote:
> Currently, mode_valid hook returns all mode as valid and it is
> defined only in drm_connector_helper_funcs. With the introduction of
> 'DRM_BRIDGE_ATTACH_NO_CONNECTOR', connector is not initialized in
> bridge_attach call for cases when the encoder has this flag enabled.
> 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>


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
> +

This bridge can drive 2560x1080@75Hz monitor(LG 34BL650), the pixel
clock can up to 181250 kHz. I remember that I have tested the native
mode with LS2K1000 SoC, and it do works in practice. And there are
also has 320x240 panels, maybe it's also usuable with this HDMI
transmitter

Well, the datasheet mentioned that it supports up to 165 MHz
dual-edge and single-edge modes. So I'm not against your patch,
just mention it to let you know.


>   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)

-- 
Best regards
Sui

Re: [v4,1/2] drm/bridge: sii902x: Fix mode_valid hook

[Sam Ravnborg]

Hi Jayesh,

> > +
> >   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,

As you have the possibility to test the driver, it would be nice with a
follow-up patch that replaces the use of enable() / disable() with the
atomic counterparts.

enable() / disable() are deprecated, so it is nice to reduce their use.

	Sam

Re: [v4,1/2] drm/bridge: sii902x: Fix mode_valid hook

[Sui Jingfeng]

Hi, Jayesh


On 5/31/24 21:33, Sam Ravnborg wrote:
> Hi Jayesh,
> 
>>> +
>>>    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,
> 
> As you have the possibility to test the driver, it would be nice with a
> follow-up patch that replaces the use of enable() / disable() with the
> atomic counterparts.
> 
> enable() / disable() are deprecated, so it is nice to reduce their use.

I agree with Sam.

Please using atomic uniformally with a follow-up patch, the mixed
using of atomic API and non atomic API is a little bit confusing IMO.


> 	Sam

Re: [v4,1/2] drm/bridge: sii902x: Fix mode_valid hook

[Jayesh Choudhary]

Hello Sui, Sam!

Thanks for the review.

(Sorry for delayed response. I was OoO last week)

On 31/05/24 19:34, Sui Jingfeng wrote:
> Hi, Jayesh
> 
> 
> On 5/31/24 21:33, Sam Ravnborg wrote:
>> Hi Jayesh,
>>
>>>> +
>>>>    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,
>>
>> As you have the possibility to test the driver, it would be nice with a
>> follow-up patch that replaces the use of enable() / disable() with the
>> atomic counterparts.
>>
>> enable() / disable() are deprecated, so it is nice to reduce their use.
> 
> I agree with Sam.
> 
> Please using atomic uniformally with a follow-up patch, the mixed
> using of atomic API and non atomic API is a little bit confusing IMO.
> 
> 

I will change the enable and disable to their atomic counter parts in 
the next revision.

Warm Regards,
-Jayesh

[PATCH v4 2/2] drm/bridge: 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 6a6055a4ccf9..1bf2f14a772d 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -494,6 +494,12 @@ 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)
+		return MODE_CLOCK_LOW;
+
+	if (crtc_state->mode.clock > SII902X_MAX_PIXEL_CLOCK_KHZ)
+		return MODE_CLOCK_HIGH;
+
 	/*
 	 * There might be flags negotiation supported in future but
 	 * set the bus flags in atomic_check statically for now.
-- 
2.25.1

Re: [PATCH v4 2/2] drm/bridge: Add pixel clock check in atomic_check

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

Hi,

On Thu, May 30, 2024 at 02:59:30PM GMT, 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 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 6a6055a4ccf9..1bf2f14a772d 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -494,6 +494,12 @@ 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)
> +		return MODE_CLOCK_LOW;
> +
> +	if (crtc_state->mode.clock > SII902X_MAX_PIXEL_CLOCK_KHZ)
> +		return MODE_CLOCK_HIGH;
> +

atomic_check doesn't return drm_mode_status but regular error codes (0
on success, negative error code on failure)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/2] drm/bridge: Add pixel clock check in atomic_check

[Jayesh Choudhary]

Hello Maxime,

On 30/05/24 15:04, Maxime Ripard wrote:
> Hi,
> 
> On Thu, May 30, 2024 at 02:59:30PM GMT, 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 | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
>> index 6a6055a4ccf9..1bf2f14a772d 100644
>> --- a/drivers/gpu/drm/bridge/sii902x.c
>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>> @@ -494,6 +494,12 @@ 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)
>> +		return MODE_CLOCK_LOW;
>> +
>> +	if (crtc_state->mode.clock > SII902X_MAX_PIXEL_CLOCK_KHZ)
>> +		return MODE_CLOCK_HIGH;
>> +
> 
> atomic_check doesn't return drm_mode_status but regular error codes (0
> on success, negative error code on failure)

Okay.

Will club together both conditions and return -EINVAL.

Warm Regards,
Jayesh

> 
> Maxime

Re: [PATCH v4 0/2] Add mode_valid and atomic_check hooks for sii902x bridge

[Sui Jingfeng]

Hi,

On 5/30/24 17:29, 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.

Good catch, as modes being supported is actually a capability of the
bridge itself. The move make the driver reflect the hardware more.