Re: [Intel-gfx] [PATCH v2 1/3] drm: Make sure at least one plane supports the fb format

2018-03-06 Thread Ville Syrjälä 
On Tue, Mar 06, 2018 at 01:57:26PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 05, 2018 at 11:41:02PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > To make life easier for drivers, let's have the core check that the
> > requested pixel format is supported by at least one plane when creating
> > a new framebuffer.
> > 
> > v2: Accept anyformat if the driver doesn't do planes (Eric)
> > s/planes_have_format/any_plane_has_format/ (Eric)
> > Check the modifier as well since we already have a function
> > that does both
> > 
> > Cc: Eric Anholt 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/drm_framebuffer.c | 43 
> > +++
> >  1 file changed, 35 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c 
> > b/drivers/gpu/drm/drm_framebuffer.c
> > index c0530a1af5e3..b9a33e3f13e9 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -152,12 +152,37 @@ static int fb_plane_height(int height,
> > return DIV_ROUND_UP(height, format->vsub);
> >  }
> >  
> > +static bool any_plane_has_format(struct drm_device *dev,
> > +u32 format, u64 modifier)
> > +{
> > +   struct drm_plane *plane;
> > +
> > +   /* TODO: maybe maintain a device level format list? */
> > +   drm_for_each_plane(plane, dev) {
> > +   /*
> > +* In case the driver doesn't really do
> > +* planes we have to accept any format here.
> > +*/
> > +   if (plane->format_default)
> > +   return true;
> > +
> > +   if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
> > +   return true;
> > +   }
> > +
> > +   return false;
> > +}
> > +
> >  static int framebuffer_check(struct drm_device *dev,
> >  const struct drm_mode_fb_cmd2 *r)
> >  {
> > const struct drm_format_info *info;
> > +   u64 modifier = 0;
> > int i;
> >  
> > +   if (r->flags & DRM_MODE_FB_MODIFIERS)
> > +   modifier = r->modifier[0];
> 
> And of course this isn't quite correct. For the !modifiers case we're
> now checking if any plane supports the format with the linear modifier,
> but the actual modifier will only be picked by the driver later.
> 
> Not quite sure how I would write this in a way that really makes
> sense. Might have to make it the driver's responsibility to call
> any_plane_has_format() instead...

Hmm. One idea would be to stick the check into drm_framebuffer_init(),
but I'm not comfortable with that since the driver may well be doing
things in its .fb_create() hook that expects the format+modifier to
have been validated already, and drm_framebuffer_init() should only be
called once everything is ready.

I guess I'll just export drm_any_plane_has_format() and add some
kerneldoc for .fb_create() to let people know about it. Better ideas
are welcome of course.

> 
> > +
> > /* check if the format is supported at all */
> > info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
> > if (!info) {
> > @@ -168,6 +193,15 @@ static int framebuffer_check(struct drm_device *dev,
> > return -EINVAL;
> > }
> >  
> > +   if (!any_plane_has_format(dev, r->pixel_format, modifier)) {
> > +   struct drm_format_name_buf format_name;
> > +
> > +   DRM_DEBUG_KMS("unsupported pixel format %s, modifier 0x%llx\n",
> > + drm_get_format_name(r->pixel_format, 
> > _name),
> > + modifier);
> > +   return -EINVAL;
> > +   }
> > +
> > /* now let the driver pick its own format info */
> > info = drm_get_format_info(dev, r);
> >  
> > @@ -202,14 +236,7 @@ static int framebuffer_check(struct drm_device *dev,
> > return -EINVAL;
> > }
> >  
> > -   if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
> > -   DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> > - r->modifier[i], i);
> > -   return -EINVAL;
> > -   }
> > -
> > -   if (r->flags & DRM_MODE_FB_MODIFIERS &&
> > -   r->modifier[i] != r->modifier[0]) {
> > +   if (r->modifier[i] != modifier) {
> > DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> >   r->modifier[i], i);
> > return -EINVAL;
> > -- 
> > 2.16.1
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org

Re: [Intel-gfx] [PATCH v2 1/3] drm: Make sure at least one plane supports the fb format

2018-03-06 Thread Ville Syrjälä 
On Mon, Mar 05, 2018 at 11:41:02PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> To make life easier for drivers, let's have the core check that the
> requested pixel format is supported by at least one plane when creating
> a new framebuffer.
> 
> v2: Accept anyformat if the driver doesn't do planes (Eric)
> s/planes_have_format/any_plane_has_format/ (Eric)
> Check the modifier as well since we already have a function
> that does both
> 
> Cc: Eric Anholt 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 43 
> +++
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c 
> b/drivers/gpu/drm/drm_framebuffer.c
> index c0530a1af5e3..b9a33e3f13e9 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -152,12 +152,37 @@ static int fb_plane_height(int height,
>   return DIV_ROUND_UP(height, format->vsub);
>  }
>  
> +static bool any_plane_has_format(struct drm_device *dev,
> +  u32 format, u64 modifier)
> +{
> + struct drm_plane *plane;
> +
> + /* TODO: maybe maintain a device level format list? */
> + drm_for_each_plane(plane, dev) {
> + /*
> +  * In case the driver doesn't really do
> +  * planes we have to accept any format here.
> +  */
> + if (plane->format_default)
> + return true;
> +
> + if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
> + return true;
> + }
> +
> + return false;
> +}
> +
>  static int framebuffer_check(struct drm_device *dev,
>const struct drm_mode_fb_cmd2 *r)
>  {
>   const struct drm_format_info *info;
> + u64 modifier = 0;
>   int i;
>  
> + if (r->flags & DRM_MODE_FB_MODIFIERS)
> + modifier = r->modifier[0];

And of course this isn't quite correct. For the !modifiers case we're
now checking if any plane supports the format with the linear modifier,
but the actual modifier will only be picked by the driver later.

Not quite sure how I would write this in a way that really makes
sense. Might have to make it the driver's responsibility to call
any_plane_has_format() instead...

> +
>   /* check if the format is supported at all */
>   info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
>   if (!info) {
> @@ -168,6 +193,15 @@ static int framebuffer_check(struct drm_device *dev,
>   return -EINVAL;
>   }
>  
> + if (!any_plane_has_format(dev, r->pixel_format, modifier)) {
> + struct drm_format_name_buf format_name;
> +
> + DRM_DEBUG_KMS("unsupported pixel format %s, modifier 0x%llx\n",
> +   drm_get_format_name(r->pixel_format, 
> _name),
> +   modifier);
> + return -EINVAL;
> + }
> +
>   /* now let the driver pick its own format info */
>   info = drm_get_format_info(dev, r);
>  
> @@ -202,14 +236,7 @@ static int framebuffer_check(struct drm_device *dev,
>   return -EINVAL;
>   }
>  
> - if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
> - DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> -   r->modifier[i], i);
> - return -EINVAL;
> - }
> -
> - if (r->flags & DRM_MODE_FB_MODIFIERS &&
> - r->modifier[i] != r->modifier[0]) {
> + if (r->modifier[i] != modifier) {
>   DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> r->modifier[i], i);
>   return -EINVAL;
> -- 
> 2.16.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 1/3] drm: Make sure at least one plane supports the fb format

2018-03-05 Thread Ville Syrjala 
From: Ville Syrjälä 

To make life easier for drivers, let's have the core check that the
requested pixel format is supported by at least one plane when creating
a new framebuffer.

v2: Accept anyformat if the driver doesn't do planes (Eric)
s/planes_have_format/any_plane_has_format/ (Eric)
Check the modifier as well since we already have a function
that does both

Cc: Eric Anholt 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_framebuffer.c | 43 +++
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c 
b/drivers/gpu/drm/drm_framebuffer.c
index c0530a1af5e3..b9a33e3f13e9 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -152,12 +152,37 @@ static int fb_plane_height(int height,
return DIV_ROUND_UP(height, format->vsub);
 }
 
+static bool any_plane_has_format(struct drm_device *dev,
+u32 format, u64 modifier)
+{
+   struct drm_plane *plane;
+
+   /* TODO: maybe maintain a device level format list? */
+   drm_for_each_plane(plane, dev) {
+   /*
+* In case the driver doesn't really do
+* planes we have to accept any format here.
+*/
+   if (plane->format_default)
+   return true;
+
+   if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
+   return true;
+   }
+
+   return false;
+}
+
 static int framebuffer_check(struct drm_device *dev,
 const struct drm_mode_fb_cmd2 *r)
 {
const struct drm_format_info *info;
+   u64 modifier = 0;
int i;
 
+   if (r->flags & DRM_MODE_FB_MODIFIERS)
+   modifier = r->modifier[0];
+
/* check if the format is supported at all */
info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
if (!info) {
@@ -168,6 +193,15 @@ static int framebuffer_check(struct drm_device *dev,
return -EINVAL;
}
 
+   if (!any_plane_has_format(dev, r->pixel_format, modifier)) {
+   struct drm_format_name_buf format_name;
+
+   DRM_DEBUG_KMS("unsupported pixel format %s, modifier 0x%llx\n",
+ drm_get_format_name(r->pixel_format, 
_name),
+ modifier);
+   return -EINVAL;
+   }
+
/* now let the driver pick its own format info */
info = drm_get_format_info(dev, r);
 
@@ -202,14 +236,7 @@ static int framebuffer_check(struct drm_device *dev,
return -EINVAL;
}
 
-   if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
-   DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
- r->modifier[i], i);
-   return -EINVAL;
-   }
-
-   if (r->flags & DRM_MODE_FB_MODIFIERS &&
-   r->modifier[i] != r->modifier[0]) {
+   if (r->modifier[i] != modifier) {
DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
  r->modifier[i], i);
return -EINVAL;
-- 
2.16.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx