diff mbox

drm/nouveau: prefer XBGR2101010 for addfb ioctl

Message ID 20180203191123.31507-1-imirkin@alum.mit.edu (mailing list archive)
State New, archived
Headers show

Commit Message

Ilia Mirkin Feb. 3, 2018, 7:11 p.m. UTC
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@alum.mit.edu>
Cc: stable@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(+)

Comments

Ville Syrjälä Feb. 5, 2018, 1:58 p.m. UTC | #1
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@alum.mit.edu>
> Cc: stable@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.

> +
>  	ret = drm_mode_addfb2(dev, &r, file_priv);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
> index dd8d4352ed99..caddce88d2d8 100644
> --- a/drivers/gpu/drm/nouveau/nv50_display.c
> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
> @@ -4477,6 +4477,7 @@ nv50_display_create(struct drm_device *dev)
>  	nouveau_display(dev)->fini = nv50_display_fini;
>  	disp->disp = &nouveau_display(dev)->disp;
>  	dev->mode_config.funcs = &nv50_disp_func;
> +	dev->driver->driver_features |= DRIVER_PREFER_XBGR_30BPP;
>  	if (nouveau_atomic)
>  		dev->driver->driver_features |= DRIVER_ATOMIC;
>  
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index d32b688eb346..d23dcdd1bd95 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -56,6 +56,7 @@ struct drm_printer;
>  #define DRIVER_ATOMIC			0x10000
>  #define DRIVER_KMS_LEGACY_CONTEXT	0x20000
>  #define DRIVER_SYNCOBJ                  0x40000
> +#define DRIVER_PREFER_XBGR_30BPP        0x80000
>  
>  /**
>   * struct drm_driver - DRM driver structure
> -- 
> 2.13.6
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ilia Mirkin Feb. 5, 2018, 2:10 p.m. UTC | #2
On Mon, Feb 5, 2018 at 8:58 AM, Ville Syrjälä
<ville.syrjala@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@alum.mit.edu>
>> Cc: stable@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.

Cheers,

  -ilia
Daniel Vetter Feb. 19, 2018, 9:21 a.m. UTC | #3
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@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@alum.mit.edu>
> >> Cc: stable@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.
-Daniel
Daniel Vetter Feb. 19, 2018, 9:33 a.m. UTC | #4
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@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@alum.mit.edu>
> > >> Cc: stable@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@ffwll.ch>

Also ack for merging through -nouveau. Or ping me on irc if you want me to
apply it to drm-misc-next.
-Daniel
Ilia Mirkin Feb. 21, 2018, 5:20 p.m. UTC | #5
On Mon, Feb 19, 2018 at 4:33 AM, Daniel Vetter <daniel@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@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@alum.mit.edu>
>> > >> Cc: stable@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@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/)

  -ilia
Ben Skeggs Feb. 21, 2018, 10:50 p.m. UTC | #6
On Thu, Feb 22, 2018 at 3:20 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Mon, Feb 19, 2018 at 4:33 AM, Daniel Vetter <daniel@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@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@alum.mit.edu>
>>> > >> Cc: stable@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@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
diff mbox

Patch

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;
+
 	ret = drm_mode_addfb2(dev, &r, file_priv);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
index dd8d4352ed99..caddce88d2d8 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -4477,6 +4477,7 @@  nv50_display_create(struct drm_device *dev)
 	nouveau_display(dev)->fini = nv50_display_fini;
 	disp->disp = &nouveau_display(dev)->disp;
 	dev->mode_config.funcs = &nv50_disp_func;
+	dev->driver->driver_features |= DRIVER_PREFER_XBGR_30BPP;
 	if (nouveau_atomic)
 		dev->driver->driver_features |= DRIVER_ATOMIC;
 
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index d32b688eb346..d23dcdd1bd95 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -56,6 +56,7 @@  struct drm_printer;
 #define DRIVER_ATOMIC			0x10000
 #define DRIVER_KMS_LEGACY_CONTEXT	0x20000
 #define DRIVER_SYNCOBJ                  0x40000
+#define DRIVER_PREFER_XBGR_30BPP        0x80000
 
 /**
  * struct drm_driver - DRM driver structure