On Wed, Feb 26, 2020 at 06:39:08AM +0100, Thomas Zimmermann wrote:
Hi
Am 25.02.20 um 18:44 schrieb Daniel Vetter:
On Tue, Feb 25, 2020 at 04:58:59PM +0100, Thomas Zimmermann wrote:
This patchset moves legacy, non-KMS driver state from struct drm_driver into struct drm_legacy_state. Only non-KMS drivers provide an instance of the latter structure. One special case is nouveau, which supports legacy interfaces. It also provides an instance of the legacy state if the legacy interfaces have been enabled (i.e., defines the config option CONFIG_NOUVEAU_LEGACY_CTX_SUPPORT)
I reviewed all call sites of legacy state and functions to verify that DRIVER_LEGACY or DRIVER_KMS_LEGACY_CONTEXT is set on the device; or that DRIVER_MODESET is not set.
With the mutable KMS state removed, instances of struct drm_driver can be declared as constant. The patchset modifies the DRM core accordingly. Individual drivers can follow later on.
Bikeshed: We very much have modern non-KMS drivers (vgem, etnaviv, v3d, panfrost, ...). non-KMS != legacy, which is what you're talking about here.
OK, probably needs to be more precise.
Other thing, and it's a bit raining on your parade: I don't see the point. Sprinkling a few more #ifdef CONFIG_DRM_LEGACY over the relevant parts sounds like a reasonable idea. But this is a lot of churn for drivers which are all pretty much dead, and just waiting for their eventual removal. And from a compile-testing pov of making sure modern drivers don't use any of the deprecated stuff wrapping it in CONFIG_DRM_LEGACY should be plenty enough.
It's not about these old drivers, it's about having constant driver structures and maybe saving a few bytes in the modern drivers. If these old drivers hold back improvements to the drivers we care about, I don't see we they are not to be touched.
This cannot be solved with CONFIG_DRM_LEGACY, unless you want to wrap any reference in core code to legacy data. I tried at first and it turned out to be an unreadable mess.
Laurent has a patch series to constify drm_driver for all !legacy drivers. Without changing the world.
So yeah it's possible, we're not hurting ourselves here (aside from the few bytes if we don't do the #ifdev CONFIG_DRM_LEGACY).
https://patchwork.freedesktop.org/series/73811/
And from a complexity POV the patchset is trivial. It adds a data structure to each old driver and moves a few initializers around. The worst thing that can happen is that code tried to dereference the legacy pointer when it's not set. This bug will happen with modern drivers, so we should see it easily.
Laurent's series is even more trivial I think. -Daniel
Best regards Thomas
And from a "make stuff const" I think Laurent's much more minimal series also gets us there for all the drivers we care about. -Daniel
Thomas Zimmermann (3): drm: Add separate state structure for legacy, non-KMS drivers drm: Move non-kms driver state into struct drm_legacy_state drm: Constify struct drm_driver in DRM core
drivers/gpu/drm/drm_bufs.c | 10 +++++----- drivers/gpu/drm/drm_context.c | 9 +++++---- drivers/gpu/drm/drm_drv.c | 12 ++++++++---- drivers/gpu/drm/drm_file.c | 4 ++-- drivers/gpu/drm/drm_legacy_misc.c | 6 +++--- drivers/gpu/drm/drm_lock.c | 7 ++++--- drivers/gpu/drm/drm_pci.c | 16 ++++++++++------ drivers/gpu/drm/drm_vblank.c | 11 ++++++----- drivers/gpu/drm/i810/i810_drv.c | 10 +++++++--- drivers/gpu/drm/mga/mga_drv.c | 16 ++++++++++------ drivers/gpu/drm/nouveau/nouveau_drm.c | 8 ++++++++ drivers/gpu/drm/r128/r128_drv.c | 16 ++++++++++------ drivers/gpu/drm/savage/savage_drv.c | 12 ++++++++---- drivers/gpu/drm/sis/sis_drv.c | 8 ++++++-- drivers/gpu/drm/tdfx/tdfx_drv.c | 6 +++++- drivers/gpu/drm/via/via_drv.c | 16 ++++++++++------ include/drm/drm_device.h | 2 +- include/drm/drm_drv.h | 21 +++++---------------- include/drm/drm_legacy.h | 27 +++++++++++++++++++++++---- include/drm/drm_pci.h | 4 ++-- 20 files changed, 138 insertions(+), 83 deletions(-)
-- 2.25.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