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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
