[PATCH] drm/nouveau: prefer XBGR2101010 for addfb ioctl

[Ben Skeggs]

On Thu, Feb 22, 2018 at 3:20 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Mon, Feb 19, 2018 at 4:33 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
>> On Mon, Feb 19, 2018 at 10:21:54AM +0100, Daniel Vetter wrote:
>>> On Mon, Feb 05, 2018 at 09:10:12AM -0500, Ilia Mirkin wrote:
>>> > On Mon, Feb 5, 2018 at 8:58 AM, Ville Syrjälä
>>> > <ville.syrjala at linux.intel.com> wrote:
>>> > > On Sat, Feb 03, 2018 at 02:11:23PM -0500, Ilia Mirkin wrote:
>>> > >> Nouveau only exposes support for XBGR2101010. Prior to the atomic
>>> > >> conversion, drm would pass in the wrong format in the framebuffer, but
>>> > >> it was always ignored -- both userspace (xf86-video-nouveau) and the
>>> > >> kernel driver agreed on the layout, so the fact that the format was
>>> > >> wrong didn't matter.
>>> > >>
>>> > >> With the atomic conversion, nouveau all of a sudden started caring about
>>> > >> the exact format, and so the previously-working code in
>>> > >> xf86-video-nouveau no longer functioned since the (internally-assigned)
>>> > >> format from the addfb ioctl was wrong.
>>> > >>
>>> > >> This change adds infrastructure to allow a drm driver to specify that it
>>> > >> prefers the XBGR format variant for the addfb ioctl, and makes nouveau's
>>> > >> nv50 display driver set it. (Prior gens had no support for 30bpp at all.)
>>> > >>
>>> > >> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>> > >> Cc: stable at vger.kernel.org # v4.10+
>>> > >> ---
>>> > >>
>>> > >> Wasn't sure if the nouveau line needs to be split out into a separate
>>> > >> change or not.
>>> > >>
>>> > >>  drivers/gpu/drm/drm_framebuffer.c      | 4 ++++
>>> > >>  drivers/gpu/drm/nouveau/nv50_display.c | 1 +
>>> > >>  include/drm/drm_drv.h                  | 1 +
>>> > >>  3 files changed, 6 insertions(+)
>>> > >>
>>> > >> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>>> > >> index 5a13ff29f4f0..c0530a1af5e3 100644
>>> > >> --- a/drivers/gpu/drm/drm_framebuffer.c
>>> > >> +++ b/drivers/gpu/drm/drm_framebuffer.c
>>> > >> @@ -121,6 +121,10 @@ int drm_mode_addfb(struct drm_device *dev,
>>> > >>       r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth);
>>> > >>       r.handles[0] = or->handle;
>>> > >>
>>> > >> +     if (r.pixel_format == DRM_FORMAT_XRGB2101010 &&
>>> > >> +         dev->driver->driver_features & DRIVER_PREFER_XBGR_30BPP)
>>> > >> +             r.pixel_format = DRM_FORMAT_XBGR2101010;
>>> > >
>>> > > I think I'd much prefer if the driver just passed some kind of a
>>> > > bpp/depth->format mapping table to the core, or maybe an optional
>>> > > vfunc to allow the driver to override drm_mode_legacy_fb_format()
>>> > > behaviour.
>>> > >
>>> > > drm_mode_legacy_fb_format() is called from the fbdev setup as well
>>> > > so handling this only in addfb doesn't look sufficient.
>>> >
>>> > Well, in practice fbdev won't pick a 30bpp mode. But I get the point.
>>> > It would also enable a driver to have a funky RGB ordering for depth
>>> > 24/32 and others. Although I don't know if there are any customers for
>>> > that in practice.
>>> >
>>> > A vfunc works for me.
>>> >
>>> > Anyone else want to opine before I go for it? I'm happy to do the
>>> > work, but want to make sure I'm not just doing things that'll get
>>> > rejected, as I have a limited amount of time to do these things.
>>>
>>> Imo the very obvious hack is totally fine, makes it stick out more that we
>>> have a fairly nasty uapi inconsistency here.
>>>
>>> Also vfunc or explicit table open up the door for even more driver abuse
>>> in the future, which I don't like. vfunc for 1 case ever is also overkill.
>>>
>>> Wrt fbdev: Linus seems to have volunteered to switch fbdev from depth/bpp
>>> to explicit pixel formats, so no worries.
>>
>> Forgot to add the most important bit.
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>>
>> Also ack for merging through -nouveau. Or ping me on irc if you want me to
>> apply it to drm-misc-next.
>
> Thanks!
>
> Ben, will you take this patch? Or is drm-misc a better route? (Patch
> at https://patchwork.freedesktop.org/patch/202322/)
I'm happy for it to go through -misc-next!

Ben.

>
>   -ilia