Message ID | 20181123143459.13133-1-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
---|---|
State | Not Applicable |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency | expand |
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, > }; > >
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, > > };
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, >>> }; >
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, > >>> };
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, };
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(+)