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

[Harry Wentland <harry.wentland@amd.com>]

On 2018-03-05 04:33 PM, Alex Deucher wrote:
> On Mon, Mar 5, 2018 at 4:15 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
>>> Ville Syrjala <ville.syrjala@linux.intel.com> writes:
>>>
>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>
>>>> 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ä <ville.syrjala@linux.intel.com>
>>>> ---
>>>>  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,
>>>> +                                             &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/ARGB8888, 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-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel