Re: [PATCH v2 00/15] DRM fbconv helpers for converting fbdev drivers

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 10549 bytes --]

Hi Daniel

Am 15.10.19 um 16:33 schrieb Daniel Vetter:
> Hi Thomas,
> 
> On Mon, Oct 14, 2019 at 04:04:01PM +0200, Thomas Zimmermann wrote:
>> (was: DRM driver for fbdev devices)
>>
>> This is version 2 of the fbdev conversion helpers. It's more or less a
>> rewrite of the original patchset.
>>
>> The fbdev subsystem is considered legacy and will probably be removed at
>> some point. This would mean the loss of a signifanct number of drivers.
>> Some of the affected hardware is not in use any longer, but some hardware
>> is still around and provides good(-enough) framebuffers.
>>
>> The fbconv helpers allow for running the current DRM stack on top of fbdev
>> drivers. It's a set of functions that convert between fbdev interfaces and
>> DRM interfaces. Based on SHMEM and simple KMS helpers, it only offers the
>> basic functionality of a framebuffer, but should be compatible with most
>> existing fbdev drivers.
>>
>> A DRM driver using fbconv helpers consists of
>>
>>   * DRM stub code that calls into fbconv helpers, and
>>   * the original fbdev driver code.
>>
>> The fbdev driver code has to be modified to register itself with the
>> stub driver instead of the fbdev core framework. A tutorial on how to use
>> the helpers is part of this patchset. The resulting driver hybrid can be
>> refactored into a first-class DRM driver. The fbconv helpers contain a
>> number of comments, labeled 'DRM porting note', which explain the required
>> steps.
>>
>> I tested the current patchset with the following drivers: atyfb, aty128fb,
>> matroxfb, pm2fb, pm3fb, rivafb, s3fb, savagefb, sisfb, tdfxfb and tridentfb.
>> With each, I was able to successfully start with fbcon enabled, run weston and
>> X11. The drivers are available at [1]. For reference, the patchset includes
>> the Matrox stub driver.
> 
> So I really don't want to rain on the parade here, since if you think this
> is useful when converting fbdev drivers I'll buy that, and I'm all for
> getting more modern drivers into drm.
> 
> But I have a bunch of concerns with the approach you're proposing here:
> 
> - we've tried staging for drm driver refactoring, it hurts. Separate tree
>   plus the quick pace in refactoring create lots of pains. And for small
>   drivers refacotoring before it's not buying you anything above
>   refactoring in your own personal tree. And for big drivers we're fairly
>   lenient with merging drivers that aren't fully polished yet, if there's
>   a team serious enough with cleaning up the mess. I think even merging
>   partial drivers directly under drivers/gpu (but behind CONFIG_BROKEN) is
>   better than staging.

I mostly put this into staging, because it's the kind of code you'd
expect there.

> - we've had conversion helpers before (for the legacy kms -> atomic
>   upgrade). They constantly broke, pretty much every release when someone
>   wanted to use them they first had to fix them up again. I think having
>   those helpers is good, but better to just have them in some branch
>   somewhere where it's clear that they might not work anymore on latest
>   upstream.
> 
> - especially for some of these simple fbdev drivers I feel like just
>   typing a new driver from scratch might be simpler.
> 
> A few more concerns specifically for your mga example:
> 
> - We already have a mga driver. Might be better to integrate support for
>   older mgas into that than have a parallel driver.

Two colleagues of mine, Takashi and Egbert, send a patch that added
support for desktop G200s to mgag200. [1] But it was rejected because
the devices are two old and not relevant any longer. If that opinion has
changed in the meantime, I wouldn't mind adding support for desktop GPUs
to the driver.

> - Your helper is based on simple display pipe, and I think for these old
>   mga chips (especially the dual pipe mga 450 and 550) simple display pipe
>   helper is more a hindering detour than actual help. From a quick read
>   through the code (especially all the custom ioctls) you definitely want
>   separate TV-out connector to expose all the tv mode properties (instead
>   of the custom ioctls).

Around the G100, there's something like a change in generation. Before,
devices had only a single output and less than 8 MiB of RAM. This works
well with GEM SHMEM and simple KMS. Afterwards, devices have 8 MiB or
more and multiple outputs. GEM VRAM and the full set of helpers fit this
much better. Maybe having 2 drivers that share common code (or 3 with
the Server Engine chipsets) makes most sense.

> 
> - On the topic of ioctls, looks like we could add FBIOGET_VBLANK to our
>   generic implementation in the fbdev helpers.
> 
> So here's my alternative proposal:
> 
> - You push this as a branch onto a gitlab repo (freedesktop.org or
>   wherever you feel like).
> 
> - You add a gitlab CI target to autobuild the very nice kerneldoc you've
>   created. Feel free to also do this with anything else you're familiar
>   with, it's just I know gitlab and it's real simple to get a few docs
>   autogenerated and published with it.
> 
> - We add a todo.rst patch linking to your branch and the docs and a few
>   lines on how to best convert an fbdev driver over to kms/atomic.

Yes we can do that.

Best regards
Thomas

[1] https://lists.freedesktop.org/archives/dri-devel/2017-July/147868.html

