| Message ID | 20191014140416.28517-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | DRM fbconv helpers for converting fbdev drivers | expand |
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. In general I like the idea of modernizing the existing fbdev drivers. What I fail to read in your intro above is if this allows us to phase out the migrated fbdev drivers sooner? Or do we end up with two drivers to maintain? Obviously a full migration to a DRM driver was preferred - but this may serve as a step in that direction. But we should not end up with two drivers doing almost the same. Another general question. Do we want the modernized DRM drivers to end up in staging? Why should they not go direct into drm/* I know they are not fully atomic but this is not new drivers so maybe they can be excused. Problem is that drm drivers in staging live a secret nonvisible life where they are easy to forget when we change interfaces and such. Sam
Hi Am 14.10.19 um 22:36 schrieb Sam Ravnborg: > 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. > > In general I like the idea of modernizing the existing fbdev drivers. > What I fail to read in your intro above is if this allows us to phase > out the migrated fbdev drivers sooner? > Or do we end up with two drivers to maintain? The idea is that an fbdev driver is converted over to DRM and, once ready, the original fbdev driver gets removed. When a hybrid driver gets added, I'd want to see the rsp developer actually clean up and refactor the code. There shouldn't be multiple drivers for long. But most of the fbdev drivers appear to be unmaintained anyway. I wouldn't expect having two drivers for a few releases would make much of a difference. > Obviously a full migration to a DRM driver was preferred - but this may > serve as a step in that direction. > But we should not end up with two drivers doing almost the same. > > Another general question. Do we want the modernized DRM drivers to end > up in staging? Why should they not go direct into drm/* > I know they are not fully atomic but this is not new drivers so maybe > they can be excused. > Problem is that drm drivers in staging live a secret nonvisible life > where they are easy to forget when we change interfaces and such. True. OTOH putting them next to the regular DRM code sends the message that the driver is already complete and in good shape. Those hybrid drivers are limited in functionality and don't really live up to anyone's requirements for code quality. It's the kind of code one would expect in staging. Best regards Thomas > Sam > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Mon, Oct 14, 2019 at 04:04:15PM +0200, Thomas Zimmermann wrote: > Only code is being copied, no functional changes are made. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/staging/mgakms/g450_pll.c | 539 +++++a ... Personally I would start from scratch. In fact some years (*cough* decade) ago I did just that and started writing a new driver for matrox stuff. Unfortunately I ran out of steam after figuring out most of the interesting hardware quirks and whatnot, so I never finished it. The end result is that it still runs in userspace but kinda looks like a kernel driver if you squint a bit. Anyways, I just slapped a MIT license on it dumped the whole thing here: https://gitlab.com/syrjala/mga The development history was, shall we say, not really useful so I just squashed it. You, or someone else, might find it interesting. I think in terms of hardware support it's a superset of any other driver, apart from the blob.
Hi Am 15.10.19 um 13:48 schrieb Ville Syrjälä: > On Mon, Oct 14, 2019 at 04:04:15PM +0200, Thomas Zimmermann wrote: >> Only code is being copied, no functional changes are made. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/staging/mgakms/g450_pll.c | 539 +++++a > > ... > > Personally I would start from scratch. In fact some years (*cough* > decade) ago I did just that and started writing a new driver for > matrox stuff. Unfortunately I ran out of steam after figuring out > most of the interesting hardware quirks and whatnot, so I never > finished it. The end result is that it still runs in userspace > but kinda looks like a kernel driver if you squint a bit. > > Anyways, I just slapped a MIT license on it dumped the whole > thing here: > https://gitlab.com/syrjala/mga > The development history was, shall we say, not really useful > so I just squashed it. > > You, or someone else, might find it interesting. I think in > terms of hardware support it's a superset of any other driver, > apart from the blob. > Just to make this clear: I do not intend to port every single fbdev driver to DRM. :) I did, however, began to convert that Matrox driver. First, to see if the approach does work in general; and because matroxfb is one of the more complex drivers. If it can be converted, any other driver should be convertible as well. I split up the driver code by HW generation and can now refactor each generation on its own. I expect to end up with several duplicated functions, which can be re-merged. Maybe our repo can be helpful. Thanks for the link. Best regards Thomas
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. - 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. - 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). - 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. 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 >
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 >> >
On Tue, Oct 15, 2019 at 7:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > 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. Yeah, except we tried, it's a real pain. Conclusion by everyone involved is that staging doesn't work for the drm subsystem. > > - 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. Hm that seems to have petered out inconclusive. I definitely think a merged mga driver is better than 2 drm atomic kms drivers for roughly the same hardware. I'm also assuming that at least for now no one plans to resurrect the 3d acceleration support for these old chips. But even then it's fairly easy to disable all that on the server chips. > > - 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. Yeah if that's the case maybe a mga100 and mga200g driver fits better. Former based on simple display pipe. You could also have them in one directory/kernel module, if there's some low-level functions worth sharing (like clock programming), with the low level driver probe either setting up the simple display based mga100 support, or the fancier mga200 support using atomic directly. If there's really no shared code at all, then separate kernel modules sounds better. > > - 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. Sounds like we have a rough plan? -Daniel > > 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 >
On Tue, Oct 15, 2019 at 07:48:29PM +0200, Daniel Vetter wrote: > On Tue, Oct 15, 2019 at 7:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > > > 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. > > Yeah, except we tried, it's a real pain. Conclusion by everyone > involved is that staging doesn't work for the drm subsystem. As the staging maintainer, I too agree with this. No drm drivers/code in staging please. thanks, greg k-h
On Tue, Oct 15, 2019 at 07:48:29PM +0200, Daniel Vetter wrote: > On Tue, Oct 15, 2019 at 7:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > > > 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. > > Yeah, except we tried, it's a real pain. Conclusion by everyone > involved is that staging doesn't work for the drm subsystem. > > > > - 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. > > Hm that seems to have petered out inconclusive. I definitely think a > merged mga driver is better than 2 drm atomic kms drivers for roughly > the same hardware. I'm also assuming that at least for now no one > plans to resurrect the 3d acceleration support for these old chips. > But even then it's fairly easy to disable all that on the server > chips. > > > > - 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. > > Yeah if that's the case maybe a mga100 and mga200g driver fits better. > Former based on simple display pipe. The display hardware differences are quite minimal from 1064SG to G200. G400 did add the second CRTC but essentially nothing else changed from G200 display. G450/G550 changed the PLLS around a bit just for the heck of it, and integrated the TMDS transmitter and TV encoder. And then they did even more PLL madness with the different G200 server chip variants. So IMO from display hw POV G100 vs. G200 split doesn't really make sense. And if you go to the effort of supporting dual head on G400+ then the single head case on earlier platforms is just the same as the single head "trivial" case on G400+, so totally different drivers doesn't make sense there either. Also not all G400 cards have the TVO/TMDS chip present in which case you can only use a single head anyway (it's almost identical to a G200 card at that point, assuming you drive the single output with the primary CRTC).
On Tue, Oct 15, 2019 at 09:13:37PM +0300, Ville Syrjälä wrote: > On Tue, Oct 15, 2019 at 07:48:29PM +0200, Daniel Vetter wrote: > > On Tue, Oct 15, 2019 at 7:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > > > > > 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. > > > > Yeah, except we tried, it's a real pain. Conclusion by everyone > > involved is that staging doesn't work for the drm subsystem. > > > > > > - 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. > > > > Hm that seems to have petered out inconclusive. I definitely think a > > merged mga driver is better than 2 drm atomic kms drivers for roughly > > the same hardware. I'm also assuming that at least for now no one > > plans to resurrect the 3d acceleration support for these old chips. > > But even then it's fairly easy to disable all that on the server > > chips. > > > > > > - 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. > > > > Yeah if that's the case maybe a mga100 and mga200g driver fits better. > > Former based on simple display pipe. > > The display hardware differences are quite minimal from > 1064SG to G200. G400 did add the second CRTC but essentially > nothing else changed from G200 display. G450/G550 changed > the PLLS around a bit just for the heck of it, and integrated > the TMDS transmitter and TV encoder. And then they did even > more PLL madness with the different G200 server chip variants. > > So IMO from display hw POV G100 vs. G200 split doesn't really > make sense. Ah, I did forget that G100 and earlier don't support the cursor 16 color mode that G200+ have. So I guess there is a little bit of a difference there.