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

2018-03-06 Thread Harry Wentland 
On 2018-03-06 05:35 AM, Daniel Vetter wrote:
> On Mon, Mar 05, 2018 at 05:44:16PM -0500, Harry Wentland wrote:
>> On 2018-03-05 04:33 PM, Alex Deucher wrote:
>>> On Mon, Mar 5, 2018 at 4:15 PM, Ville Syrjälä
>>>  wrote:
 On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
> Ville Syrjala  writes:
>
>> 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.
>>
>> Signed-off-by: Ville Syrjälä 
>> ---
>>  drivers/gpu/drm/drm_framebuffer.c | 26 ++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_framebuffer.c 
>> b/drivers/gpu/drm/drm_framebuffer.c
>> index c0530a1af5e3..155b21e579c4 100644
>> --- a/drivers/gpu/drm/drm_framebuffer.c
>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>> @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>> return DIV_ROUND_UP(height, format->vsub);
>>  }
>>
>> +static bool planes_have_format(struct drm_device *dev, u32 format)
>> +{
>> +   struct drm_plane *plane;
>> +
>> +   /* TODO: maybe maintain a device level format list? */
>> +   drm_for_each_plane(plane, dev) {
>> +   int i;
>> +
>> +   for (i = 0; i < plane->format_count; i++) {
>> +   if (plane->format_types[i] == format)
>> +   return true;
>> +   }
>> +   }
>> +
>> +   return false;
>> +}
>> +
>>  static int framebuffer_check(struct drm_device *dev,
>>  const struct drm_mode_fb_cmd2 *r)
>>  {
>> @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>> return -EINVAL;
>> }
>>
>> +   if (!planes_have_format(dev, r->pixel_format)) {
>> +   struct drm_format_name_buf format_name;
>> +
>> +   DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
>> + drm_get_format_name(r->pixel_format,
>> + _name));
>> +   return -EINVAL;
>> +   }
>> +
>
> Won't this break KMS on things like the radeon driver, which doesn't do
> planes?  Maybe check if any universal planes have been registered and
> only do the check in that case?

 Hmm. I thought we add the implicit planes always. Apparently
 drm_crtc_init() adds a primary with X/ARGB, but no more. So
 this would break all other formats, which is probably a bit too
 aggressive.

 I guess I could just skip the check in case any plane has
 plane->format_default set. That should be indicating that the driver
 doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
 Harry?
>>>
>>> Probably leftover from DC bringup?
>>
>> I'm not sure if I'm following. Which flag are we talking about?
>>
>> Is the concern the DC driver or amdgpu with DC (or radeon)?
>>
>> DC does not call drm_crtc_init(). It initializes universal planes in 
>> amdgpu_dm_initialize_drm_device() -> amdgpu_dm_plane_init().
>>
>> From a quick glance this patch looks fine by me.
> 
> From amdgpu_dm_plane_init:
> 
>   aplane->base.format_default = true;
> ^^ this is entirely bogus, pls remove.
> 

Makes sense. At one point we had a comment "for legacy code only" but that got 
lost. I'll clean it up.

Harry

>   res = drm_universal_plane_init(
>   dm->adev->ddev,
>   >base,
>   possible_crtcs,
>   _plane_funcs,
>   rgb_formats,
>   ARRAY_SIZE(rgb_formats),
>   NULL, aplane->base.type, NULL);
> 
> 
> Wrt discussions: Also checking whether any plane has set
> plane->format_default and aborting the check should keep old kms drivers
> working I hope.
> 
> Cheers, Daniel
> 
>>
>> Harry
>>
>>>
>>> Alex
>>>

>
> Also, "any_planes_have_format()" might be slightly more descriptive.

 Or any_plane_has_format()? Is that more englishy? :)

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

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