> 
> And all the drivers would land the usual way, like any of the other
> drivers we've added to drivers/gpu/drm over the past few years.
> 
> Thoughts?
> 
> Cheers, Daniel
>>
>> v2:
>> 	* rename to fbconv helpers
>> 	* rewrite as helper library
>> 	* switch over to simple KMS helpers
>> 	* switch over to SHMEM
>> 	* add documentation
>>
>> [1] https://gitlab.freedesktop.org/tzimmermann/linux/commits/fbconv-plus-drivers
>>
>> Thomas Zimmermann (15):
>>   fbdev: Export fb_check_foreignness()
>>   fbdev: Export FBPIXMAPSIZE
>>   drm/simple-kms-helper: Add mode_fixup() to simple display pipe
>>   drm: Add fbconv helper module
>>   drm/fbconv: Add DRM <-> fbdev pixel-format conversion
>>   drm/fbconv: Add mode conversion DRM <-> fbdev
>>   drm/fbconv: Add modesetting infrastructure
>>   drm/fbconv: Add plane-state check and update
>>   drm/fbconv: Mode-setting pipeline enable / disable
>>   drm/fbconv: Reimplement several fbdev interfaces
>>   drm/fbconv: Add helpers for init and cleanup of fb_info structures
>>   drm/fbconv: Add helper documentation
>>   staging: Add mgakms driver
>>   staging/mgakms: Import matroxfb driver source code
>>   staging/mgakms: Update matroxfb driver code for DRM
>>
>>  Documentation/gpu/drm-kms-helpers.rst     |   12 +
>>  Documentation/gpu/todo.rst                |   15 +
>>  drivers/gpu/drm/Kconfig                   |   11 +
>>  drivers/gpu/drm/Makefile                  |    1 +
>>  drivers/gpu/drm/drm_fbconv_helper.c       | 2126 +++++++++++++++++
>>  drivers/gpu/drm/drm_simple_kms_helper.c   |   15 +
>>  drivers/staging/Kconfig                   |    2 +
>>  drivers/staging/Makefile                  |    1 +
>>  drivers/staging/mgakms/Kconfig            |   18 +
>>  drivers/staging/mgakms/Makefile           |   17 +
>>  drivers/staging/mgakms/g450_pll.c         |  539 +++++
>>  drivers/staging/mgakms/g450_pll.h         |   13 +
>>  drivers/staging/mgakms/i2c-matroxfb.c     |  238 ++
>>  drivers/staging/mgakms/matroxfb_DAC1064.c | 1082 +++++++++
>>  drivers/staging/mgakms/matroxfb_DAC1064.h |  174 ++
>>  drivers/staging/mgakms/matroxfb_Ti3026.c  |  746 ++++++
>>  drivers/staging/mgakms/matroxfb_Ti3026.h  |   10 +
>>  drivers/staging/mgakms/matroxfb_accel.c   |  519 +++++
>>  drivers/staging/mgakms/matroxfb_accel.h   |    9 +
>>  drivers/staging/mgakms/matroxfb_base.c    | 2592 +++++++++++++++++++++
>>  drivers/staging/mgakms/matroxfb_base.h    |  700 ++++++
>>  drivers/staging/mgakms/matroxfb_crtc2.h   |   35 +
>>  drivers/staging/mgakms/matroxfb_g450.c    |  640 +++++
>>  drivers/staging/mgakms/matroxfb_g450.h    |   10 +
>>  drivers/staging/mgakms/matroxfb_maven.h   |   21 +
>>  drivers/staging/mgakms/matroxfb_misc.c    |  815 +++++++
>>  drivers/staging/mgakms/matroxfb_misc.h    |   22 +
>>  drivers/staging/mgakms/mga_device.c       |   68 +
>>  drivers/staging/mgakms/mga_device.h       |   30 +
>>  drivers/staging/mgakms/mga_drv.c          |  129 +
>>  drivers/staging/mgakms/mga_drv.h          |   14 +
>>  drivers/video/fbdev/core/fbmem.c          |    5 +-
>>  include/drm/drm_fbconv_helper.h           |  150 ++
>>  include/drm/drm_simple_kms_helper.h       |   43 +
>>  include/linux/fb.h                        |    3 +
>>  35 files changed, 10822 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/gpu/drm/drm_fbconv_helper.c
>>  create mode 100644 drivers/staging/mgakms/Kconfig
>>  create mode 100644 drivers/staging/mgakms/Makefile
>>  create mode 100644 drivers/staging/mgakms/g450_pll.c
>>  create mode 100644 drivers/staging/mgakms/g450_pll.h
>>  create mode 100644 drivers/staging/mgakms/i2c-matroxfb.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_DAC1064.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_DAC1064.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_Ti3026.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_Ti3026.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_accel.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_accel.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_base.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_base.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_crtc2.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_g450.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_g450.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_maven.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_misc.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_misc.h
>>  create mode 100644 drivers/staging/mgakms/mga_device.c
>>  create mode 100644 drivers/staging/mgakms/mga_device.h
>>  create mode 100644 drivers/staging/mgakms/mga_drv.c
>>  create mode 100644 drivers/staging/mgakms/mga_drv.h
>>  create mode 100644 include/drm/drm_fbconv_helper.h
>>
>> --
>> 2.23.0
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]