| Message ID | 20180326162128.8740-2-bparrot@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26/03/18 19:21, Benoit Parrot wrote: > Currently available display mode from a connector are filtered out > based only on pixel clock capability. However we also need to filter > out wider mode if we cannot handle them based on available pipeline > capabilities. > > Signed-off-by: Benoit Parrot <bparrot@ti.com> > --- > drivers/gpu/drm/omapdrm/dss/dispc.c | 27 +++++++++++++++++++++++++++ > drivers/gpu/drm/omapdrm/dss/omapdss.h | 1 + > drivers/gpu/drm/omapdrm/omap_connector.c | 10 ++++++++++ > 3 files changed, 38 insertions(+) > > diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c > index 624dee22f46b..35541d4441df 100644 > --- a/drivers/gpu/drm/omapdrm/dss/dispc.c > +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c > @@ -100,6 +100,8 @@ struct dispc_features { > u8 mgr_height_start; > u16 mgr_width_max; > u16 mgr_height_max; > + u16 ovl_width_max; > + u16 ovl_height_max; > unsigned long max_lcd_pclk; > unsigned long max_tv_pclk; > unsigned int max_downscale; > @@ -2465,6 +2467,12 @@ static int dispc_ovl_calc_scaling(unsigned long pclk, unsigned long lclk, > return 0; > } > > +static void dispc_ovl_get_max_size(u16 *width, u16 *height) > +{ > + *width = dispc.feat->ovl_width_max; > + *height = dispc.feat->ovl_height_max; > +} > + > static int dispc_ovl_setup_common(enum omap_plane_id plane, > enum omap_overlay_caps caps, u32 paddr, u32 p_uv_addr, > u16 screen_width, int pos_x, int pos_y, u16 width, u16 height, > @@ -2500,6 +2508,10 @@ static int dispc_ovl_setup_common(enum omap_plane_id plane, > out_width = out_width == 0 ? width : out_width; > out_height = out_height == 0 ? height : out_height; > > + WARN(out_width > dispc.feat->ovl_width_max, > + "Requested OVL width (%d) is larger than can be supported (%d).\n", > + out_width, dispc.feat->ovl_width_max); Why don't you return an error here? I don't see a need for WARN here. > void dispc_set_ops(const struct dispc_ops *o); > diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c > index a0d7b1d905e8..d5e059abb555 100644 > --- a/drivers/gpu/drm/omapdrm/omap_connector.c > +++ b/drivers/gpu/drm/omapdrm/omap_connector.c > @@ -197,6 +197,16 @@ static int omap_connector_mode_valid(struct drm_connector *connector, > r = 0; > } > > + /* Check if the advertised width exceed what the pipeline can do */ > + if (!r) { > + struct omap_drm_private *priv = dev->dev_private; > + u16 width, height; > + > + priv->dispc_ops->ovl_get_max_size(&width, &height); > + if (mode->hdisplay > width) > + r = -EINVAL; You should check the height also. Tomi
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Wed [2018-Apr-04 14:12:13 +0300]: > On 26/03/18 19:21, Benoit Parrot wrote: > > Currently available display mode from a connector are filtered out > > based only on pixel clock capability. However we also need to filter > > out wider mode if we cannot handle them based on available pipeline > > capabilities. > > > > Signed-off-by: Benoit Parrot <bparrot@ti.com> > > --- > > drivers/gpu/drm/omapdrm/dss/dispc.c | 27 +++++++++++++++++++++++++++ > > drivers/gpu/drm/omapdrm/dss/omapdss.h | 1 + > > drivers/gpu/drm/omapdrm/omap_connector.c | 10 ++++++++++ > > 3 files changed, 38 insertions(+) > > > > diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c > > index 624dee22f46b..35541d4441df 100644 > > --- a/drivers/gpu/drm/omapdrm/dss/dispc.c > > +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c > > @@ -100,6 +100,8 @@ struct dispc_features { > > u8 mgr_height_start; > > u16 mgr_width_max; > > u16 mgr_height_max; > > + u16 ovl_width_max; > > + u16 ovl_height_max; > > unsigned long max_lcd_pclk; > > unsigned long max_tv_pclk; > > unsigned int max_downscale; > > @@ -2465,6 +2467,12 @@ static int dispc_ovl_calc_scaling(unsigned long pclk, unsigned long lclk, > > return 0; > > } > > > > +static void dispc_ovl_get_max_size(u16 *width, u16 *height) > > +{ > > + *width = dispc.feat->ovl_width_max; > > + *height = dispc.feat->ovl_height_max; > > +} > > + > > static int dispc_ovl_setup_common(enum omap_plane_id plane, > > enum omap_overlay_caps caps, u32 paddr, u32 p_uv_addr, > > u16 screen_width, int pos_x, int pos_y, u16 width, u16 height, > > @@ -2500,6 +2508,10 @@ static int dispc_ovl_setup_common(enum omap_plane_id plane, > > out_width = out_width == 0 ? width : out_width; > > out_height = out_height == 0 ? height : out_height; > > > > + WARN(out_width > dispc.feat->ovl_width_max, > > + "Requested OVL width (%d) is larger than can be supported (%d).\n", > > + out_width, dispc.feat->ovl_width_max); > > Why don't you return an error here? I don't see a need for WARN here. So here you mean replace the WARN with something like this: if (out_width > dispc.feat->ovl_width_max) { DSSERR("Requested OVL width (%d) is larger than can be supported (%d).\n", out_width, dispc.feat->ovl_width_max); return -EINVAL; } > > > void dispc_set_ops(const struct dispc_ops *o); > > diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c > > index a0d7b1d905e8..d5e059abb555 100644 > > --- a/drivers/gpu/drm/omapdrm/omap_connector.c > > +++ b/drivers/gpu/drm/omapdrm/omap_connector.c > > @@ -197,6 +197,16 @@ static int omap_connector_mode_valid(struct drm_connector *connector, > > r = 0; > > } > > > > + /* Check if the advertised width exceed what the pipeline can do */ > > + if (!r) { > > + struct omap_drm_private *priv = dev->dev_private; > > + u16 width, height; > > + > > + priv->dispc_ops->ovl_get_max_size(&width, &height); > > + if (mode->hdisplay > width) > > + r = -EINVAL; > > You should check the height also. Yeah, I'll fix that. Benoit > > Tomi > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Benoit, On Wednesday, 4 April 2018 16:15:11 EEST Benoit Parrot wrote: > Tomi Valkeinen wrote on Wed [2018-Apr-04 14:12:13 +0300]: > > On 26/03/18 19:21, Benoit Parrot wrote: > >> Currently available display mode from a connector are filtered out > >> based only on pixel clock capability. However we also need to filter > >> out wider mode if we cannot handle them based on available pipeline > >> capabilities. > >> > >> Signed-off-by: Benoit Parrot <bparrot@ti.com> > >> --- > >> > >> drivers/gpu/drm/omapdrm/dss/dispc.c | 27 +++++++++++++++++++++++++ > >> drivers/gpu/drm/omapdrm/dss/omapdss.h | 1 + > >> drivers/gpu/drm/omapdrm/omap_connector.c | 10 ++++++++++ > >> 3 files changed, 38 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c > >> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 624dee22f46b..35541d4441df > >> 100644 > >--- a/drivers/gpu/drm/omapdrm/dss/dispc.c > >> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c > >> @@ -100,6 +100,8 @@ struct dispc_features { > >> u8 mgr_height_start; > >> u16 mgr_width_max; > >> u16 mgr_height_max; > >> + u16 ovl_width_max; > >> + u16 ovl_height_max; > >> unsigned long max_lcd_pclk; > >> unsigned long max_tv_pclk; > >> unsigned int max_downscale; > >> @@ -2465,6 +2467,12 @@ static int dispc_ovl_calc_scaling(unsigned long > >> pclk, unsigned long lclk, > >> return 0; > >> } > >> > >> +static void dispc_ovl_get_max_size(u16 *width, u16 *height) > >> +{ > >> + *width = dispc.feat->ovl_width_max; > >> + *height = dispc.feat->ovl_height_max; > >> +} > >> + > >> static int dispc_ovl_setup_common(enum omap_plane_id plane, > >> enum omap_overlay_caps caps, u32 paddr, u32 p_uv_addr, > >> u16 screen_width, int pos_x, int pos_y, u16 width, u16 height, > >> @@ -2500,6 +2508,10 @@ static int dispc_ovl_setup_common(enum > >> omap_plane_id plane, > >> out_width = out_width == 0 ? width : out_width; > >> out_height = out_height == 0 ? height : out_height; > >> > >> + WARN(out_width > dispc.feat->ovl_width_max, > >> + "Requested OVL width (%d) is larger than can be supported > >> (%d).\n", > >> + out_width, dispc.feat->ovl_width_max); > > > > Why don't you return an error here? I don't see a need for WARN here. > > So here you mean replace the WARN with something like this: > > if (out_width > dispc.feat->ovl_width_max) { > DSSERR("Requested OVL width (%d) is larger than can be supported (%d). \n", > out_width, dispc.feat->ovl_width_max); > return -EINVAL; > } Can this happen ? If we reject invalid settings in omapdrm we should never get them here. > >> void dispc_set_ops(const struct dispc_ops *o); > >> > >> diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c > >> b/drivers/gpu/drm/omapdrm/omap_connector.c index > >> a0d7b1d905e8..d5e059abb555 100644 > >> --- a/drivers/gpu/drm/omapdrm/omap_connector.c > >> +++ b/drivers/gpu/drm/omapdrm/omap_connector.c > >> @@ -197,6 +197,16 @@ static int omap_connector_mode_valid(struct > >> drm_connector *connector, > >> r = 0; > >> } > >> > >> + /* Check if the advertised width exceed what the pipeline can do */ > >> + if (!r) { > >> + struct omap_drm_private *priv = dev->dev_private; > >> + u16 width, height; > >> + > >> + priv->dispc_ops->ovl_get_max_size(&width, &height); > >> + if (mode->hdisplay > width) > >> + r = -EINVAL; > > > > You should check the height also. > > Yeah, I'll fix that. Unless I'm mistaken the restriction doesn't come from the output side of the display controller but from the overlays (planes), right ? Shouldn't it then be implemented in the drm_plane_helper_funcs.atomic_check operation ?
On 04/04/18 17:23, Laurent Pinchart wrote: >>>> + WARN(out_width > dispc.feat->ovl_width_max, >>>> + "Requested OVL width (%d) is larger than can be supported >>>> (%d).\n", >>>> + out_width, dispc.feat->ovl_width_max); >>> >>> Why don't you return an error here? I don't see a need for WARN here. >> >> So here you mean replace the WARN with something like this: >> >> if (out_width > dispc.feat->ovl_width_max) { >> DSSERR("Requested OVL width (%d) is larger than can be supported (%d). > \n", >> out_width, dispc.feat->ovl_width_max); >> return -EINVAL; >> } > > Can this happen ? If we reject invalid settings in omapdrm we should never get > them here. That's true. And we should check them in the plane atomic check (but do we?). In that case I don't mind a warn there, but you should still return an error if it happens, instead of continuing with bad config. >>>> + /* Check if the advertised width exceed what the pipeline can do */ >>>> + if (!r) { >>>> + struct omap_drm_private *priv = dev->dev_private; >>>> + u16 width, height; >>>> + >>>> + priv->dispc_ops->ovl_get_max_size(&width, &height); >>>> + if (mode->hdisplay > width) >>>> + r = -EINVAL; >>> >>> You should check the height also. >> >> Yeah, I'll fix that. > > Unless I'm mistaken the restriction doesn't come from the output side of the > display controller but from the overlays (planes), right ? Shouldn't it then > be implemented in the drm_plane_helper_funcs.atomic_check operation ? Yes, but I don't so. If our planes can support up to, say, 1000. Then we plug in a monitor with native width of 1100, which omapdrm would accept happily and try to use it by default. But we can't show fbdev or any normal setup there, because the planes won't support it. How would we manage that? While not strictly correct, I think it's fine to reject videomodes which can't be shown with a normal full-screen plane. Tomi
Hi Tomi, On Thursday, 5 April 2018 13:21:30 EEST Tomi Valkeinen wrote: > On 04/04/18 17:23, Laurent Pinchart wrote: > >>>> + WARN(out_width > dispc.feat->ovl_width_max, > >>>> + "Requested OVL width (%d) is larger than can be supported > >>>> (%d).\n", > >>>> + out_width, dispc.feat->ovl_width_max); > >>> > >>> Why don't you return an error here? I don't see a need for WARN here. > >> > >> So here you mean replace the WARN with something like this: > >> if (out_width > dispc.feat->ovl_width_max) { > >> DSSERR("Requested OVL width (%d) is larger than can be supported > >> (%d).\n", > >> out_width, dispc.feat->ovl_width_max); > >> return -EINVAL; > >> } > > > > Can this happen ? If we reject invalid settings in omapdrm we should never > > get them here. > > That's true. And we should check them in the plane atomic check (but do > we?). We don't, that should be added. > In that case I don't mind a warn there, but you should still return an > error if it happens, instead of continuing with bad config. But this should really not happen if we add a check to the CRTC atomic_check() handler. Do you distrust the DRM core that much ? :-) > >>>> + /* Check if the advertised width exceed what the pipeline can do */ > >>>> + if (!r) { > >>>> + struct omap_drm_private *priv = dev->dev_private; > >>>> + u16 width, height; > >>>> + > >>>> + priv->dispc_ops->ovl_get_max_size(&width, &height); > >>>> + if (mode->hdisplay > width) > >>>> + r = -EINVAL; > >>> > >>> You should check the height also. > >> > >> Yeah, I'll fix that. > > > > Unless I'm mistaken the restriction doesn't come from the output side of > > the display controller but from the overlays (planes), right ? Shouldn't > > it then be implemented in the drm_plane_helper_funcs.atomic_check > > operation ? > > Yes, but I don't so. If our planes can support up to, say, 1000. Then we > plug in a monitor with native width of 1100, which omapdrm would accept > happily and try to use it by default. But we can't show fbdev or any > normal setup there, because the planes won't support it. How would we > manage that? > > While not strictly correct, I think it's fine to reject videomodes which > can't be shown with a normal full-screen plane. It could be argued that such modes would still be useful even if planes can't be shown full-screen, or that two planes could be used side by side to achieve a larger full-screen display than what would be possible with a single plane. I'll leave it up to you to decide whether we should support such use cases.
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index 624dee22f46b..35541d4441df 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -100,6 +100,8 @@ struct dispc_features { u8 mgr_height_start; u16 mgr_width_max; u16 mgr_height_max; + u16 ovl_width_max; + u16 ovl_height_max; unsigned long max_lcd_pclk; unsigned long max_tv_pclk; unsigned int max_downscale; @@ -2465,6 +2467,12 @@ static int dispc_ovl_calc_scaling(unsigned long pclk, unsigned long lclk, return 0; } +static void dispc_ovl_get_max_size(u16 *width, u16 *height) +{ + *width = dispc.feat->ovl_width_max; + *height = dispc.feat->ovl_height_max; +} + static int dispc_ovl_setup_common(enum omap_plane_id plane, enum omap_overlay_caps caps, u32 paddr, u32 p_uv_addr, u16 screen_width, int pos_x, int pos_y, u16 width, u16 height, @@ -2500,6 +2508,10 @@ static int dispc_ovl_setup_common(enum omap_plane_id plane, out_width = out_width == 0 ? width : out_width; out_height = out_height == 0 ? height : out_height; + WARN(out_width > dispc.feat->ovl_width_max, + "Requested OVL width (%d) is larger than can be supported (%d).\n", + out_width, dispc.feat->ovl_width_max); + if (ilace && height == out_height) fieldmode = true; @@ -4043,6 +4055,8 @@ static const struct dispc_features omap24xx_dispc_feats = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048, + .ovl_width_max = 2048, + .ovl_height_max = 2048, .max_lcd_pclk = 66500000, .max_downscale = 2, /* @@ -4080,6 +4094,8 @@ static const struct dispc_features omap34xx_rev1_0_dispc_feats = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048, + .ovl_width_max = 2048, + .ovl_height_max = 2048, .max_lcd_pclk = 173000000, .max_tv_pclk = 59000000, .max_downscale = 4, @@ -4114,6 +4130,8 @@ static const struct dispc_features omap34xx_rev3_0_dispc_feats = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048, + .ovl_width_max = 2048, + .ovl_height_max = 2048, .max_lcd_pclk = 173000000, .max_tv_pclk = 59000000, .max_downscale = 4, @@ -4148,6 +4166,8 @@ static const struct dispc_features omap36xx_dispc_feats = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048, + .ovl_width_max = 2048, + .ovl_height_max = 2048, .max_lcd_pclk = 173000000, .max_tv_pclk = 59000000, .max_downscale = 4, @@ -4182,6 +4202,8 @@ static const struct dispc_features am43xx_dispc_feats = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048, + .ovl_width_max = 2048, + .ovl_height_max = 2048, .max_lcd_pclk = 173000000, .max_tv_pclk = 59000000, .max_downscale = 4, @@ -4216,6 +4238,8 @@ static const struct dispc_features omap44xx_dispc_feats = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048, + .ovl_width_max = 2048, + .ovl_height_max = 2048, .max_lcd_pclk = 170000000, .max_tv_pclk = 185625000, .max_downscale = 4, @@ -4255,6 +4279,8 @@ static const struct dispc_features omap54xx_dispc_feats = { .mgr_height_start = 27, .mgr_width_max = 4096, .mgr_height_max = 4096, + .ovl_width_max = 2048, + .ovl_height_max = 4096, .max_lcd_pclk = 170000000, .max_tv_pclk = 186000000, .max_downscale = 4, @@ -4525,6 +4551,7 @@ static const struct dispc_ops dispc_ops = { .ovl_enable = dispc_ovl_enable, .ovl_setup = dispc_ovl_setup, .ovl_get_color_modes = dispc_ovl_get_color_modes, + .ovl_get_max_size = dispc_ovl_get_max_size, }; /* DISPC HW IP initialisation */ diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h index f8f83e826a56..c58c75292182 100644 --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h @@ -719,6 +719,7 @@ struct dispc_ops { enum omap_channel channel); const u32 *(*ovl_get_color_modes)(enum omap_plane_id plane); + void (*ovl_get_max_size)(u16 *width, u16 *height); }; void dispc_set_ops(const struct dispc_ops *o); diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c index a0d7b1d905e8..d5e059abb555 100644 --- a/drivers/gpu/drm/omapdrm/omap_connector.c +++ b/drivers/gpu/drm/omapdrm/omap_connector.c @@ -197,6 +197,16 @@ static int omap_connector_mode_valid(struct drm_connector *connector, r = 0; } + /* Check if the advertised width exceed what the pipeline can do */ + if (!r) { + struct omap_drm_private *priv = dev->dev_private; + u16 width, height; + + priv->dispc_ops->ovl_get_max_size(&width, &height); + if (mode->hdisplay > width) + r = -EINVAL; + } + if (!r) { /* check if vrefresh is still valid */ new_mode = drm_mode_duplicate(dev, mode);
Currently available display mode from a connector are filtered out based only on pixel clock capability. However we also need to filter out wider mode if we cannot handle them based on available pipeline capabilities. Signed-off-by: Benoit Parrot <bparrot@ti.com> --- drivers/gpu/drm/omapdrm/dss/dispc.c | 27 +++++++++++++++++++++++++++ drivers/gpu/drm/omapdrm/dss/omapdss.h | 1 + drivers/gpu/drm/omapdrm/omap_connector.c | 10 ++++++++++ 3 files changed, 38 insertions(+)