2018-03-06 Thread Daniel Vetter 
On Mon, Mar 05, 2018 at 05:44:16PM -0500, Harry Wentland wrote:
> On 2018-03-05 04:33 PM, Alex Deucher wrote:
> > On Mon, Mar 5, 2018 at 4:15 PM, Ville Syrjälä
> >  wrote:
> >> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
> >>> Ville Syrjala  writes:
> >>>
>  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.
> 
>  Signed-off-by: Ville Syrjälä 
>  ---
>   drivers/gpu/drm/drm_framebuffer.c | 26 ++
>   1 file changed, 26 insertions(+)
> 
>  diff --git a/drivers/gpu/drm/drm_framebuffer.c 
>  b/drivers/gpu/drm/drm_framebuffer.c
>  index c0530a1af5e3..155b21e579c4 100644
>  --- a/drivers/gpu/drm/drm_framebuffer.c
>  +++ b/drivers/gpu/drm/drm_framebuffer.c
>  @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>  return DIV_ROUND_UP(height, format->vsub);
>   }
> 
>  +static bool planes_have_format(struct drm_device *dev, u32 format)
>  +{
>  +   struct drm_plane *plane;
>  +
>  +   /* TODO: maybe maintain a device level format list? */
>  +   drm_for_each_plane(plane, dev) {
>  +   int i;
>  +
>  +   for (i = 0; i < plane->format_count; i++) {
>  +   if (plane->format_types[i] == format)
>  +   return true;
>  +   }
>  +   }
>  +
>  +   return false;
>  +}
>  +
>   static int framebuffer_check(struct drm_device *dev,
>   const struct drm_mode_fb_cmd2 *r)
>   {
>  @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>  return -EINVAL;
>  }
> 
>  +   if (!planes_have_format(dev, r->pixel_format)) {
>  +   struct drm_format_name_buf format_name;
>  +
>  +   DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
>  + drm_get_format_name(r->pixel_format,
>  + _name));
>  +   return -EINVAL;
>  +   }
>  +
> >>>
> >>> Won't this break KMS on things like the radeon driver, which doesn't do
> >>> planes?  Maybe check if any universal planes have been registered and
> >>> only do the check in that case?
> >>
> >> Hmm. I thought we add the implicit planes always. Apparently
> >> drm_crtc_init() adds a primary with X/ARGB, but no more. So
> >> this would break all other formats, which is probably a bit too
> >> aggressive.
> >>
> >> I guess I could just skip the check in case any plane has
> >> plane->format_default set. That should be indicating that the driver
> >> doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
> >> Harry?
> > 
> > Probably leftover from DC bringup?
> 
> I'm not sure if I'm following. Which flag are we talking about?
> 
> Is the concern the DC driver or amdgpu with DC (or radeon)?
> 
> DC does not call drm_crtc_init(). It initializes universal planes in 
> amdgpu_dm_initialize_drm_device() -> amdgpu_dm_plane_init().
> 
> From a quick glance this patch looks fine by me.

From amdgpu_dm_plane_init:

aplane->base.format_default = true;
^^ this is entirely bogus, pls remove.

res = drm_universal_plane_init(
dm->adev->ddev,
>base,
possible_crtcs,
_plane_funcs,
rgb_formats,
ARRAY_SIZE(rgb_formats),
NULL, aplane->base.type, NULL);


Wrt discussions: Also checking whether any plane has set
plane->format_default and aborting the check should keep old kms drivers
working I hope.

Cheers, Daniel

