| Message ID | 20200225155902.9751-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add separate non-KMS state; constify struct drm_driver | expand |
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. 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. 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 >
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. 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. 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 >> >
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 >