[07/11] drm/rockchip: inno_hdmi: Add basic mode validation

  As per TRM this controller supports pixelclocks starting from 25 MHz. The
maximum supported pixelclocks are defined by the phy configurations we
have. Also it can't support modes that require doubled clocks.
If there is a phy reference clock we can additionally validate against
VESA DMT's recommendations.
Those checks are added to the mode_valid hook of the connector and
encoder's mode_fixup hook.

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)
  

Hi,

On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
> As per TRM this controller supports pixelclocks starting from 25 MHz. The
> maximum supported pixelclocks are defined by the phy configurations we
> have. Also it can't support modes that require doubled clocks.
> If there is a phy reference clock we can additionally validate against
> VESA DMT's recommendations.
> Those checks are added to the mode_valid hook of the connector and
> encoder's mode_fixup hook.
> 
> Signed-off-by: Alex Bee <knaerzche@gmail.com>
> ---
>  drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index f7f0bec725f9..2f839ff31c1c 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
>  	struct inno_hdmi_phy_config *default_phy_config;
>  };
>  
> +#define INNO_HDMI_MIN_TMDS_CLOCK  25000000U
> +
>  struct hdmi_data_info {
>  	int vic;
>  	bool sink_has_audio;
> @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>  	return 0;
>  }
>  
> +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
> +						 struct drm_display_mode *mode)
> +{

So, mode_valid is only called to filter out the modes retrieved by
get_modes, but it won't be called when userspace programs a mode. That's
atomic_check's job.

So you probably want to create a shared function between atomic_check
and mode_valid, and call it from both places (or call mode_valid from
atomic_check).

> +	/* No support for double-clock modes */
> +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> +		return MODE_BAD;
> +
> +	unsigned int mpixelclk = mode->clock * 1000;

Variables should be declared at the top of the function.

> +	if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
> +		return MODE_CLOCK_LOW;

You probably want to check the max TMDS clock too?

> +	if (inno_hdmi_find_phy_config(hdmi, mpixelclk) < 0)
> +		return MODE_CLOCK_HIGH;
> +
> +	if (hdmi->refclk) {
> +		long refclk = clk_round_rate(hdmi->refclk, mpixelclk);
> +		unsigned int max_tolerance = mpixelclk / 5000;
> +
> +		/* Vesa DMT standard mentions +/- 0.5% max tolerance */
> +		if (abs(refclk - mpixelclk) > max_tolerance ||
> +		    mpixelclk - refclk > max_tolerance;
> +			return MODE_NOCLOCK;

You should use abs_diff here. abs() will get confused by the unsigned vs
signed comparison.

> +	}
> +
> +	return MODE_OK;
> +}
> +
>  static void inno_hdmi_encoder_mode_set(struct drm_encoder *encoder,
>  				       struct drm_display_mode *mode,
>  				       struct drm_display_mode *adj_mode)
> @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
>  					 const struct drm_display_mode *mode,
>  					 struct drm_display_mode *adj_mode)
>  {
> -	return true;
> +	struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
> +
> +	return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
>  }

Why do you call mode_valid in mode_fixup?

Maxime
  

Hi Maxime,

Am 14.12.23 um 09:05 schrieb Maxime Ripard:
> Hi,
>
> On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
>> As per TRM this controller supports pixelclocks starting from 25 MHz. The
>> maximum supported pixelclocks are defined by the phy configurations we
>> have. Also it can't support modes that require doubled clocks.
>> If there is a phy reference clock we can additionally validate against
>> VESA DMT's recommendations.
>> Those checks are added to the mode_valid hook of the connector and
>> encoder's mode_fixup hook.
>>
>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>> ---
>>   drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
>>   1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> index f7f0bec725f9..2f839ff31c1c 100644
>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
>>   	struct inno_hdmi_phy_config *default_phy_config;
>>   };
>>   
>> +#define INNO_HDMI_MIN_TMDS_CLOCK  25000000U
>> +
>>   struct hdmi_data_info {
>>   	int vic;
>>   	bool sink_has_audio;
>> @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>>   	return 0;
>>   }
>>   
>> +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
>> +						 struct drm_display_mode *mode)
>> +{
> So, mode_valid is only called to filter out the modes retrieved by
> get_modes, but it won't be called when userspace programs a mode. That's
> atomic_check's job.
>
> So you probably want to create a shared function between atomic_check
> and mode_valid, and call it from both places (or call mode_valid from
> atomic_check).
This actually _is_ a shared function called in 
inno_hdmi_connector_mode_valid and inno_hdmi_encoder_mode_fixup. Yes, I  
probably should use it in atomic_check _also_.
>
>> +	/* No support for double-clock modes */
>> +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
>> +		return MODE_BAD;
>> +
>> +	unsigned int mpixelclk = mode->clock * 1000;
> Variables should be declared at the top of the function.
Oookay ... can move them.
>> +	if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
>> +		return MODE_CLOCK_LOW;
> You probably want to check the max TMDS clock too?
Not sure what you mean here. For the currently supported formats of this 
driver (rgb only) tmds clock and pixel clock are always the same.
>
>> +	if (inno_hdmi_find_phy_config(hdmi, mpixelclk) < 0)
>> +		return MODE_CLOCK_HIGH;
>> +
>> +	if (hdmi->refclk) {
>> +		long refclk = clk_round_rate(hdmi->refclk, mpixelclk);
>> +		unsigned int max_tolerance = mpixelclk / 5000;
>> +
>> +		/* Vesa DMT standard mentions +/- 0.5% max tolerance */
>> +		if (abs(refclk - mpixelclk) > max_tolerance ||
>> +		    mpixelclk - refclk > max_tolerance;
>> +			return MODE_NOCLOCK;
> You should use abs_diff here. abs() will get confused by the unsigned vs
> signed comparison.
Ack.
>
>> +	}
>> +
>> +	return MODE_OK;
>> +}
>> +
>>   static void inno_hdmi_encoder_mode_set(struct drm_encoder *encoder,
>>   				       struct drm_display_mode *mode,
>>   				       struct drm_display_mode *adj_mode)
>> @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
>>   					 const struct drm_display_mode *mode,
>>   					 struct drm_display_mode *adj_mode)
>>   {
>> -	return true;
>> +	struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
>> +
>> +	return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
>>   }
> Why do you call mode_valid in mode_fixup?

I'm calling the shared function you asked me to introduce 
(inno_hdmi_connector_mode_valid != inno_mode_valid)

Alex

>
> Maxime
  

On Thu, Dec 14, 2023 at 12:17:39PM +0100, Alex Bee wrote:
> Hi Maxime,
> 
> Am 14.12.23 um 09:05 schrieb Maxime Ripard:
> > Hi,
> > 
> > On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
> > > As per TRM this controller supports pixelclocks starting from 25 MHz. The
> > > maximum supported pixelclocks are defined by the phy configurations we
> > > have. Also it can't support modes that require doubled clocks.
> > > If there is a phy reference clock we can additionally validate against
> > > VESA DMT's recommendations.
> > > Those checks are added to the mode_valid hook of the connector and
> > > encoder's mode_fixup hook.
> > > 
> > > Signed-off-by: Alex Bee <knaerzche@gmail.com>
> > > ---
> > >   drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
> > >   1 file changed, 36 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > index f7f0bec725f9..2f839ff31c1c 100644
> > > --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
> > >   	struct inno_hdmi_phy_config *default_phy_config;
> > >   };
> > > +#define INNO_HDMI_MIN_TMDS_CLOCK  25000000U
> > > +
> > >   struct hdmi_data_info {
> > >   	int vic;
> > >   	bool sink_has_audio;
> > > @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > >   	return 0;
> > >   }
> > > +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
> > > +						 struct drm_display_mode *mode)
> > > +{
> > So, mode_valid is only called to filter out the modes retrieved by
> > get_modes, but it won't be called when userspace programs a mode. That's
> > atomic_check's job.
> > 
> > So you probably want to create a shared function between atomic_check
> > and mode_valid, and call it from both places (or call mode_valid from
> > atomic_check).
>
> This actually _is_ a shared function called in
> inno_hdmi_connector_mode_valid and inno_hdmi_encoder_mode_fixup. Yes, I 
> probably should use it in atomic_check _also_.

Yeah, I saw that later and forgot to rephrase.

> > > +	/* No support for double-clock modes */
> > > +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > +		return MODE_BAD;
> > > +
> > > +	unsigned int mpixelclk = mode->clock * 1000;
> > Variables should be declared at the top of the function.
> Oookay ... can move them.
> > > +	if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
> > > +		return MODE_CLOCK_LOW;
> > You probably want to check the max TMDS clock too?
>
> Not sure what you mean here. For the currently supported formats of this
> driver (rgb only) tmds clock and pixel clock are always the same.

I mean that your controller has a maximum TMDS rate it supports too
(probably something like 340MHz). You should also filter out the modes
that have a pixel clock higher than the one you can reach.

> > > @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
> > >   					 const struct drm_display_mode *mode,
> > >   					 struct drm_display_mode *adj_mode)
> > >   {
> > > -	return true;
> > > +	struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
> > > +
> > > +	return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
> > >   }
> > Why do you call mode_valid in mode_fixup?
> 
> I'm calling the shared function you asked me to introduce
> (inno_hdmi_connector_mode_valid != inno_mode_valid)

That's the mode_fixup part that I'm focused on :)

mode_fixup is the legacy function to adjust the mode to the controller
capabilities. It's optional, and you're not adjusting anything here,
just testing the same thing mode_valid did.

mode_valid has been superseeded by atomic_check anyway, so just drop
mode_valid and use your function in atomic_check like we discussed.

Maxime
  

Am 14.12.23 um 12:40 schrieb Maxime Ripard:
> On Thu, Dec 14, 2023 at 12:17:39PM +0100, Alex Bee wrote:
>> Hi Maxime,
>>
>> Am 14.12.23 um 09:05 schrieb Maxime Ripard:
>>> Hi,
>>>
>>> On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
>>>> As per TRM this controller supports pixelclocks starting from 25 MHz. The
>>>> maximum supported pixelclocks are defined by the phy configurations we
>>>> have. Also it can't support modes that require doubled clocks.
>>>> If there is a phy reference clock we can additionally validate against
>>>> VESA DMT's recommendations.
>>>> Those checks are added to the mode_valid hook of the connector and
>>>> encoder's mode_fixup hook.
>>>>
>>>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>>>> ---
>>>>    drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
>>>>    1 file changed, 36 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> index f7f0bec725f9..2f839ff31c1c 100644
>>>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
>>>>    	struct inno_hdmi_phy_config *default_phy_config;
>>>>    };
>>>> +#define INNO_HDMI_MIN_TMDS_CLOCK  25000000U
>>>> +
>>>>    struct hdmi_data_info {
>>>>    	int vic;
>>>>    	bool sink_has_audio;
>>>> @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>>>>    	return 0;
>>>>    }
>>>> +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
>>>> +						 struct drm_display_mode *mode)
>>>> +{
>>> So, mode_valid is only called to filter out the modes retrieved by
>>> get_modes, but it won't be called when userspace programs a mode. That's
>>> atomic_check's job.
>>>
>>> So you probably want to create a shared function between atomic_check
>>> and mode_valid, and call it from both places (or call mode_valid from
>>> atomic_check).
>> This actually _is_ a shared function called in
>> inno_hdmi_connector_mode_valid and inno_hdmi_encoder_mode_fixup. Yes, I
>> probably should use it in atomic_check _also_.
> Yeah, I saw that later and forgot to rephrase.
>
>>>> +	/* No support for double-clock modes */
>>>> +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
>>>> +		return MODE_BAD;
>>>> +
>>>> +	unsigned int mpixelclk = mode->clock * 1000;
>>> Variables should be declared at the top of the function.
>> Oookay ... can move them.
>>>> +	if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
>>>> +		return MODE_CLOCK_LOW;
>>> You probably want to check the max TMDS clock too?
>> Not sure what you mean here. For the currently supported formats of this
>> driver (rgb only) tmds clock and pixel clock are always the same.
> I mean that your controller has a maximum TMDS rate it supports too
> (probably something like 340MHz). You should also filter out the modes
> that have a pixel clock higher than the one you can reach.
Yes it does have it and it is defined by the phy configurations that do 
exist. The mode is validated against those exactly below this statement. 
(See commit message, btw.)
>
>>>> @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
>>>>    					 const struct drm_display_mode *mode,
>>>>    					 struct drm_display_mode *adj_mode)
>>>>    {
>>>> -	return true;
>>>> +	struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
>>>> +
>>>> +	return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
>>>>    }
>>> Why do you call mode_valid in mode_fixup?
>> I'm calling the shared function you asked me to introduce
>> (inno_hdmi_connector_mode_valid != inno_mode_valid)
> That's the mode_fixup part that I'm focused on :)
>
> mode_fixup is the legacy function to adjust the mode to the controller
> capabilities. It's optional, and you're not adjusting anything here,
> just testing the same thing mode_valid did.
>
> mode_valid has been superseeded by atomic_check anyway, so just drop
> mode_valid and use your function in atomic_check like we discussed.

OK.

I just read that mode_fixup won't be called at all if atomic_check 
exists. I will drop here and call in atomic_check only.

Alex

> Maxime