> 
> Harry
> 
> > 
> > Alex
> > 
> >>
> >>>
> >>> Also, "any_planes_have_format()" might be slightly more descriptive.
> >>
> >> Or any_plane_has_format()? Is that more englishy? :)
> >>
> >> --
> >> Ville Syrjälä
> >> Intel OTC
> >> ___
> >> dri-devel mailing list
> >> dri-de...@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 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 05:44:16PM -0500, Harry Wentland wrote:
> On 2018-03-05 04:33 PM, Alex Deucher wrote:
> > On Mon, Mar 5, 2018 at 4:15 PM, Ville Syrjälä
> >  wrote:
> >> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
> >>> Ville Syrjala  writes:
> >>>
>  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.
> 
>  Signed-off-by: Ville Syrjälä 
>  ---
>   drivers/gpu/drm/drm_framebuffer.c | 26 ++
>   1 file changed, 26 insertions(+)
> 
>  diff --git a/drivers/gpu/drm/drm_framebuffer.c 
>  b/drivers/gpu/drm/drm_framebuffer.c
>  index c0530a1af5e3..155b21e579c4 100644
>  --- a/drivers/gpu/drm/drm_framebuffer.c
>  +++ b/drivers/gpu/drm/drm_framebuffer.c
>  @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>  return DIV_ROUND_UP(height, format->vsub);
>   }
> 
>  +static bool planes_have_format(struct drm_device *dev, u32 format)
>  +{
>  +   struct drm_plane *plane;
>  +
>  +   /* TODO: maybe maintain a device level format list? */
>  +   drm_for_each_plane(plane, dev) {
>  +   int i;
>  +
>  +   for (i = 0; i < plane->format_count; i++) {
>  +   if (plane->format_types[i] == format)
>  +   return true;
>  +   }
>  +   }
>  +
>  +   return false;
>  +}
>  +
>   static int framebuffer_check(struct drm_device *dev,
>   const struct drm_mode_fb_cmd2 *r)
>   {
>  @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>  return -EINVAL;
>  }
> 
>  +   if (!planes_have_format(dev, r->pixel_format)) {
>  +   struct drm_format_name_buf format_name;
>  +
>  +   DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
>  + drm_get_format_name(r->pixel_format,
>  + _name));
>  +   return -EINVAL;
>  +   }
>  +
> >>>
> >>> Won't this break KMS on things like the radeon driver, which doesn't do
> >>> planes?  Maybe check if any universal planes have been registered and
> >>> only do the check in that case?
> >>
> >> Hmm. I thought we add the implicit planes always. Apparently
> >> drm_crtc_init() adds a primary with X/ARGB, but no more. So
> >> this would break all other formats, which is probably a bit too
> >> aggressive.
> >>
> >> I guess I could just skip the check in case any plane has
> >> plane->format_default set. That should be indicating that the driver
> >> doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
> >> Harry?
> > 
> > Probably leftover from DC bringup?
> 
> I'm not sure if I'm following. Which flag are we talking about?

In amdgpu_dm_plane_init().

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c-  switch 
(aplane->base.type) {
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c-  case 
DRM_PLANE_TYPE_PRIMARY:
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:  
aplane->base.format_default = true;

^^^
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c-
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c-  res = 
drm_universal_plane_init(

> 
> Is the concern the DC driver or amdgpu with DC (or radeon)?
> 
> DC does not call drm_crtc_init(). It initializes universal planes in 
> amdgpu_dm_initialize_drm_device() -> amdgpu_dm_plane_init().
> 
> >From a quick glance this patch looks fine by me.
> 
> Harry
> 
> > 
> > Alex
> > 
> >>
> >>>
> >>> Also, "any_planes_have_format()" might be slightly more descriptive.
> >>
> >> Or any_plane_has_format()? Is that more englishy? :)
> >>
> >> --
> >> Ville Syrjälä
> >> Intel OTC
> >> ___
> >> dri-devel mailing list
> >> dri-de...@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

2018-03-05 Thread Eric Anholt 
Ville Syrjälä  writes:

> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
>> Ville Syrjala  writes:
>> 
>> > 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.
>> >
>> > Signed-off-by: Ville Syrjälä 
>> > ---
>> >  drivers/gpu/drm/drm_framebuffer.c | 26 ++
>> >  1 file changed, 26 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_framebuffer.c 
>> > b/drivers/gpu/drm/drm_framebuffer.c
>> > index c0530a1af5e3..155b21e579c4 100644
>> > --- a/drivers/gpu/drm/drm_framebuffer.c
>> > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> > @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>> >return DIV_ROUND_UP(height, format->vsub);
>> >  }
>> >  
>> > +static bool planes_have_format(struct drm_device *dev, u32 format)
>> > +{
>> > +  struct drm_plane *plane;
>> > +
>> > +  /* TODO: maybe maintain a device level format list? */
>> > +  drm_for_each_plane(plane, dev) {
>> > +  int i;
>> > +
>> > +  for (i = 0; i < plane->format_count; i++) {
>> > +  if (plane->format_types[i] == format)
>> > +  return true;
>> > +  }
>> > +  }
>> > +
>> > +  return false;
>> > +}
>> > +
>> >  static int framebuffer_check(struct drm_device *dev,
>> > const struct drm_mode_fb_cmd2 *r)
>> >  {
>> > @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>> >return -EINVAL;
>> >}
>> >  
>> > +  if (!planes_have_format(dev, r->pixel_format)) {
>> > +  struct drm_format_name_buf format_name;
>> > +
>> > +  DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
>> > +drm_get_format_name(r->pixel_format,
>> > +_name));
>> > +  return -EINVAL;
>> > +  }
>> > +
>> 
>> Won't this break KMS on things like the radeon driver, which doesn't do
>> planes?  Maybe check if any universal planes have been registered and
>> only do the check in that case?
>
> Hmm. I thought we add the implicit planes always. Apparently
> drm_crtc_init() adds a primary with X/ARGB, but no more. So
> this would break all other formats, which is probably a bit too
> aggressive.
>
> I guess I could just skip the check in case any plane has
> plane->format_default set. That should be indicating that the driver
> doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
> Harry?
>
>> 
>> Also, "any_planes_have_format()" might be slightly more descriptive.
>
> Or any_plane_has_format()? Is that more englishy? :)

I like it.


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

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

2018-03-05 Thread Harry Wentland 
On 2018-03-05 04:33 PM, Alex Deucher wrote:
> On Mon, Mar 5, 2018 at 4:15 PM, Ville Syrjälä
>  wrote:
>> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
>>> Ville Syrjala  writes:
>>>
 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.

 Signed-off-by: Ville Syrjälä 
 ---
  drivers/gpu/drm/drm_framebuffer.c | 26 ++
  1 file changed, 26 insertions(+)

 diff --git a/drivers/gpu/drm/drm_framebuffer.c 
 b/drivers/gpu/drm/drm_framebuffer.c
 index c0530a1af5e3..155b21e579c4 100644
 --- a/drivers/gpu/drm/drm_framebuffer.c
 +++ b/drivers/gpu/drm/drm_framebuffer.c
 @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
 return DIV_ROUND_UP(height, format->vsub);
  }

 +static bool planes_have_format(struct drm_device *dev, u32 format)
 +{
 +   struct drm_plane *plane;
 +
 +   /* TODO: maybe maintain a device level format list? */
 +   drm_for_each_plane(plane, dev) {
 +   int i;
 +
 +   for (i = 0; i < plane->format_count; i++) {
 +   if (plane->format_types[i] == format)
 +   return true;
 +   }
 +   }
 +
 +   return false;
 +}
 +
  static int framebuffer_check(struct drm_device *dev,
  const struct drm_mode_fb_cmd2 *r)
  {
 @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
 return -EINVAL;
 }

 +   if (!planes_have_format(dev, r->pixel_format)) {
 +   struct drm_format_name_buf format_name;
 +
 +   DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
 + drm_get_format_name(r->pixel_format,
 + _name));
 +   return -EINVAL;
 +   }
 +
>>>
>>> Won't this break KMS on things like the radeon driver, which doesn't do
>>> planes?  Maybe check if any universal planes have been registered and
>>> only do the check in that case?
>>
>> Hmm. I thought we add the implicit planes always. Apparently
>> drm_crtc_init() adds a primary with X/ARGB, but no more. So
>> this would break all other formats, which is probably a bit too
>> aggressive.
>>
>> I guess I could just skip the check in case any plane has
>> plane->format_default set. That should be indicating that the driver
>> doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
>> Harry?
> 
> Probably leftover from DC bringup?

I'm not sure if I'm following. Which flag are we talking about?

Is the concern the DC driver or amdgpu with DC (or radeon)?

DC does not call drm_crtc_init(). It initializes universal planes in 
amdgpu_dm_initialize_drm_device() -> amdgpu_dm_plane_init().

From a quick glance this patch looks fine by me.

Harry

> 
> Alex
> 
>>
>>>
>>> Also, "any_planes_have_format()" might be slightly more descriptive.
>>
>> Or any_plane_has_format()? Is that more englishy? :)
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2018-03-05 Thread Alex Deucher 
On Mon, Mar 5, 2018 at 4:15 PM, Ville Syrjälä
 wrote:
> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
>> Ville Syrjala  writes:
>>
>> > 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.
>> >
>> > Signed-off-by: Ville Syrjälä 
>> > ---
>> >  drivers/gpu/drm/drm_framebuffer.c | 26 ++
>> >  1 file changed, 26 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_framebuffer.c 
>> > b/drivers/gpu/drm/drm_framebuffer.c
>> > index c0530a1af5e3..155b21e579c4 100644
>> > --- a/drivers/gpu/drm/drm_framebuffer.c
>> > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> > @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>> > return DIV_ROUND_UP(height, format->vsub);
>> >  }
>> >
>> > +static bool planes_have_format(struct drm_device *dev, u32 format)
>> > +{
>> > +   struct drm_plane *plane;
>> > +
>> > +   /* TODO: maybe maintain a device level format list? */
>> > +   drm_for_each_plane(plane, dev) {
>> > +   int i;
>> > +
>> > +   for (i = 0; i < plane->format_count; i++) {
>> > +   if (plane->format_types[i] == format)
>> > +   return true;
>> > +   }
>> > +   }
>> > +
>> > +   return false;
>> > +}
>> > +
>> >  static int framebuffer_check(struct drm_device *dev,
>> >  const struct drm_mode_fb_cmd2 *r)
>> >  {
>> > @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>> > return -EINVAL;
>> > }
>> >
>> > +   if (!planes_have_format(dev, r->pixel_format)) {
>> > +   struct drm_format_name_buf format_name;
>> > +
>> > +   DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
>> > + drm_get_format_name(r->pixel_format,
>> > + _name));
>> > +   return -EINVAL;
>> > +   }
>> > +
>>
>> Won't this break KMS on things like the radeon driver, which doesn't do
>> planes?  Maybe check if any universal planes have been registered and
>> only do the check in that case?
>
> Hmm. I thought we add the implicit planes always. Apparently
> drm_crtc_init() adds a primary with X/ARGB, but no more. So
> this would break all other formats, which is probably a bit too
> aggressive.
>
> I guess I could just skip the check in case any plane has
> plane->format_default set. That should be indicating that the driver
> doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
> Harry?

Probably leftover from DC bringup?

Alex

>
>>
>> Also, "any_planes_have_format()" might be slightly more descriptive.
>
> Or any_plane_has_format()? Is that more englishy? :)
>
> --
> Ville Syrjälä
> Intel OTC
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2018-03-05 Thread Ville Syrjälä 
On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
> Ville Syrjala  writes:
> 
> > 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.
> >
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/drm_framebuffer.c | 26 ++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c 
> > b/drivers/gpu/drm/drm_framebuffer.c
> > index c0530a1af5e3..155b21e579c4 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
> > return DIV_ROUND_UP(height, format->vsub);
> >  }
> >  
> > +static bool planes_have_format(struct drm_device *dev, u32 format)
> > +{
> > +   struct drm_plane *plane;
> > +
> > +   /* TODO: maybe maintain a device level format list? */
> > +   drm_for_each_plane(plane, dev) {
> > +   int i;
> > +
> > +   for (i = 0; i < plane->format_count; i++) {
> > +   if (plane->format_types[i] == format)
> > +   return true;
> > +   }
> > +   }
> > +
> > +   return false;
> > +}
> > +
> >  static int framebuffer_check(struct drm_device *dev,
> >  const struct drm_mode_fb_cmd2 *r)
> >  {
> > @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
> > return -EINVAL;
> > }
> >  
> > +   if (!planes_have_format(dev, r->pixel_format)) {
> > +   struct drm_format_name_buf format_name;
> > +
> > +   DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
> > + drm_get_format_name(r->pixel_format,
> > + _name));
> > +   return -EINVAL;
> > +   }
> > +
> 
> Won't this break KMS on things like the radeon driver, which doesn't do
> planes?  Maybe check if any universal planes have been registered and
> only do the check in that case?

Hmm. I thought we add the implicit planes always. Apparently
drm_crtc_init() adds a primary with X/ARGB, but no more. So
this would break all other formats, which is probably a bit too
aggressive.

I guess I could just skip the check in case any plane has
plane->format_default set. That should be indicating that the driver
doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
Harry?

> 
> Also, "any_planes_have_format()" might be slightly more descriptive.

Or any_plane_has_format()? Is that more englishy? :)

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

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

2018-03-05 Thread Eric Anholt 
Ville Syrjala  writes:

> 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.
>
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 26 ++
>  1 file changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_framebuffer.c 
> b/drivers/gpu/drm/drm_framebuffer.c
> index c0530a1af5e3..155b21e579c4 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>   return DIV_ROUND_UP(height, format->vsub);
>  }
>  
> +static bool planes_have_format(struct drm_device *dev, u32 format)
> +{
> + struct drm_plane *plane;
> +
> + /* TODO: maybe maintain a device level format list? */
> + drm_for_each_plane(plane, dev) {
> + int i;
> +
> + for (i = 0; i < plane->format_count; i++) {
> + if (plane->format_types[i] == format)
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
>  static int framebuffer_check(struct drm_device *dev,
>const struct drm_mode_fb_cmd2 *r)
>  {
> @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>   return -EINVAL;
>   }
>  
> + if (!planes_have_format(dev, r->pixel_format)) {
> + struct drm_format_name_buf format_name;
> +
> + DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
> +   drm_get_format_name(r->pixel_format,
> +   _name));
> + return -EINVAL;
> + }
> +

Won't this break KMS on things like the radeon driver, which doesn't do
planes?  Maybe check if any universal planes have been registered and
only do the check in that case?

Also, "any_planes_have_format()" might be slightly more descriptive.


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

[Intel-gfx] [PATCH 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.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_framebuffer.c | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/drm_framebuffer.c 
b/drivers/gpu/drm/drm_framebuffer.c
index c0530a1af5e3..155b21e579c4 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -152,6 +152,23 @@ static int fb_plane_height(int height,
return DIV_ROUND_UP(height, format->vsub);
 }
 
+static bool planes_have_format(struct drm_device *dev, u32 format)
+{
+   struct drm_plane *plane;
+
+   /* TODO: maybe maintain a device level format list? */
+   drm_for_each_plane(plane, dev) {
+   int i;
+
+   for (i = 0; i < plane->format_count; i++) {
+   if (plane->format_types[i] == format)
+   return true;
+   }
+   }
+
+   return false;
+}
+
 static int framebuffer_check(struct drm_device *dev,
 const struct drm_mode_fb_cmd2 *r)
 {
@@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
return -EINVAL;
}
 
+   if (!planes_have_format(dev, r->pixel_format)) {
+   struct drm_format_name_buf format_name;
+
+   DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
+ drm_get_format_name(r->pixel_format,
+ _name));
+   return -EINVAL;
+   }
+
/* now let the driver pick its own format info */
info = drm_get_format_info(dev, r);
 
-- 
2.16.1

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