[PATCH] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency

[PATCH] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency

[Laurent Pinchart]

Implement a .mode_valid() handler in the R-Car glue layer to reject
modes with an unsupported clock frequency.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
index 75490a3e0a2a..8a603235f22d 100644
--- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
+++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
@@ -35,6 +35,16 @@ static const struct rcar_hdmi_phy_params rcar_hdmi_phy_params[] = {
 	{ ~0UL,      0x0000, 0x0000, 0x0000 },
 };
 
+static enum drm_mode_status
+rcar_hdmi_mode_valid(struct drm_connector *connector,
+		     const struct drm_display_mode *mode)
+{
+	if (mode->clock > 297000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
 static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
 				   const struct dw_hdmi_plat_data *pdata,
 				   unsigned long mpixelclock)
@@ -59,6 +69,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
 }
 
 static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
+	.mode_valid = rcar_hdmi_mode_valid,
 	.configure_phy	= rcar_hdmi_phy_configure,
 };
 
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency

[Kieran Bingham]

Hi Laurent,

Thank you for the patch,

On 23/11/2018 14:34, Laurent Pinchart wrote:
> Implement a .mode_valid() handler in the R-Car glue layer to reject
> modes with an unsupported clock frequency.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> index 75490a3e0a2a..8a603235f22d 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> @@ -35,6 +35,16 @@ static const struct rcar_hdmi_phy_params rcar_hdmi_phy_params[] = {
>  	{ ~0UL,      0x0000, 0x0000, 0x0000 },
>  };
>  
> +static enum drm_mode_status
> +rcar_hdmi_mode_valid(struct drm_connector *connector,
> +		     const struct drm_display_mode *mode)
> +{
> +	if (mode->clock > 297000)

Is 29700 constant? Can it be determined from any other location or is it
just a magically known platform value?

> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +
>  static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
>  				   const struct dw_hdmi_plat_data *pdata,
>  				   unsigned long mpixelclock)
> @@ -59,6 +69,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
>  }
>  
>  static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
> +	.mode_valid = rcar_hdmi_mode_valid,
>  	.configure_phy	= rcar_hdmi_phy_configure,
>  };
>  
> 

-- 
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency

[Laurent Pinchart]

Hi Kieran,

On Friday, 23 November 2018 16:43:28 EET Kieran Bingham wrote:
> On 23/11/2018 14:34, Laurent Pinchart wrote:
> > Implement a .mode_valid() handler in the R-Car glue layer to reject
> > modes with an unsupported clock frequency.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> > b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c index 75490a3e0a2a..8a603235f22d
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> > @@ -35,6 +35,16 @@ static const struct rcar_hdmi_phy_params
> > rcar_hdmi_phy_params[] = {
> >  	{ ~0UL,      0x0000, 0x0000, 0x0000 },
> >  
> >  };
> > 
> > +static enum drm_mode_status
> > +rcar_hdmi_mode_valid(struct drm_connector *connector,
> > +		     const struct drm_display_mode *mode)
> > +{
> > +	if (mode->clock > 297000)
> 
> Is 29700 constant? Can it be determined from any other location or is it
> just a magically known platform value?

It's the last entry of the rcar_hdmi_phy_params table above. I considered 
writing it

	if (mode->clock > 
rcar_hdmi_phy_params[ARRAY_SIZE(rcar_hdmi_phy_params)-2].mpixelclock)

but found it a but hard to parse. Do you think it would be better ?

> > +		return MODE_CLOCK_HIGH;
> > +
> > +	return MODE_OK;
> > +}
> > +
> > 
> >  static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
> >  				   const struct dw_hdmi_plat_data *pdata,
> >  				   unsigned long mpixelclock)
> > @@ -59,6 +69,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
> >  }
> >  
> >  static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
> > +	.mode_valid = rcar_hdmi_mode_valid,
> >  	.configure_phy	= rcar_hdmi_phy_configure,
> >  };

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency

[Kieran Bingham]

Hi Laurent,

On 23/11/2018 14:47, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Friday, 23 November 2018 16:43:28 EET Kieran Bingham wrote:
>> On 23/11/2018 14:34, Laurent Pinchart wrote:
>>> Implement a .mode_valid() handler in the R-Car glue layer to reject
>>> modes with an unsupported clock frequency.
>>>
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>
>>>  drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
>>> b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c index 75490a3e0a2a..8a603235f22d
>>> 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
>>> @@ -35,6 +35,16 @@ static const struct rcar_hdmi_phy_params
>>> rcar_hdmi_phy_params[] = {
>>>  	{ ~0UL,      0x0000, 0x0000, 0x0000 },
>>>  
>>>  };
>>>
>>> +static enum drm_mode_status
>>> +rcar_hdmi_mode_valid(struct drm_connector *connector,
>>> +		     const struct drm_display_mode *mode)
>>> +{
>>> +	if (mode->clock > 297000)
>>
>> Is 29700 constant? Can it be determined from any other location or is it
>> just a magically known platform value?
> 
> It's the last entry of the rcar_hdmi_phy_params table above. I considered 
> writing it
> 
> 	if (mode->clock > 
> rcar_hdmi_phy_params[ARRAY_SIZE(rcar_hdmi_phy_params)-2].mpixelclock)
> 
> but found it a but hard to parse. Do you think it would be better ?

Well - for readability - no,

But for accuracy - yes:

	297000000 != 297000

Unless the /1000 is intentional?

How about keep the (corrected?) constant value, but add a comment
referencing it's extraction.

I don't think the coded table extraction helps here. Especially as it
necessitates indexing against ARRAY_SIZE - 2.

--
Regards

Kieran




> 
>>> +		return MODE_CLOCK_HIGH;
>>> +
>>> +	return MODE_OK;
>>> +}
>>> +
>>>
>>>  static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
>>>  				   const struct dw_hdmi_plat_data *pdata,
>>>  				   unsigned long mpixelclock)
>>> @@ -59,6 +69,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
>>>  }
>>>  
>>>  static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
>>> +	.mode_valid = rcar_hdmi_mode_valid,
>>>  	.configure_phy	= rcar_hdmi_phy_configure,
>>>  };
> 

-- 
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency

[Laurent Pinchart]

Hi Kieran,

On Friday, 23 November 2018 17:30:43 EET Kieran Bingham wrote:
> On 23/11/2018 14:47, Laurent Pinchart wrote:
> > On Friday, 23 November 2018 16:43:28 EET Kieran Bingham wrote:
> >> On 23/11/2018 14:34, Laurent Pinchart wrote:
> >>> Implement a .mode_valid() handler in the R-Car glue layer to reject
> >>> modes with an unsupported clock frequency.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>> 
> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> >>> b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c index
> >>> 75490a3e0a2a..8a603235f22d
> >>> 100644
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> >>> @@ -35,6 +35,16 @@ static const struct rcar_hdmi_phy_params
> >>> rcar_hdmi_phy_params[] = {
> >>>  	{ ~0UL,      0x0000, 0x0000, 0x0000 },
> >>>  };
> >>> 
> >>> +static enum drm_mode_status
> >>> +rcar_hdmi_mode_valid(struct drm_connector *connector,
> >>> +		     const struct drm_display_mode *mode)
> >>> +{
> >>> +	if (mode->clock > 297000)
> >> 
> >> Is 29700 constant? Can it be determined from any other location or is it
> >> just a magically known platform value?
> > 
> > It's the last entry of the rcar_hdmi_phy_params table above. I considered
> > writing it
> > 
> > 	if (mode->clock >
> > 
> > rcar_hdmi_phy_params[ARRAY_SIZE(rcar_hdmi_phy_params)-2].mpixelclock)
> > 
> > but found it a but hard to parse. Do you think it would be better ?
> 
> Well - for readability - no,
> 
> But for accuracy - yes:
> 
> 	297000000 != 297000
> 
> Unless the /1000 is intentional?

I would have had to write / 1000 indeed :-) mode->clock is expressed in kHz.

> How about keep the (corrected?) constant value, but add a comment
> referencing it's extraction.

Good idea, I'll do so.

> I don't think the coded table extraction helps here. Especially as it
> necessitates indexing against ARRAY_SIZE - 2.
> 
> >>> +		return MODE_CLOCK_HIGH;
> >>> +
> >>> +	return MODE_OK;
> >>> +}
> >>> +
> >>>  static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
> >>>  				   const struct dw_hdmi_plat_data *pdata,
> >>>  				   unsigned long mpixelclock)
> >>> @@ -59,6 +69,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi
> >>> *hdmi,
> >>>  }
> >>>  
> >>>  static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
> >>> +	.mode_valid = rcar_hdmi_mode_valid,
> >>>  	.configure_phy	= rcar_hdmi_phy_configure,
> >>>  };

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel