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.
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
Non-KMS drivers store state in struct drm_driver. This bloats the structure for KMS drivers and prevents it from being declared with 'static const' qualifiers. Moving the non-KMS state into a separate data structure resolves this.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/drm_drv.c | 4 ++++ drivers/gpu/drm/i810/i810_drv.c | 4 ++++ drivers/gpu/drm/mga/mga_drv.c | 4 ++++ drivers/gpu/drm/nouveau/nouveau_drm.c | 8 ++++++++ drivers/gpu/drm/r128/r128_drv.c | 4 ++++ drivers/gpu/drm/savage/savage_drv.c | 4 ++++ drivers/gpu/drm/sis/sis_drv.c | 4 ++++ drivers/gpu/drm/tdfx/tdfx_drv.c | 4 ++++ drivers/gpu/drm/via/via_drv.c | 4 ++++ include/drm/drm_drv.h | 3 +++ include/drm/drm_legacy.h | 6 ++++++ 11 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 7b1a628d1f6e..4ba0df097602 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -625,6 +625,10 @@ int drm_dev_init(struct drm_device *dev, if (WARN_ON(!parent)) return -EINVAL;
+ if (drm_core_check_feature(dev, DRIVER_LEGACY) && + WARN_ON(!driver->legacy)) + return -EINVAL; + kref_init(&dev->ref); dev->dev = get_device(parent); dev->driver = driver; diff --git a/drivers/gpu/drm/i810/i810_drv.c b/drivers/gpu/drm/i810/i810_drv.c index 0e53a066d4db..55f17f00bae9 100644 --- a/drivers/gpu/drm/i810/i810_drv.c +++ b/drivers/gpu/drm/i810/i810_drv.c @@ -56,6 +56,9 @@ static const struct file_operations i810_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = i810_legacy_state { +}; + static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_LEGACY, .dev_priv_size = sizeof(drm_i810_buf_priv_t), @@ -71,6 +74,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, + .legacy = &i810_legacy_state, };
static struct pci_driver i810_pci_driver = { diff --git a/drivers/gpu/drm/mga/mga_drv.c b/drivers/gpu/drm/mga/mga_drv.c index 71128e6f6ae9..4865982d949c 100644 --- a/drivers/gpu/drm/mga/mga_drv.c +++ b/drivers/gpu/drm/mga/mga_drv.c @@ -53,6 +53,9 @@ static const struct file_operations mga_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = mga_legacy_state { +}; + static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_LEGACY | @@ -78,6 +81,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, + .legacy = &mga_legacy_state, };
static struct pci_driver mga_pci_driver = { diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 6b1629c14dd7..c88cf32e521c 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -1103,6 +1103,11 @@ nouveau_driver_fops = { .llseek = noop_llseek, };
+#if defined(CONFIG_NOUVEAU_LEGACY_CTX_SUPPORT) +static struct drm_legacy_state nouveau_legacy_state{ +}; +#endif + static struct drm_driver driver_stub = { .driver_features = @@ -1150,6 +1155,9 @@ driver_stub = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, +#if defined(CONFIG_NOUVEAU_LEGACY_CTX_SUPPORT) + .legacy = &nouveau_legacy_state, +#endif };
static struct pci_device_id diff --git a/drivers/gpu/drm/r128/r128_drv.c b/drivers/gpu/drm/r128/r128_drv.c index b7a5f162ebae..ecece3a48d93 100644 --- a/drivers/gpu/drm/r128/r128_drv.c +++ b/drivers/gpu/drm/r128/r128_drv.c @@ -57,6 +57,9 @@ static const struct file_operations r128_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = r128_legacy_state { +}; + static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_SG | DRIVER_LEGACY | @@ -81,6 +84,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, + .legacy = &r128_legacy_state, };
int r128_driver_load(struct drm_device *dev, unsigned long flags) diff --git a/drivers/gpu/drm/savage/savage_drv.c b/drivers/gpu/drm/savage/savage_drv.c index 799bd11adb9c..c0a7146fbde1 100644 --- a/drivers/gpu/drm/savage/savage_drv.c +++ b/drivers/gpu/drm/savage/savage_drv.c @@ -47,6 +47,9 @@ static const struct file_operations savage_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = savage_legacy_state { +}; + static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_PCI_DMA | DRIVER_LEGACY, @@ -65,6 +68,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, + .legacy = &savage_legacy_state, };
static struct pci_driver savage_pci_driver = { diff --git a/drivers/gpu/drm/sis/sis_drv.c b/drivers/gpu/drm/sis/sis_drv.c index 2c54b33abb54..1c794a82c8e6 100644 --- a/drivers/gpu/drm/sis/sis_drv.c +++ b/drivers/gpu/drm/sis/sis_drv.c @@ -99,6 +99,9 @@ static void sis_driver_postclose(struct drm_device *dev, struct drm_file *file) kfree(file_priv); }
+static struct drm_legacy_state = sis_legacy_state { +}; + static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_LEGACY, .load = sis_driver_load, @@ -116,6 +119,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, + .legacy = &sis_legacy_state, };
static struct pci_driver sis_pci_driver = { diff --git a/drivers/gpu/drm/tdfx/tdfx_drv.c b/drivers/gpu/drm/tdfx/tdfx_drv.c index ab699bf0ac5c..17aa93d82d6b 100644 --- a/drivers/gpu/drm/tdfx/tdfx_drv.c +++ b/drivers/gpu/drm/tdfx/tdfx_drv.c @@ -56,6 +56,9 @@ static const struct file_operations tdfx_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = tdfx_legacy_state { +}; + static struct drm_driver driver = { .driver_features = DRIVER_LEGACY, .fops = &tdfx_driver_fops, @@ -65,6 +68,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, + .legacy = &tdfx_legacy_state, };
static struct pci_driver tdfx_pci_driver = { diff --git a/drivers/gpu/drm/via/via_drv.c b/drivers/gpu/drm/via/via_drv.c index 5da38082821f..22113d93ecdd 100644 --- a/drivers/gpu/drm/via/via_drv.c +++ b/drivers/gpu/drm/via/via_drv.c @@ -71,6 +71,9 @@ static const struct file_operations via_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = via_legacy_state { +}; + static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_HAVE_IRQ | DRIVER_LEGACY, @@ -97,6 +100,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, + .legacy = &via_legacy_state, };
static struct pci_driver via_pci_driver = { diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 97109df5beac..94078af0cc28 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -40,6 +40,7 @@ struct dma_buf_attachment; struct drm_display_mode; struct drm_mode_create_dumb; struct drm_printer; +struct drm_legacy_state;
/** * enum drm_driver_feature - feature flags @@ -599,6 +600,8 @@ struct drm_driver { const struct file_operations *fops;
/* Everything below here is for legacy driver, never use! */ + struct drm_legacy_state *legacy; + /* private: */
/* List of devices hanging off this driver with stealth attach. */ diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h index dcef3598f49e..c98a29e1c2b3 100644 --- a/include/drm/drm_legacy.h +++ b/include/drm/drm_legacy.h @@ -49,6 +49,12 @@ struct pci_driver; * you're doing it terribly wrong. */
+/** + * State for non-KMS drivers. + */ +struct drm_legacy_state { +}; + /** * DMA buffer. */
On Tue, Feb 25, 2020 at 10:59 AM Thomas Zimmermann tzimmermann@suse.de wrote:
Non-KMS drivers store state in struct drm_driver. This bloats the structure for KMS drivers and prevents it from being declared with 'static const' qualifiers. Moving the non-KMS state into a separate data structure resolves this.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_drv.c | 4 ++++ drivers/gpu/drm/i810/i810_drv.c | 4 ++++ drivers/gpu/drm/mga/mga_drv.c | 4 ++++ drivers/gpu/drm/nouveau/nouveau_drm.c | 8 ++++++++ drivers/gpu/drm/r128/r128_drv.c | 4 ++++ drivers/gpu/drm/savage/savage_drv.c | 4 ++++ drivers/gpu/drm/sis/sis_drv.c | 4 ++++ drivers/gpu/drm/tdfx/tdfx_drv.c | 4 ++++ drivers/gpu/drm/via/via_drv.c | 4 ++++ include/drm/drm_drv.h | 3 +++ include/drm/drm_legacy.h | 6 ++++++ 11 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 7b1a628d1f6e..4ba0df097602 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -625,6 +625,10 @@ int drm_dev_init(struct drm_device *dev, if (WARN_ON(!parent)) return -EINVAL;
if (drm_core_check_feature(dev, DRIVER_LEGACY) &&WARN_ON(!driver->legacy))return -EINVAL;kref_init(&dev->ref); dev->dev = get_device(parent); dev->driver = driver;diff --git a/drivers/gpu/drm/i810/i810_drv.c b/drivers/gpu/drm/i810/i810_drv.c index 0e53a066d4db..55f17f00bae9 100644 --- a/drivers/gpu/drm/i810/i810_drv.c +++ b/drivers/gpu/drm/i810/i810_drv.c @@ -56,6 +56,9 @@ static const struct file_operations i810_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = i810_legacy_state {
Does this compile? I might have assumed this would need to be
static struct drm_legacy_state i810_legacy_state = {
+};
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_LEGACY, .dev_priv_size = sizeof(drm_i810_buf_priv_t), @@ -71,6 +74,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &i810_legacy_state,};
static struct pci_driver i810_pci_driver = { diff --git a/drivers/gpu/drm/mga/mga_drv.c b/drivers/gpu/drm/mga/mga_drv.c index 71128e6f6ae9..4865982d949c 100644 --- a/drivers/gpu/drm/mga/mga_drv.c +++ b/drivers/gpu/drm/mga/mga_drv.c @@ -53,6 +53,9 @@ static const struct file_operations mga_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = mga_legacy_state { +};
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_LEGACY | @@ -78,6 +81,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &mga_legacy_state,};
static struct pci_driver mga_pci_driver = { diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 6b1629c14dd7..c88cf32e521c 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -1103,6 +1103,11 @@ nouveau_driver_fops = { .llseek = noop_llseek, };
+#if defined(CONFIG_NOUVEAU_LEGACY_CTX_SUPPORT) +static struct drm_legacy_state nouveau_legacy_state{ +}; +#endif
static struct drm_driver driver_stub = { .driver_features = @@ -1150,6 +1155,9 @@ driver_stub = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, +#if defined(CONFIG_NOUVEAU_LEGACY_CTX_SUPPORT)
.legacy = &nouveau_legacy_state,+#endif };
static struct pci_device_id diff --git a/drivers/gpu/drm/r128/r128_drv.c b/drivers/gpu/drm/r128/r128_drv.c index b7a5f162ebae..ecece3a48d93 100644 --- a/drivers/gpu/drm/r128/r128_drv.c +++ b/drivers/gpu/drm/r128/r128_drv.c @@ -57,6 +57,9 @@ static const struct file_operations r128_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = r128_legacy_state { +};
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_SG | DRIVER_LEGACY | @@ -81,6 +84,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &r128_legacy_state,};
int r128_driver_load(struct drm_device *dev, unsigned long flags) diff --git a/drivers/gpu/drm/savage/savage_drv.c b/drivers/gpu/drm/savage/savage_drv.c index 799bd11adb9c..c0a7146fbde1 100644 --- a/drivers/gpu/drm/savage/savage_drv.c +++ b/drivers/gpu/drm/savage/savage_drv.c @@ -47,6 +47,9 @@ static const struct file_operations savage_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = savage_legacy_state { +};
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_PCI_DMA | DRIVER_LEGACY, @@ -65,6 +68,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &savage_legacy_state,};
static struct pci_driver savage_pci_driver = { diff --git a/drivers/gpu/drm/sis/sis_drv.c b/drivers/gpu/drm/sis/sis_drv.c index 2c54b33abb54..1c794a82c8e6 100644 --- a/drivers/gpu/drm/sis/sis_drv.c +++ b/drivers/gpu/drm/sis/sis_drv.c @@ -99,6 +99,9 @@ static void sis_driver_postclose(struct drm_device *dev, struct drm_file *file) kfree(file_priv); }
+static struct drm_legacy_state = sis_legacy_state { +};
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_LEGACY, .load = sis_driver_load, @@ -116,6 +119,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &sis_legacy_state,};
static struct pci_driver sis_pci_driver = { diff --git a/drivers/gpu/drm/tdfx/tdfx_drv.c b/drivers/gpu/drm/tdfx/tdfx_drv.c index ab699bf0ac5c..17aa93d82d6b 100644 --- a/drivers/gpu/drm/tdfx/tdfx_drv.c +++ b/drivers/gpu/drm/tdfx/tdfx_drv.c @@ -56,6 +56,9 @@ static const struct file_operations tdfx_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = tdfx_legacy_state { +};
static struct drm_driver driver = { .driver_features = DRIVER_LEGACY, .fops = &tdfx_driver_fops, @@ -65,6 +68,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &tdfx_legacy_state,};
static struct pci_driver tdfx_pci_driver = { diff --git a/drivers/gpu/drm/via/via_drv.c b/drivers/gpu/drm/via/via_drv.c index 5da38082821f..22113d93ecdd 100644 --- a/drivers/gpu/drm/via/via_drv.c +++ b/drivers/gpu/drm/via/via_drv.c @@ -71,6 +71,9 @@ static const struct file_operations via_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = via_legacy_state { +};
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_HAVE_IRQ | DRIVER_LEGACY, @@ -97,6 +100,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &via_legacy_state,};
static struct pci_driver via_pci_driver = { diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 97109df5beac..94078af0cc28 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -40,6 +40,7 @@ struct dma_buf_attachment; struct drm_display_mode; struct drm_mode_create_dumb; struct drm_printer; +struct drm_legacy_state;
/**
- enum drm_driver_feature - feature flags
@@ -599,6 +600,8 @@ struct drm_driver { const struct file_operations *fops;
/* Everything below here is for legacy driver, never use! */
struct drm_legacy_state *legacy;/* private: */ /* List of devices hanging off this driver with stealth attach. */diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h index dcef3598f49e..c98a29e1c2b3 100644 --- a/include/drm/drm_legacy.h +++ b/include/drm/drm_legacy.h @@ -49,6 +49,12 @@ struct pci_driver;
- you're doing it terribly wrong.
*/
+/**
- State for non-KMS drivers.
- */
+struct drm_legacy_state { +};
/**
- DMA buffer.
*/
2.25.0
Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Hi
Am 25.02.20 um 17:06 schrieb Ilia Mirkin:
On Tue, Feb 25, 2020 at 10:59 AM Thomas Zimmermann tzimmermann@suse.de wrote:
Non-KMS drivers store state in struct drm_driver. This bloats the structure for KMS drivers and prevents it from being declared with 'static const' qualifiers. Moving the non-KMS state into a separate data structure resolves this.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_drv.c | 4 ++++ drivers/gpu/drm/i810/i810_drv.c | 4 ++++ drivers/gpu/drm/mga/mga_drv.c | 4 ++++ drivers/gpu/drm/nouveau/nouveau_drm.c | 8 ++++++++ drivers/gpu/drm/r128/r128_drv.c | 4 ++++ drivers/gpu/drm/savage/savage_drv.c | 4 ++++ drivers/gpu/drm/sis/sis_drv.c | 4 ++++ drivers/gpu/drm/tdfx/tdfx_drv.c | 4 ++++ drivers/gpu/drm/via/via_drv.c | 4 ++++ include/drm/drm_drv.h | 3 +++ include/drm/drm_legacy.h | 6 ++++++ 11 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 7b1a628d1f6e..4ba0df097602 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -625,6 +625,10 @@ int drm_dev_init(struct drm_device *dev, if (WARN_ON(!parent)) return -EINVAL;
if (drm_core_check_feature(dev, DRIVER_LEGACY) &&WARN_ON(!driver->legacy))return -EINVAL;kref_init(&dev->ref); dev->dev = get_device(parent); dev->driver = driver;diff --git a/drivers/gpu/drm/i810/i810_drv.c b/drivers/gpu/drm/i810/i810_drv.c index 0e53a066d4db..55f17f00bae9 100644 --- a/drivers/gpu/drm/i810/i810_drv.c +++ b/drivers/gpu/drm/i810/i810_drv.c @@ -56,6 +56,9 @@ static const struct file_operations i810_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = i810_legacy_state {
Does this compile? I might have assumed this would need to be
I compiled an tested this patchset. But apparently I sent an old patch or something went wrong somewhere. Thanks for pointing this out. I'll resend a fixed version.
Best regards Thomas
static struct drm_legacy_state i810_legacy_state = {
+};
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_LEGACY, .dev_priv_size = sizeof(drm_i810_buf_priv_t), @@ -71,6 +74,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &i810_legacy_state,};
static struct pci_driver i810_pci_driver = { diff --git a/drivers/gpu/drm/mga/mga_drv.c b/drivers/gpu/drm/mga/mga_drv.c index 71128e6f6ae9..4865982d949c 100644 --- a/drivers/gpu/drm/mga/mga_drv.c +++ b/drivers/gpu/drm/mga/mga_drv.c @@ -53,6 +53,9 @@ static const struct file_operations mga_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = mga_legacy_state { +};
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_LEGACY | @@ -78,6 +81,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &mga_legacy_state,};
static struct pci_driver mga_pci_driver = { diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 6b1629c14dd7..c88cf32e521c 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -1103,6 +1103,11 @@ nouveau_driver_fops = { .llseek = noop_llseek, };
+#if defined(CONFIG_NOUVEAU_LEGACY_CTX_SUPPORT) +static struct drm_legacy_state nouveau_legacy_state{ +}; +#endif
static struct drm_driver driver_stub = { .driver_features = @@ -1150,6 +1155,9 @@ driver_stub = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, +#if defined(CONFIG_NOUVEAU_LEGACY_CTX_SUPPORT)
.legacy = &nouveau_legacy_state,+#endif };
static struct pci_device_id diff --git a/drivers/gpu/drm/r128/r128_drv.c b/drivers/gpu/drm/r128/r128_drv.c index b7a5f162ebae..ecece3a48d93 100644 --- a/drivers/gpu/drm/r128/r128_drv.c +++ b/drivers/gpu/drm/r128/r128_drv.c @@ -57,6 +57,9 @@ static const struct file_operations r128_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = r128_legacy_state { +};
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_SG | DRIVER_LEGACY | @@ -81,6 +84,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &r128_legacy_state,};
int r128_driver_load(struct drm_device *dev, unsigned long flags) diff --git a/drivers/gpu/drm/savage/savage_drv.c b/drivers/gpu/drm/savage/savage_drv.c index 799bd11adb9c..c0a7146fbde1 100644 --- a/drivers/gpu/drm/savage/savage_drv.c +++ b/drivers/gpu/drm/savage/savage_drv.c @@ -47,6 +47,9 @@ static const struct file_operations savage_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = savage_legacy_state { +};
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_PCI_DMA | DRIVER_LEGACY, @@ -65,6 +68,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &savage_legacy_state,};
static struct pci_driver savage_pci_driver = { diff --git a/drivers/gpu/drm/sis/sis_drv.c b/drivers/gpu/drm/sis/sis_drv.c index 2c54b33abb54..1c794a82c8e6 100644 --- a/drivers/gpu/drm/sis/sis_drv.c +++ b/drivers/gpu/drm/sis/sis_drv.c @@ -99,6 +99,9 @@ static void sis_driver_postclose(struct drm_device *dev, struct drm_file *file) kfree(file_priv); }
+static struct drm_legacy_state = sis_legacy_state { +};
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_LEGACY, .load = sis_driver_load, @@ -116,6 +119,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &sis_legacy_state,};
static struct pci_driver sis_pci_driver = { diff --git a/drivers/gpu/drm/tdfx/tdfx_drv.c b/drivers/gpu/drm/tdfx/tdfx_drv.c index ab699bf0ac5c..17aa93d82d6b 100644 --- a/drivers/gpu/drm/tdfx/tdfx_drv.c +++ b/drivers/gpu/drm/tdfx/tdfx_drv.c @@ -56,6 +56,9 @@ static const struct file_operations tdfx_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = tdfx_legacy_state { +};
static struct drm_driver driver = { .driver_features = DRIVER_LEGACY, .fops = &tdfx_driver_fops, @@ -65,6 +68,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &tdfx_legacy_state,};
static struct pci_driver tdfx_pci_driver = { diff --git a/drivers/gpu/drm/via/via_drv.c b/drivers/gpu/drm/via/via_drv.c index 5da38082821f..22113d93ecdd 100644 --- a/drivers/gpu/drm/via/via_drv.c +++ b/drivers/gpu/drm/via/via_drv.c @@ -71,6 +71,9 @@ static const struct file_operations via_driver_fops = { .llseek = noop_llseek, };
+static struct drm_legacy_state = via_legacy_state { +};
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_HAVE_IRQ | DRIVER_LEGACY, @@ -97,6 +100,7 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
.legacy = &via_legacy_state,};
static struct pci_driver via_pci_driver = { diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 97109df5beac..94078af0cc28 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -40,6 +40,7 @@ struct dma_buf_attachment; struct drm_display_mode; struct drm_mode_create_dumb; struct drm_printer; +struct drm_legacy_state;
/**
- enum drm_driver_feature - feature flags
@@ -599,6 +600,8 @@ struct drm_driver { const struct file_operations *fops;
/* Everything below here is for legacy driver, never use! */
struct drm_legacy_state *legacy;/* private: */ /* List of devices hanging off this driver with stealth attach. */diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h index dcef3598f49e..c98a29e1c2b3 100644 --- a/include/drm/drm_legacy.h +++ b/include/drm/drm_legacy.h @@ -49,6 +49,12 @@ struct pci_driver;
- you're doing it terribly wrong.
*/
+/**
- State for non-KMS drivers.
- */
+struct drm_legacy_state { +};
/**
- DMA buffer.
*/
2.25.0
Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Thomas,
On Tuesday, 25 February 2020, Thomas Zimmermann tzimmermann@suse.de wrote:
Non-KMS drivers store state in struct drm_driver. This bloats the structure for KMS drivers and prevents it from being declared with 'static const' qualifiers. Moving the non-KMS state into a separate data structure resolves this.
Have you considered subclassing the const drm_driver for legacy drivers. This way the legacy (pointer) data does not need to be exposed to non-legacy drivers.
Not to mention that const data (drm_driver), which has (const) pointer to mutable data (legacy bits) is a trap IMHO
Emil
Hi Emil
Am 25.02.20 um 18:37 schrieb Emil Velikov:
Hi Thomas,
On Tuesday, 25 February 2020, Thomas Zimmermann <tzimmermann@suse.de mailto:tzimmermann@suse.de> wrote:
Non-KMS drivers store state in struct drm_driver. This bloats the structure for KMS drivers and prevents it from being declared with 'static const' qualifiers. Moving the non-KMS state into a separate data structure resolves this.Have you considered subclassing the const drm_driver for legacy drivers. This way the legacy (pointer) data does not need to be exposed to non-legacy drivers.
I didn't want to do an upcast each time the legacy data is required. I know that subclassing is closer to the style of current DRM, but given the current code base, the additional legacy pointer seemed like the lesser evil.
Best regards Thomas
Not to mention that const data (drm_driver), which has (const) pointer to mutable data (legacy bits) is a trap IMHO
Emil
All non-kms driver fields are now located in struct drm_legacy_state. A driver-wide instance is available via struct drm_driver.legacy. The call sites test if the driver is marked with DRIVER_LEGACY before accessing the fields.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/drm_bufs.c | 10 +++++----- drivers/gpu/drm/drm_context.c | 9 +++++---- drivers/gpu/drm/drm_drv.c | 8 ++++---- 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 | 8 +++++--- drivers/gpu/drm/drm_vblank.c | 11 ++++++----- drivers/gpu/drm/i810/i810_drv.c | 8 ++++---- drivers/gpu/drm/mga/mga_drv.c | 14 +++++++------- drivers/gpu/drm/r128/r128_drv.c | 14 +++++++------- drivers/gpu/drm/savage/savage_drv.c | 10 +++++----- drivers/gpu/drm/sis/sis_drv.c | 6 +++--- drivers/gpu/drm/tdfx/tdfx_drv.c | 2 +- drivers/gpu/drm/via/via_drv.c | 14 +++++++------- include/drm/drm_drv.h | 14 -------------- include/drm/drm_legacy.h | 11 +++++++++++ 17 files changed, 79 insertions(+), 77 deletions(-)
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index 19297e58b232..fbbd5f5820a4 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -812,7 +812,7 @@ int drm_legacy_addbufs_agp(struct drm_device *dev, buf->pending = 0; buf->file_priv = NULL;
- buf->dev_priv_size = dev->driver->dev_priv_size; + buf->dev_priv_size = dev->driver->legacy->dev_priv_size; buf->dev_private = kzalloc(buf->dev_priv_size, GFP_KERNEL); if (!buf->dev_private) { /* Set count correctly so we free the proper amount. */ @@ -1011,7 +1011,7 @@ int drm_legacy_addbufs_pci(struct drm_device *dev, buf->pending = 0; buf->file_priv = NULL;
- buf->dev_priv_size = dev->driver->dev_priv_size; + buf->dev_priv_size = dev->driver->legacy->dev_priv_size; buf->dev_private = kzalloc(buf->dev_priv_size, GFP_KERNEL); if (!buf->dev_private) { @@ -1175,7 +1175,7 @@ static int drm_legacy_addbufs_sg(struct drm_device *dev, buf->pending = 0; buf->file_priv = NULL;
- buf->dev_priv_size = dev->driver->dev_priv_size; + buf->dev_priv_size = dev->driver->legacy->dev_priv_size; buf->dev_private = kzalloc(buf->dev_priv_size, GFP_KERNEL); if (!buf->dev_private) { /* Set count correctly so we free the proper amount. */ @@ -1581,8 +1581,8 @@ int drm_legacy_dma_ioctl(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_LEGACY)) return -EOPNOTSUPP;
- if (dev->driver->dma_ioctl) - return dev->driver->dma_ioctl(dev, data, file_priv); + if (dev->driver->legacy->dma_ioctl) + return dev->driver->legacy->dma_ioctl(dev, data, file_priv); else return -EINVAL; } diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c index 1f802d8e5681..b433f755d7c2 100644 --- a/drivers/gpu/drm/drm_context.c +++ b/drivers/gpu/drm/drm_context.c @@ -145,8 +145,9 @@ void drm_legacy_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file) list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) { if (pos->tag == file && pos->handle != DRM_KERNEL_CONTEXT) { - if (dev->driver->context_dtor) - dev->driver->context_dtor(dev, pos->handle); + if (dev->driver->legacy->context_dtor) + dev->driver->legacy->context_dtor(dev, + pos->handle);
drm_legacy_ctxbitmap_free(dev, pos->handle); list_del(&pos->head); @@ -500,8 +501,8 @@ int drm_legacy_rmctx(struct drm_device *dev, void *data,
DRM_DEBUG("%d\n", ctx->handle); if (ctx->handle != DRM_KERNEL_CONTEXT) { - if (dev->driver->context_dtor) - dev->driver->context_dtor(dev, ctx->handle); + if (dev->driver->legacy->context_dtor) + dev->driver->legacy->context_dtor(dev, ctx->handle); drm_legacy_ctxbitmap_free(dev, ctx->handle); }
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 4ba0df097602..4875b76e7656 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -625,14 +625,14 @@ int drm_dev_init(struct drm_device *dev, if (WARN_ON(!parent)) return -EINVAL;
- if (drm_core_check_feature(dev, DRIVER_LEGACY) && - WARN_ON(!driver->legacy)) - return -EINVAL; - kref_init(&dev->ref); dev->dev = get_device(parent); dev->driver = driver;
+ if (drm_core_check_feature(dev, DRIVER_LEGACY) && + WARN_ON(!driver->legacy)) + return -EINVAL; + /* no per-device feature limits by default */ dev->driver_features = ~0u;
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index c4c704e01961..e9cb7cf2eb48 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -254,8 +254,8 @@ void drm_file_free(struct drm_file *file) atomic_read(&dev->open_count));
if (drm_core_check_feature(dev, DRIVER_LEGACY) && - dev->driver->preclose) - dev->driver->preclose(dev, file); + dev->driver->legacy->preclose) + dev->driver->legacy->preclose(dev, file);
if (drm_core_check_feature(dev, DRIVER_LEGACY)) drm_legacy_lock_release(dev, file->filp); diff --git a/drivers/gpu/drm/drm_legacy_misc.c b/drivers/gpu/drm/drm_legacy_misc.c index 8f54e6a78b6f..07172d0a4f6f 100644 --- a/drivers/gpu/drm/drm_legacy_misc.c +++ b/drivers/gpu/drm/drm_legacy_misc.c @@ -60,9 +60,9 @@ int drm_legacy_setup(struct drm_device * dev) { int ret;
- if (dev->driver->firstopen && - drm_core_check_feature(dev, DRIVER_LEGACY)) { - ret = dev->driver->firstopen(dev); + if (drm_core_check_feature(dev, DRIVER_LEGACY) && + dev->driver->legacy->firstopen) { + ret = dev->driver->legacy->firstopen(dev); if (ret != 0) return ret; } diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c index 2c79e8199e3c..f07bf74065ec 100644 --- a/drivers/gpu/drm/drm_lock.c +++ b/drivers/gpu/drm/drm_lock.c @@ -223,7 +223,7 @@ int drm_legacy_lock(struct drm_device *dev, void *data, ret ? "interrupted" : "has lock"); if (ret) return ret;
- /* don't set the block all signals on the master process for now + /* don't set the block all signals on the master process for now * really probably not the correct answer but lets us debug xkb * xserver for now */ if (!drm_is_current_master(file_priv)) { @@ -231,9 +231,10 @@ int drm_legacy_lock(struct drm_device *dev, void *data, dev->sigdata.lock = master->lock.hw_lock; }
- if (dev->driver->dma_quiescent && (lock->flags & _DRM_LOCK_QUIESCENT)) + if (dev->driver->legacy->dma_quiescent && + (lock->flags & _DRM_LOCK_QUIESCENT)) { - if (dev->driver->dma_quiescent(dev)) { + if (dev->driver->legacy->dma_quiescent(dev)) { DRM_DEBUG("%d waiting for DMA quiescent\n", lock->context); return -EBUSY; diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index c6bb98729a26..24ed0e92fd66 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -239,7 +239,8 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, /* No locking needed since shadow-attach is single-threaded since it may * only be called from the per-driver module init hook. */ if (drm_core_check_feature(dev, DRIVER_LEGACY)) - list_add_tail(&dev->legacy_dev_list, &driver->legacy_dev_list); + list_add_tail(&dev->legacy_dev_list, + &driver->legacy->legacy_dev_list);
return 0;
@@ -275,7 +276,7 @@ int drm_legacy_pci_init(struct drm_driver *driver, struct pci_driver *pdriver) return -EINVAL;
/* If not using KMS, fall back to stealth mode manual scanning. */ - INIT_LIST_HEAD(&driver->legacy_dev_list); + INIT_LIST_HEAD(&driver->legacy->legacy_dev_list); for (i = 0; pdriver->id_table[i].vendor != 0; i++) { pid = &pdriver->id_table[i];
@@ -317,7 +318,8 @@ void drm_legacy_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver) if (!(driver->driver_features & DRIVER_LEGACY)) { WARN_ON(1); } else { - list_for_each_entry_safe(dev, tmp, &driver->legacy_dev_list, + list_for_each_entry_safe(dev, tmp, + &driver->legacy->legacy_dev_list, legacy_dev_list) { list_del(&dev->legacy_dev_list); drm_put_dev(dev); diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 47fc4339ec7f..8655cf04712b 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -30,6 +30,7 @@ #include <drm/drm_crtc.h> #include <drm/drm_drv.h> #include <drm/drm_framebuffer.h> +#include <drm/drm_legacy.h> #include <drm/drm_modeset_helper_vtables.h> #include <drm/drm_print.h> #include <drm/drm_vblank.h> @@ -144,8 +145,8 @@ static u32 __get_vblank_counter(struct drm_device *dev, unsigned int pipe)
if (crtc->funcs->get_vblank_counter) return crtc->funcs->get_vblank_counter(crtc); - } else if (dev->driver->get_vblank_counter) { - return dev->driver->get_vblank_counter(dev, pipe); + } else if (dev->driver->legacy->get_vblank_counter) { + return dev->driver->legacy->get_vblank_counter(dev, pipe); }
return drm_vblank_no_hw_counter(dev, pipe); @@ -364,7 +365,7 @@ static void __disable_vblank(struct drm_device *dev, unsigned int pipe) if (crtc->funcs->disable_vblank) crtc->funcs->disable_vblank(crtc); } else { - dev->driver->disable_vblank(dev, pipe); + dev->driver->legacy->disable_vblank(dev, pipe); } }
@@ -1033,8 +1034,8 @@ static int __enable_vblank(struct drm_device *dev, unsigned int pipe)
if (crtc->funcs->enable_vblank) return crtc->funcs->enable_vblank(crtc); - } else if (dev->driver->enable_vblank) { - return dev->driver->enable_vblank(dev, pipe); + } else if (dev->driver->legacy->enable_vblank) { + return dev->driver->legacy->enable_vblank(dev, pipe); }
return -EINVAL; diff --git a/drivers/gpu/drm/i810/i810_drv.c b/drivers/gpu/drm/i810/i810_drv.c index 55f17f00bae9..dd036e3ceb3d 100644 --- a/drivers/gpu/drm/i810/i810_drv.c +++ b/drivers/gpu/drm/i810/i810_drv.c @@ -56,16 +56,16 @@ static const struct file_operations i810_driver_fops = { .llseek = noop_llseek, };
-static struct drm_legacy_state = i810_legacy_state { +static struct drm_legacy_state i810_legacy_state = { + .dev_priv_size = sizeof(drm_i810_buf_priv_t), + .preclose = i810_driver_preclose, + .dma_quiescent = i810_driver_dma_quiescent, };
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_LEGACY, - .dev_priv_size = sizeof(drm_i810_buf_priv_t), .load = i810_driver_load, .lastclose = i810_driver_lastclose, - .preclose = i810_driver_preclose, - .dma_quiescent = i810_driver_dma_quiescent, .ioctls = i810_ioctls, .fops = &i810_driver_fops, .name = DRIVER_NAME, diff --git a/drivers/gpu/drm/mga/mga_drv.c b/drivers/gpu/drm/mga/mga_drv.c index 4865982d949c..2454c57f9370 100644 --- a/drivers/gpu/drm/mga/mga_drv.c +++ b/drivers/gpu/drm/mga/mga_drv.c @@ -53,27 +53,27 @@ static const struct file_operations mga_driver_fops = { .llseek = noop_llseek, };
-static struct drm_legacy_state = mga_legacy_state { +static struct drm_legacy_state mga_legacy_state = { + .dev_priv_size = sizeof(drm_mga_buf_priv_t), + .dma_quiescent = mga_driver_dma_quiescent, + .get_vblank_counter = mga_get_vblank_counter, + .enable_vblank = mga_enable_vblank, + .disable_vblank = mga_disable_vblank, + .dma_ioctl = mga_dma_buffers, };
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_LEGACY | DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ, - .dev_priv_size = sizeof(drm_mga_buf_priv_t), .load = mga_driver_load, .unload = mga_driver_unload, .lastclose = mga_driver_lastclose, - .dma_quiescent = mga_driver_dma_quiescent, - .get_vblank_counter = mga_get_vblank_counter, - .enable_vblank = mga_enable_vblank, - .disable_vblank = mga_disable_vblank, .irq_preinstall = mga_driver_irq_preinstall, .irq_postinstall = mga_driver_irq_postinstall, .irq_uninstall = mga_driver_irq_uninstall, .irq_handler = mga_driver_irq_handler, .ioctls = mga_ioctls, - .dma_ioctl = mga_dma_buffers, .fops = &mga_driver_fops, .name = DRIVER_NAME, .desc = DRIVER_DESC, diff --git a/drivers/gpu/drm/r128/r128_drv.c b/drivers/gpu/drm/r128/r128_drv.c index ecece3a48d93..9cfd86117e80 100644 --- a/drivers/gpu/drm/r128/r128_drv.c +++ b/drivers/gpu/drm/r128/r128_drv.c @@ -57,26 +57,26 @@ static const struct file_operations r128_driver_fops = { .llseek = noop_llseek, };
-static struct drm_legacy_state = r128_legacy_state { +static struct drm_legacy_state r128_legacy_state = { + .dev_priv_size = sizeof(drm_r128_buf_priv_t), + .preclose = r128_driver_preclose, + .get_vblank_counter = r128_get_vblank_counter, + .enable_vblank = r128_enable_vblank, + .disable_vblank = r128_disable_vblank, + .dma_ioctl = r128_cce_buffers, };
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_SG | DRIVER_LEGACY | DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ, - .dev_priv_size = sizeof(drm_r128_buf_priv_t), .load = r128_driver_load, - .preclose = r128_driver_preclose, .lastclose = r128_driver_lastclose, - .get_vblank_counter = r128_get_vblank_counter, - .enable_vblank = r128_enable_vblank, - .disable_vblank = r128_disable_vblank, .irq_preinstall = r128_driver_irq_preinstall, .irq_postinstall = r128_driver_irq_postinstall, .irq_uninstall = r128_driver_irq_uninstall, .irq_handler = r128_driver_irq_handler, .ioctls = r128_ioctls, - .dma_ioctl = r128_cce_buffers, .fops = &r128_driver_fops, .name = DRIVER_NAME, .desc = DRIVER_DESC, diff --git a/drivers/gpu/drm/savage/savage_drv.c b/drivers/gpu/drm/savage/savage_drv.c index c0a7146fbde1..2d2680b47eff 100644 --- a/drivers/gpu/drm/savage/savage_drv.c +++ b/drivers/gpu/drm/savage/savage_drv.c @@ -47,20 +47,20 @@ static const struct file_operations savage_driver_fops = { .llseek = noop_llseek, };
-static struct drm_legacy_state = savage_legacy_state { +static struct drm_legacy_state savage_legacy_state = { + .dev_priv_size = sizeof(drm_savage_buf_priv_t), + .firstopen = savage_driver_firstopen, + .preclose = savage_reclaim_buffers, + .dma_ioctl = savage_bci_buffers, };
static struct drm_driver driver = { .driver_features = DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_PCI_DMA | DRIVER_LEGACY, - .dev_priv_size = sizeof(drm_savage_buf_priv_t), .load = savage_driver_load, - .firstopen = savage_driver_firstopen, - .preclose = savage_reclaim_buffers, .lastclose = savage_driver_lastclose, .unload = savage_driver_unload, .ioctls = savage_ioctls, - .dma_ioctl = savage_bci_buffers, .fops = &savage_driver_fops, .name = DRIVER_NAME, .desc = DRIVER_DESC, diff --git a/drivers/gpu/drm/sis/sis_drv.c b/drivers/gpu/drm/sis/sis_drv.c index 1c794a82c8e6..c465776e3180 100644 --- a/drivers/gpu/drm/sis/sis_drv.c +++ b/drivers/gpu/drm/sis/sis_drv.c @@ -99,7 +99,9 @@ static void sis_driver_postclose(struct drm_device *dev, struct drm_file *file) kfree(file_priv); }
-static struct drm_legacy_state = sis_legacy_state { +static struct drm_legacy_state sis_legacy_state = { + .preclose = sis_reclaim_buffers_locked, + .dma_quiescent = sis_idle, };
static struct drm_driver driver = { @@ -107,9 +109,7 @@ static struct drm_driver driver = { .load = sis_driver_load, .unload = sis_driver_unload, .open = sis_driver_open, - .preclose = sis_reclaim_buffers_locked, .postclose = sis_driver_postclose, - .dma_quiescent = sis_idle, .lastclose = sis_lastclose, .ioctls = sis_ioctls, .fops = &sis_driver_fops, diff --git a/drivers/gpu/drm/tdfx/tdfx_drv.c b/drivers/gpu/drm/tdfx/tdfx_drv.c index 17aa93d82d6b..a8a6bce6afbe 100644 --- a/drivers/gpu/drm/tdfx/tdfx_drv.c +++ b/drivers/gpu/drm/tdfx/tdfx_drv.c @@ -56,7 +56,7 @@ static const struct file_operations tdfx_driver_fops = { .llseek = noop_llseek, };
-static struct drm_legacy_state = tdfx_legacy_state { +static struct drm_legacy_state tdfx_legacy_state = { };
static struct drm_driver driver = { diff --git a/drivers/gpu/drm/via/via_drv.c b/drivers/gpu/drm/via/via_drv.c index 22113d93ecdd..62f026da5958 100644 --- a/drivers/gpu/drm/via/via_drv.c +++ b/drivers/gpu/drm/via/via_drv.c @@ -71,7 +71,13 @@ static const struct file_operations via_driver_fops = { .llseek = noop_llseek, };
-static struct drm_legacy_state = via_legacy_state { +static struct drm_legacy_state via_legacy_state = { + .preclose = via_reclaim_buffers_locked, + .context_dtor = via_final_context, + .get_vblank_counter = via_get_vblank_counter, + .enable_vblank = via_enable_vblank, + .disable_vblank = via_disable_vblank, + .dma_quiescent = via_driver_dma_quiescent, };
static struct drm_driver driver = { @@ -80,17 +86,11 @@ static struct drm_driver driver = { .load = via_driver_load, .unload = via_driver_unload, .open = via_driver_open, - .preclose = via_reclaim_buffers_locked, .postclose = via_driver_postclose, - .context_dtor = via_final_context, - .get_vblank_counter = via_get_vblank_counter, - .enable_vblank = via_enable_vblank, - .disable_vblank = via_disable_vblank, .irq_preinstall = via_driver_irq_preinstall, .irq_postinstall = via_driver_irq_postinstall, .irq_uninstall = via_driver_irq_uninstall, .irq_handler = via_driver_irq_handler, - .dma_quiescent = via_driver_dma_quiescent, .lastclose = via_lastclose, .ioctls = via_ioctls, .fops = &via_driver_fops, diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 94078af0cc28..15dd9a179216 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -601,20 +601,6 @@ struct drm_driver {
/* Everything below here is for legacy driver, never use! */ struct drm_legacy_state *legacy; - - /* private: */ - - /* List of devices hanging off this driver with stealth attach. */ - struct list_head legacy_dev_list; - int (*firstopen) (struct drm_device *); - void (*preclose) (struct drm_device *, struct drm_file *file_priv); - int (*dma_ioctl) (struct drm_device *dev, void *data, struct drm_file *file_priv); - int (*dma_quiescent) (struct drm_device *); - int (*context_dtor) (struct drm_device *dev, int context); - u32 (*get_vblank_counter)(struct drm_device *dev, unsigned int pipe); - int (*enable_vblank)(struct drm_device *dev, unsigned int pipe); - void (*disable_vblank)(struct drm_device *dev, unsigned int pipe); - int dev_priv_size; };
int drm_dev_init(struct drm_device *dev, diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h index c98a29e1c2b3..7b5ee6a94a8c 100644 --- a/include/drm/drm_legacy.h +++ b/include/drm/drm_legacy.h @@ -53,6 +53,17 @@ struct pci_driver; * State for non-KMS drivers. */ struct drm_legacy_state { + /* List of devices hanging off this driver with stealth attach. */ + struct list_head legacy_dev_list; + int (*firstopen)(struct drm_device *dev); + void (*preclose)(struct drm_device *dev, struct drm_file *file_priv); + int (*dma_ioctl)(struct drm_device *dev, void *data, struct drm_file *file_priv); + int (*dma_quiescent)(struct drm_device *dev); + int (*context_dtor)(struct drm_device *dev, int context); + u32 (*get_vblank_counter)(struct drm_device *dev, unsigned int pipe); + int (*enable_vblank)(struct drm_device *dev, unsigned int pipe); + void (*disable_vblank)(struct drm_device *dev, unsigned int pipe); + int dev_priv_size; };
/**
Once used for creatign a DRM device, instances of struct drm_driver are not modified. So all related variables in the DRM core can be declared 'const'.
In addition to the core interfaces, the patch also changes tdfx accordingly. The other non-KMS drivers set.num_ioctls outside of their instance's initialization, so they cannot have the instance declared const yet.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/drm_drv.c | 8 ++++---- drivers/gpu/drm/drm_pci.c | 8 +++++--- drivers/gpu/drm/tdfx/tdfx_drv.c | 2 +- include/drm/drm_device.h | 2 +- include/drm/drm_drv.h | 6 +++--- include/drm/drm_legacy.h | 10 ++++++---- include/drm/drm_pci.h | 4 ++-- 7 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 4875b76e7656..98d1f4169e6c 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -612,7 +612,7 @@ static void drm_fs_inode_free(struct inode *inode) * 0 on success, or error code on failure. */ int drm_dev_init(struct drm_device *dev, - struct drm_driver *driver, + const struct drm_driver *driver, struct device *parent) { int ret; @@ -726,7 +726,7 @@ static void devm_drm_dev_init_release(void *data) */ int devm_drm_dev_init(struct device *parent, struct drm_device *dev, - struct drm_driver *driver) + const struct drm_driver *driver) { int ret;
@@ -804,7 +804,7 @@ EXPORT_SYMBOL(drm_dev_fini); * RETURNS: * Pointer to new DRM device, or ERR_PTR on failure. */ -struct drm_device *drm_dev_alloc(struct drm_driver *driver, +struct drm_device *drm_dev_alloc(const struct drm_driver *driver, struct device *parent) { struct drm_device *dev; @@ -947,7 +947,7 @@ static void remove_compat_control_link(struct drm_device *dev) */ int drm_dev_register(struct drm_device *dev, unsigned long flags) { - struct drm_driver *driver = dev->driver; + const struct drm_driver *driver = dev->driver; int ret;
if (drm_dev_needs_global_mutex(dev)) diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 24ed0e92fd66..f6b7b30f7c5e 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -207,7 +207,7 @@ void drm_pci_agp_destroy(struct drm_device *dev) * Return: 0 on success or a negative error code on failure. */ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, - struct drm_driver *driver) + const struct drm_driver *driver) { struct drm_device *dev; int ret; @@ -264,7 +264,8 @@ EXPORT_SYMBOL(drm_get_pci_dev); * * Return: 0 on success or a negative error code on failure. */ -int drm_legacy_pci_init(struct drm_driver *driver, struct pci_driver *pdriver) +int drm_legacy_pci_init(const struct drm_driver *driver, + struct pci_driver *pdriver) { struct pci_dev *pdev = NULL; const struct pci_device_id *pid; @@ -310,7 +311,8 @@ EXPORT_SYMBOL(drm_legacy_pci_init); * Unregister a DRM driver shadow-attached through drm_legacy_pci_init(). This * is deprecated and only used by dri1 drivers. */ -void drm_legacy_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver) +void drm_legacy_pci_exit(const struct drm_driver *driver, + struct pci_driver *pdriver) { struct drm_device *dev, *tmp; DRM_DEBUG("\n"); diff --git a/drivers/gpu/drm/tdfx/tdfx_drv.c b/drivers/gpu/drm/tdfx/tdfx_drv.c index a8a6bce6afbe..637c9352c1af 100644 --- a/drivers/gpu/drm/tdfx/tdfx_drv.c +++ b/drivers/gpu/drm/tdfx/tdfx_drv.c @@ -59,7 +59,7 @@ static const struct file_operations tdfx_driver_fops = { static struct drm_legacy_state tdfx_legacy_state = { };
-static struct drm_driver driver = { +static const struct drm_driver driver = { .driver_features = DRIVER_LEGACY, .fops = &tdfx_driver_fops, .name = DRIVER_NAME, diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h index bb60a949f416..6b2e2799d83c 100644 --- a/include/drm/drm_device.h +++ b/include/drm/drm_device.h @@ -68,7 +68,7 @@ struct drm_device { struct device *dev;
/** @driver: DRM driver managing the device */ - struct drm_driver *driver; + const struct drm_driver *driver;
/** * @dev_private: diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 15dd9a179216..887728d28226 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -604,14 +604,14 @@ struct drm_driver { };
int drm_dev_init(struct drm_device *dev, - struct drm_driver *driver, + const struct drm_driver *driver, struct device *parent); int devm_drm_dev_init(struct device *parent, struct drm_device *dev, - struct drm_driver *driver); + const struct drm_driver *driver); void drm_dev_fini(struct drm_device *dev);
-struct drm_device *drm_dev_alloc(struct drm_driver *driver, +struct drm_device *drm_dev_alloc(const struct drm_driver *driver, struct device *parent); int drm_dev_register(struct drm_device *dev, unsigned long flags); void drm_dev_unregister(struct drm_device *dev); diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h index 7b5ee6a94a8c..abc77cf84752 100644 --- a/include/drm/drm_legacy.h +++ b/include/drm/drm_legacy.h @@ -211,18 +211,20 @@ void drm_legacy_idlelock_release(struct drm_lock_data *lock);
#ifdef CONFIG_PCI
-int drm_legacy_pci_init(struct drm_driver *driver, struct pci_driver *pdriver); -void drm_legacy_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver); +int drm_legacy_pci_init(const struct drm_driver *driver, + struct pci_driver *pdriver); +void drm_legacy_pci_exit(const struct drm_driver *driver, + struct pci_driver *pdriver);
#else
-static inline int drm_legacy_pci_init(struct drm_driver *driver, +static inline int drm_legacy_pci_init(const struct drm_driver *driver, struct pci_driver *pdriver) { return -EINVAL; }
-static inline void drm_legacy_pci_exit(struct drm_driver *driver, +static inline void drm_legacy_pci_exit(const struct drm_driver *driver, struct pci_driver *pdriver) { } diff --git a/include/drm/drm_pci.h b/include/drm/drm_pci.h index 9031e217b506..8f98ae8384c2 100644 --- a/include/drm/drm_pci.h +++ b/include/drm/drm_pci.h @@ -47,7 +47,7 @@ void drm_pci_free(struct drm_device *dev, struct drm_dma_handle * dmah);
int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, - struct drm_driver *driver); + const struct drm_driver *driver);
#else
@@ -64,7 +64,7 @@ static inline void drm_pci_free(struct drm_device *dev,
static inline int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, - struct drm_driver *driver) + const struct drm_driver *driver) { return -ENOSYS; }
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Emil Velikov -
Ilia Mirkin -
Thomas Zimmermann