Re: [PATCH] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency
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
> >>>
> >>> ---
> >>>
> >>> 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, 0x, 0x, 0x },
> >>> };
> >>>
> >>> +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:
>
> 29700 != 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
Re: [PATCH] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency
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
>>>
>>> ---
>>>
>>> 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, 0x, 0x, 0x },
>>>
>>> };
>>>
>>> +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:
29700 != 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
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
> >
> > ---
> >
> > 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, 0x, 0x, 0x },
> >
> > };
> >
> > +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
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
> ---
> 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, 0x, 0x, 0x },
> };
>
> +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
