subject:"\[Patch v2 1\/6\] drm\/omap\: Add ability to filter out modes which can't be supported"

Re: [Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

2018-04-24 Thread Laurent Pinchart

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(, );
>  +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.

-- 
Regards,

Laurent Pinchart



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

Re: [Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

2018-04-05 Thread Tomi Valkeinen

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(, );
 +  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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

2018-04-04 Thread Laurent Pinchart

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 
> >> ---
> >> 
> >>  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(, );
> >> +  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 ?

-- 
Regards,

Laurent Pinchart



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

Re: [Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

2018-04-04 Thread Benoit Parrot

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 
> > ---
> >  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(, );
> > +   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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

2018-04-04 Thread Tomi Valkeinen

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 
> ---
>  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(, );
> + if (mode->hdisplay > width)
> + r = -EINVAL;

You should check the height also.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

2018-03-26 Thread Benoit Parrot

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 
---
 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);
+
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   =   6650,
.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   =   17300,
.max_tv_pclk=   5900,
.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   =   17300,
.max_tv_pclk=   5900,
.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   =   17300,
.max_tv_pclk=   5900,
.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   =   17300,
.max_tv_pclk=   5900,
.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   =   17000,
.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   =

Re: [Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

Re: [Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

Re: [Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

Re: [Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

Re: [Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

[Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

6 matches

Site Navigation

Mail list logo

Footer information