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

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

[Ilia Mirkin]

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;
+
 	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

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ilia Mirkin]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ilia Mirkin]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ben Skeggs]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel