On Mon, Feb 19, 2018 at 4:33 AM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx> >> > >> Cc: stable@xxxxxxxxxxxxxxx # 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@xxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
