[PATCH] drm/vc4: Fix HDMI mode validation

[PATCH] drm/vc4: Fix HDMI mode validation

[Nicolas Saenz Julienne]

Current mode validation impedes setting up some video modes which should
be supported otherwise. Namely 1920x1200@60Hz.

Fix this by lowering the minimum HDMI state machine clock to pixel clock
ratio allowed.

Fixes: 32e823c63e90 ("drm/vc4: Reject HDMI modes with too high of clocks")
Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index cea18dc15f77..340719238753 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -681,11 +681,23 @@ static enum drm_mode_status
 vc4_hdmi_encoder_mode_valid(struct drm_encoder *crtc,
 			    const struct drm_display_mode *mode)
 {
-	/* HSM clock must be 108% of the pixel clock.  Additionally,
-	 * the AXI clock needs to be at least 25% of pixel clock, but
-	 * HSM ends up being the limiting factor.
+	/*
+	 * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must
+	 * be faster than pixel clock, infinitesimally faster, tested in
+	 * simulation. Otherwise, exact value is unimportant for HDMI
+	 * operation." This conflicts with bcm2835's vc4 documentation, which
+	 * states HSM's clock has to be at least 108% of the pixel clock.
+	 *
+	 * Real life tests reveal that vc4's firmware statement holds up, and
+	 * users are able to use pixel clocks closer to HSM's, namely for
+	 * 1920x1200@60Hz. So it was decided to have leave a 1% margin between
+	 * both clocks. Which, for RPi0-3 implies a maximum pixel clock of
+	 * 162MHz.
+	 *
+	 * Additionally, the AXI clock needs to be at least 25% of
+	 * pixel clock, but HSM ends up being the limiting factor.
 	 */
-	if (mode->clock > HSM_CLOCK_FREQ / (1000 * 108 / 100))
+	if (mode->clock > HSM_CLOCK_FREQ / (1000 * 101 / 100))
 		return MODE_CLOCK_HIGH;
 
 	return MODE_OK;
-- 
2.25.1

Re: [PATCH] drm/vc4: Fix HDMI mode validation

[Maxime Ripard]

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

On Thu, Mar 26, 2020 at 01:20:01PM +0100, Nicolas Saenz Julienne wrote:
> Current mode validation impedes setting up some video modes which should
> be supported otherwise. Namely 1920x1200@60Hz.
>
> Fix this by lowering the minimum HDMI state machine clock to pixel clock
> ratio allowed.
>
> Fixes: 32e823c63e90 ("drm/vc4: Reject HDMI modes with too high of clocks")
> Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Maxime

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

Re: [PATCH] drm/vc4: Fix HDMI mode validation

[Stefan Wahren]

Am 26.03.20 um 13:20 schrieb Nicolas Saenz Julienne:
> Current mode validation impedes setting up some video modes which should
> be supported otherwise. Namely 1920x1200@60Hz.
>
> Fix this by lowering the minimum HDMI state machine clock to pixel clock
> ratio allowed.
>
> Fixes: 32e823c63e90 ("drm/vc4: Reject HDMI modes with too high of clocks")
> Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index cea18dc15f77..340719238753 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -681,11 +681,23 @@ static enum drm_mode_status
>  vc4_hdmi_encoder_mode_valid(struct drm_encoder *crtc,
>  			    const struct drm_display_mode *mode)
>  {
> -	/* HSM clock must be 108% of the pixel clock.  Additionally,
> -	 * the AXI clock needs to be at least 25% of pixel clock, but
> -	 * HSM ends up being the limiting factor.
> +	/*
> +	 * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must
> +	 * be faster than pixel clock, infinitesimally faster, tested in
> +	 * simulation. Otherwise, exact value is unimportant for HDMI
> +	 * operation." This conflicts with bcm2835's vc4 documentation, which
> +	 * states HSM's clock has to be at least 108% of the pixel clock.
> +	 *
> +	 * Real life tests reveal that vc4's firmware statement holds up, and
> +	 * users are able to use pixel clocks closer to HSM's, namely for
> +	 * 1920x1200@60Hz. So it was decided to have leave a 1% margin between
> +	 * both clocks. Which, for RPi0-3 implies a maximum pixel clock of

s/RPi0-3/bcm2835/ ?

Beside this nit:

Tested-by: Stefan Wahren <stefan.wahre@i2se.com>

Thanks

Re: [PATCH] drm/vc4: Fix HDMI mode validation

[Stefan Wahren]

Am 26.03.20 um 18:19 schrieb Stefan Wahren:
> Am 26.03.20 um 13:20 schrieb Nicolas Saenz Julienne:
>> Current mode validation impedes setting up some video modes which should
>> be supported otherwise. Namely 1920x1200@60Hz.
>>
>> Fix this by lowering the minimum HDMI state machine clock to pixel clock
>> ratio allowed.
>>
>> Fixes: 32e823c63e90 ("drm/vc4: Reject HDMI modes with too high of clocks")
>> Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
>> Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>> ---
>>  drivers/gpu/drm/vc4/vc4_hdmi.c | 20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> index cea18dc15f77..340719238753 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> @@ -681,11 +681,23 @@ static enum drm_mode_status
>>  vc4_hdmi_encoder_mode_valid(struct drm_encoder *crtc,
>>  			    const struct drm_display_mode *mode)
>>  {
>> -	/* HSM clock must be 108% of the pixel clock.  Additionally,
>> -	 * the AXI clock needs to be at least 25% of pixel clock, but
>> -	 * HSM ends up being the limiting factor.
>> +	/*
>> +	 * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must
>> +	 * be faster than pixel clock, infinitesimally faster, tested in
>> +	 * simulation. Otherwise, exact value is unimportant for HDMI
>> +	 * operation." This conflicts with bcm2835's vc4 documentation, which
>> +	 * states HSM's clock has to be at least 108% of the pixel clock.
>> +	 *
>> +	 * Real life tests reveal that vc4's firmware statement holds up, and
>> +	 * users are able to use pixel clocks closer to HSM's, namely for
>> +	 * 1920x1200@60Hz. So it was decided to have leave a 1% margin between
>> +	 * both clocks. Which, for RPi0-3 implies a maximum pixel clock of
> s/RPi0-3/bcm2835/ ?
>
> Beside this nit:
>
> Tested-by: Stefan Wahren <stefan.wahre@i2se.com>
>
> Thanks

Sorry typo in the mail address:

stefan.wahren@i2se.com

Re: [PATCH] drm/vc4: Fix HDMI mode validation

[Nicolas Saenz Julienne]

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

On Thu, 2020-03-26 at 18:19 +0100, Stefan Wahren wrote:
> Am 26.03.20 um 13:20 schrieb Nicolas Saenz Julienne:
> > Current mode validation impedes setting up some video modes which should
> > be supported otherwise. Namely 1920x1200@60Hz.
> > 
> > Fix this by lowering the minimum HDMI state machine clock to pixel clock
> > ratio allowed.
> > 
> > Fixes: 32e823c63e90 ("drm/vc4: Reject HDMI modes with too high of clocks")
> > Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> > Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> >  drivers/gpu/drm/vc4/vc4_hdmi.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > index cea18dc15f77..340719238753 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -681,11 +681,23 @@ static enum drm_mode_status
> >  vc4_hdmi_encoder_mode_valid(struct drm_encoder *crtc,
> >  			    const struct drm_display_mode *mode)
> >  {
> > -	/* HSM clock must be 108% of the pixel clock.  Additionally,
> > -	 * the AXI clock needs to be at least 25% of pixel clock, but
> > -	 * HSM ends up being the limiting factor.
> > +	/*
> > +	 * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must
> > +	 * be faster than pixel clock, infinitesimally faster, tested in
> > +	 * simulation. Otherwise, exact value is unimportant for HDMI
> > +	 * operation." This conflicts with bcm2835's vc4 documentation, which
> > +	 * states HSM's clock has to be at least 108% of the pixel clock.
> > +	 *
> > +	 * Real life tests reveal that vc4's firmware statement holds up, and
> > +	 * users are able to use pixel clocks closer to HSM's, namely for
> > +	 * 1920x1200@60Hz. So it was decided to have leave a 1% margin between
> > +	 * both clocks. Which, for RPi0-3 implies a maximum pixel clock of
> 
> s/RPi0-3/bcm2835/ ?

Well as Dave Stevenson stated on the previous thread[1], it's the firmware that
sets up the HSM limitation. IIUC technically both HSM and pixel clocks could be
increased. Hence this being more of a RPi limitation than the chip itself.

"Whilst the firmware would appear to use a fixed HSM clock on Pi0-3, I
don't anticipate there being any issue with varying it. It looks like
there was the expectation of it being variable in the past, but
someone has refactored it and either accidentally or deliberately
given up on the idea."

Regards,
Nicolas

[1] https://www.spinics.net/lists/arm-kernel/msg794591.html


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] drm/vc4: Fix HDMI mode validation

[Stefan Wahren]

Am 26.03.20 um 18:36 schrieb Nicolas Saenz Julienne:
> On Thu, 2020-03-26 at 18:19 +0100, Stefan Wahren wrote:
>> Am 26.03.20 um 13:20 schrieb Nicolas Saenz Julienne:
>>> Current mode validation impedes setting up some video modes which should
>>> be supported otherwise. Namely 1920x1200@60Hz.
>>>
>>> Fix this by lowering the minimum HDMI state machine clock to pixel clock
>>> ratio allowed.
>>>
>>> Fixes: 32e823c63e90 ("drm/vc4: Reject HDMI modes with too high of clocks")
>>> Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>> ---
>>>  drivers/gpu/drm/vc4/vc4_hdmi.c | 20 ++++++++++++++++----
>>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> index cea18dc15f77..340719238753 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> @@ -681,11 +681,23 @@ static enum drm_mode_status
>>>  vc4_hdmi_encoder_mode_valid(struct drm_encoder *crtc,
>>>  			    const struct drm_display_mode *mode)
>>>  {
>>> -	/* HSM clock must be 108% of the pixel clock.  Additionally,
>>> -	 * the AXI clock needs to be at least 25% of pixel clock, but
>>> -	 * HSM ends up being the limiting factor.
>>> +	/*
>>> +	 * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must
>>> +	 * be faster than pixel clock, infinitesimally faster, tested in
>>> +	 * simulation. Otherwise, exact value is unimportant for HDMI
>>> +	 * operation." This conflicts with bcm2835's vc4 documentation, which
>>> +	 * states HSM's clock has to be at least 108% of the pixel clock.
>>> +	 *
>>> +	 * Real life tests reveal that vc4's firmware statement holds up, and
>>> +	 * users are able to use pixel clocks closer to HSM's, namely for
>>> +	 * 1920x1200@60Hz. So it was decided to have leave a 1% margin between
>>> +	 * both clocks. Which, for RPi0-3 implies a maximum pixel clock of
>> s/RPi0-3/bcm2835/ ?
> Well as Dave Stevenson stated on the previous thread[1], it's the firmware that
> sets up the HSM limitation. IIUC technically both HSM and pixel clocks could be
> increased. Hence this being more of a RPi limitation than the chip itself.
>
> "Whilst the firmware would appear to use a fixed HSM clock on Pi0-3, I
> don't anticipate there being any issue with varying it. It looks like
> there was the expectation of it being variable in the past, but
> someone has refactored it and either accidentally or deliberately
> given up on the idea."

Okay thanks for the explanation again

So i'm fine this patch

>
> Regards,
> Nicolas
>
> [1] https://www.spinics.net/lists/arm-kernel/msg794591.html
>

Re: [PATCH] drm/vc4: Fix HDMI mode validation

[Nicolas Saenz Julienne]

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

Hi Daniel,

On Thu, 2020-03-26 at 13:20 +0100, Nicolas Saenz Julienne wrote:
> Current mode validation impedes setting up some video modes which should
> be supported otherwise. Namely 1920x1200@60Hz.
> 
> Fix this by lowering the minimum HDMI state machine clock to pixel clock
> ratio allowed.
> 
> Fixes: 32e823c63e90 ("drm/vc4: Reject HDMI modes with too high of clocks")
> Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Would it be possible for you to take this in for v5.7 (or as a fix for v5.6,
but it seems a little late)?

A device-tree patch I have to channel trough the SoC tree depends on this to
avoid regressions.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] drm/vc4: Fix HDMI mode validation

[Maxime Ripard]

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

On Fri, Mar 27, 2020 at 12:46:52PM +0100, Nicolas Saenz Julienne wrote:
> Hi Daniel,
>
> On Thu, 2020-03-26 at 13:20 +0100, Nicolas Saenz Julienne wrote:
> > Current mode validation impedes setting up some video modes which should
> > be supported otherwise. Namely 1920x1200@60Hz.
> >
> > Fix this by lowering the minimum HDMI state machine clock to pixel clock
> > ratio allowed.
> >
> > Fixes: 32e823c63e90 ("drm/vc4: Reject HDMI modes with too high of clocks")
> > Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> > Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>
> Would it be possible for you to take this in for v5.7 (or as a fix for v5.6,
> but it seems a little late)?
>
> A device-tree patch I have to channel trough the SoC tree depends on this to
> avoid regressions.

I've applied it for 5.7-rc1

Maxime

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

Re: [PATCH] drm/vc4: Fix HDMI mode validation

[Nicolas Saenz Julienne]

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

On Fri, 2020-03-27 at 13:40 +0100, Maxime Ripard wrote:
> On Fri, Mar 27, 2020 at 12:46:52PM +0100, Nicolas Saenz Julienne wrote:
> > Hi Daniel,
> > 
> > On Thu, 2020-03-26 at 13:20 +0100, Nicolas Saenz Julienne wrote:
> > > Current mode validation impedes setting up some video modes which should
> > > be supported otherwise. Namely 1920x1200@60Hz.
> > > 
> > > Fix this by lowering the minimum HDMI state machine clock to pixel clock
> > > ratio allowed.
> > > 
> > > Fixes: 32e823c63e90 ("drm/vc4: Reject HDMI modes with too high of clocks")
> > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > 
> > Would it be possible for you to take this in for v5.7 (or as a fix for v5.6,
> > but it seems a little late)?
> > 
> > A device-tree patch I have to channel trough the SoC tree depends on this to
> > avoid regressions.
> 
> I've applied it for 5.7-rc1

Thanks!


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]