[PATCH] drm/cirrus: rewrite and modernize driver.

[Daniel Vetter]

On Thu, Apr 4, 2019 at 7:51 AM Gerd Hoffmann <kraxel at redhat.com> wrote:
>
> On Thu, Apr 04, 2019 at 12:58:09PM +1000, David Airlie wrote:
> > On Wed, Apr 3, 2019 at 5:23 PM Gerd Hoffmann <kraxel at redhat.com> wrote:
> > >
> > > Time to kill some bad sample code people are copying from ;)
> > >
> > > This is a complete rewrite of the cirrus driver.  The cirrus_mode_set()
> > > function is pretty much the only function which is carried over largely
> > > unmodified.  Everything else is upside down.
> > >
> > > It is a single monster patch.  But given that it does some pretty
> > > fundamental changes to the drivers workflow and also reduces the code
> > > size by roughly 70% I think it'll still be alot easier to review than a
> > > longish baby-step patch series.
> > >
> > > Changes summary:
> > >  - Given the small amout of video memory (4 MB) the cirrus device has
> > >    the rewritten driver doesn't try to manage buffers there.  Instead
> > >    it will blit (memcpy) the active framebuffer to video memory.
> >
> > Does it get any slower, with TTM I just wrote it to migrate just the
> > frontbuffer in/out of VRAM on modeset, won't we end up with more
> > copies now?
>
> I didn't benchmark it, but if you care about performance you should not
> be using cirrus anyway ...
>
> For fbcon it probably doesn't make much of a difference, fbcon used a
> shadow framebuffer before (for fbdev mmap and dirty tracking).
>
> xorg probably ends up with more copying.
>
> Anything doing display updates with page flips (i.e wayland) should end
> up with less copying, because you have one copy (blit) instead of two
> copies (migrate old frontbuffer out of vram, migrate new frontbuffer
> into vram) on pageflip.
>
> Speaking of wayland:  Seems at least gnome-shell insists on using XR24.

Yeah XR24 is pretty much mandatory. Noralf added a few helpers to
convert XR24 to other formats, for display not supporting anything
else. Because userspace.

> > >  - Only DRM_FORMAT_RGB565 (depth 16) is supported.  The old driver does
> > >    that too by default.  There was a module parameter which enables 24/32
> > >    bpp support and disables higher resolutions (due to cirrus hardware
> > >    constrains).  That parameter wasn't reimplemented.
>
> > This might be the big sticking point, this is a userspace regression
> > for a feature that was explicitly added a few years ago, can we really
> > get away without it?
>
> Well, I can reintroduce the module option.  I don't see any other
> reasonable way to support 32bpp.  If the driver reports XR24 as
> supported and also adds the higher resolutions (which work with RG16
> only) to the mode list userspace will of course try the higher
> resolutions with XR24 and struggle ...

Maybe atomic userspace is better (it should be, it can do TEST_ONLY),
but I'm not so sure that exposing all modes for atomic clients would
work. Also, currently not possible with our probe helpers (we don't
refilter the list per client).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch