It's deprecated and only should be used by drivers which still use drm_platform_init, not by anyone else.
And indeed it's entirely unused and can be nuked.
This required a bit more fudging, but I guess kirin_dc_ops really wants to operate on the platform_device, not something else. Also bonus points for implementing abstraction, and then storing the vfunc in a global variable.
v2: Don't break the build soooo badly :( Note that the cleanup function is a bit confused: ade_data was never set as drvdata, and calling drm_crtc_cleanup directly is a bug - this is called indirectly through drm_mode_config_cleanup, which calls into crtc->destroy, which already has the call to drm_crtc_cleanup. Which means we can just nuke it.
Note this is the 2nd attempt after the first one failed and had to be reverted again in
commit 9cd2e854d61ccfa51686f3ed7b0c917708fc641f Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Wed Aug 17 13:59:40 2016 +0200
Revert "drm/hisilicon: Don't set drm_device->platformdev"
Cc: Xinliang Liu xinliang.liu@linaro.org Cc: Xinwei Kong kong.kongxinwei@hisilicon.com Cc: Archit Taneja architt@codeaurora.org Cc: Sean Paul seanpaul@chromium.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 11 +++-------- drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 8 +++----- drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h | 4 ++-- 3 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c index afc2b5d2d5f0..3ea70459b901 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c @@ -973,9 +973,9 @@ static int ade_dts_parse(struct platform_device *pdev, struct ade_hw_ctx *ctx) return 0; }
-static int ade_drm_init(struct drm_device *dev) +static int ade_drm_init(struct platform_device *pdev) { - struct platform_device *pdev = dev->platformdev; + struct drm_device *dev = platform_get_drvdata(pdev); struct ade_data *ade; struct ade_hw_ctx *ctx; struct ade_crtc *acrtc; @@ -1034,13 +1034,8 @@ static int ade_drm_init(struct drm_device *dev) return 0; }
-static void ade_drm_cleanup(struct drm_device *dev) +static void ade_drm_cleanup(struct platform_device *pdev) { - struct platform_device *pdev = dev->platformdev; - struct ade_data *ade = platform_get_drvdata(pdev); - struct drm_crtc *crtc = &ade->acrtc.base; - - drm_crtc_cleanup(crtc); }
const struct kirin_dc_ops ade_dc_ops = { diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c index ebd5f4fe4c23..fa228b7b022c 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c @@ -42,7 +42,7 @@ static int kirin_drm_kms_cleanup(struct drm_device *dev) #endif drm_kms_helper_poll_fini(dev); drm_vblank_cleanup(dev); - dc_ops->cleanup(dev); + dc_ops->cleanup(to_platform_device(dev->dev)); drm_mode_config_cleanup(dev); devm_kfree(dev->dev, priv); dev->dev_private = NULL; @@ -104,7 +104,7 @@ static int kirin_drm_kms_init(struct drm_device *dev) kirin_drm_mode_config_init(dev);
/* display controller init */ - ret = dc_ops->init(dev); + ret = dc_ops->init(to_platform_device(dev->dev)); if (ret) goto err_mode_config_cleanup;
@@ -138,7 +138,7 @@ static int kirin_drm_kms_init(struct drm_device *dev) err_unbind_all: component_unbind_all(dev->dev, dev); err_dc_cleanup: - dc_ops->cleanup(dev); + dc_ops->cleanup(to_platform_device(dev->dev)); err_mode_config_cleanup: drm_mode_config_cleanup(dev); devm_kfree(dev->dev, priv); @@ -209,8 +209,6 @@ static int kirin_drm_bind(struct device *dev) if (IS_ERR(drm_dev)) return PTR_ERR(drm_dev);
- drm_dev->platformdev = to_platform_device(dev); - ret = kirin_drm_kms_init(drm_dev); if (ret) goto err_drm_dev_unref; diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h index 1a07caf8e7f4..a0bb217c4c64 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h @@ -15,8 +15,8 @@
/* display controller init/cleanup ops */ struct kirin_dc_ops { - int (*init)(struct drm_device *dev); - void (*cleanup)(struct drm_device *dev); + int (*init)(struct platform_device *pdev); + void (*cleanup)(struct platform_device *pdev); };
struct kirin_drm_private {
No one looks at the major/minor versions except the unique/busid stuff. If we protect that with the master_mutex (since it also affects the unique of each master, oh well) we can mark these two IOCTL with DRM_UNLOCKED.
While doing this I realized that the comment for the magic_map is outdated, I've forgotten to update it in:
commit d2b34ee62b409a03c6fe43c07b779983be51d017 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 17 09:33:21 2016 +0200
drm: Protect authmagic with master_mutex
Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Emil Velikov emil.l.velikov@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_ioctl.c | 12 +++++++++--- include/drm/drm_auth.h | 17 +++++++++++++---- 2 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 17e941a533bd..42a17ea5d801 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -115,11 +115,15 @@ static int drm_getunique(struct drm_device *dev, void *data, struct drm_unique *u = data; struct drm_master *master = file_priv->master;
+ mutex_lock(&master->dev->master_mutex); if (u->unique_len >= master->unique_len) { - if (copy_to_user(u->unique, master->unique, master->unique_len)) + if (copy_to_user(u->unique, master->unique, master->unique_len)) { + mutex_unlock(&master->dev->master_mutex); return -EFAULT; + } } u->unique_len = master->unique_len; + mutex_unlock(&master->dev->master_mutex);
return 0; } @@ -340,6 +344,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f struct drm_set_version *sv = data; int if_version, retcode = 0;
+ mutex_lock(&dev->master_mutex); if (sv->drm_di_major != -1) { if (sv->drm_di_major != DRM_IF_MAJOR || sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) { @@ -374,6 +379,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f sv->drm_di_minor = DRM_IF_MINOR; sv->drm_dd_major = dev->driver->major; sv->drm_dd_minor = dev->driver->minor; + mutex_unlock(&dev->master_mutex);
return retcode; } @@ -528,7 +534,7 @@ EXPORT_SYMBOL(drm_ioctl_permit); static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version, DRM_UNLOCKED|DRM_RENDER_ALLOW|DRM_CONTROL_ALLOW), - DRM_IOCTL_DEF(DRM_IOCTL_GET_UNIQUE, drm_getunique, 0), + DRM_IOCTL_DEF(DRM_IOCTL_GET_UNIQUE, drm_getunique, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_GET_MAGIC, drm_getmagic, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_IRQ_BUSID, drm_irq_by_busid, DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF(DRM_IOCTL_GET_MAP, drm_legacy_getmap_ioctl, DRM_UNLOCKED), @@ -536,7 +542,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_GET_STATS, drm_getstats, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_GET_CAP, drm_getcap, DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, 0), - DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_MASTER), + DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_UNLOCKED | DRM_MASTER),
DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF(DRM_IOCTL_BLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h index 610223b0481b..155588eb8ccf 100644 --- a/include/drm/drm_auth.h +++ b/include/drm/drm_auth.h @@ -33,10 +33,7 @@ * * @refcount: Refcount for this master object. * @dev: Link back to the DRM device - * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex. - * @unique_len: Length of unique field. Protected by drm_global_mutex. - * @magic_map: Map of used authentication tokens. Protected by struct_mutex. - * @lock: DRI lock information. + * @lock: DRI1 lock information. * @driver_priv: Pointer to driver-private information. * * Note that master structures are only relevant for the legacy/primary device @@ -45,8 +42,20 @@ struct drm_master { struct kref refcount; struct drm_device *dev; + /** + * @unique: Unique identifier: e.g. busid. Protected by struct + * &drm_device master_mutex. + */ char *unique; + /** + * @unique_len: Length of unique field. Protected by struct &drm_device + * master_mutex. + */ int unique_len; + /** + * @magic_map: Map of used authentication tokens. Protected by struct + * &drm_device master_mutex. + */ struct idr magic_map; struct drm_lock_data lock; void *driver_priv;
On 9 December 2016 at 14:19, Daniel Vetter daniel.vetter@ffwll.ch wrote:
No one looks at the major/minor versions except the unique/busid stuff. If we protect that with the master_mutex (since it also affects the unique of each master, oh well) we can mark these two IOCTL with DRM_UNLOCKED.
While doing this I realized that the comment for the magic_map is outdated, I've forgotten to update it in:
commit d2b34ee62b409a03c6fe43c07b779983be51d017 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 17 09:33:21 2016 +0200
drm: Protect authmagic with master_mutexCc: Chris Wilson chris@chris-wilson.co.uk Cc: Emil Velikov emil.l.velikov@gmail.com
There's a comment/wild idea below, but the patch itself is
Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
@@ -340,6 +344,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f struct drm_set_version *sv = data; int if_version, retcode = 0;
mutex_lock(&dev->master_mutex); if (sv->drm_di_major != -1) { if (sv->drm_di_major != DRM_IF_MAJOR || sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) {@@ -374,6 +379,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f sv->drm_di_minor = DRM_IF_MINOR; sv->drm_dd_major = dev->driver->major; sv->drm_dd_minor = dev->driver->minor;
mutex_unlock(&dev->master_mutex);
Was going to shout "error paths" only to realise that we clobber the user provided storage, even on error. Realistically one could use that info, but from a _very_ quick look I did see any.
Seems like we have all the "used by KMS drivers" ioctls converted to DRM_UNLOCKED, so I'm just wondering: Is it worth, respinning things to: - annotate the legacy-only ioctls (mostly as sanity-check) and do global lock on those, dropping any DRM_UNLOCKED references. - if !legacy-only ioctl, bail out from drm_ioctl() (again, mostly a sanity check) and dropping the boiler plate from the individual ioctls. Not too sure if this one is correct though.
if (drm_core_check_feature(dev, DRIVER_MODESET)) return 0;
Thanks Emil
On Fri, Dec 09, 2016 at 02:54:34PM +0000, Emil Velikov wrote:
On 9 December 2016 at 14:19, Daniel Vetter daniel.vetter@ffwll.ch wrote:
No one looks at the major/minor versions except the unique/busid stuff. If we protect that with the master_mutex (since it also affects the unique of each master, oh well) we can mark these two IOCTL with DRM_UNLOCKED.
While doing this I realized that the comment for the magic_map is outdated, I've forgotten to update it in:
commit d2b34ee62b409a03c6fe43c07b779983be51d017 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 17 09:33:21 2016 +0200
drm: Protect authmagic with master_mutexCc: Chris Wilson chris@chris-wilson.co.uk Cc: Emil Velikov emil.l.velikov@gmail.com
There's a comment/wild idea below, but the patch itself is
Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
@@ -340,6 +344,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f struct drm_set_version *sv = data; int if_version, retcode = 0;
mutex_lock(&dev->master_mutex); if (sv->drm_di_major != -1) { if (sv->drm_di_major != DRM_IF_MAJOR || sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) {@@ -374,6 +379,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f sv->drm_di_minor = DRM_IF_MINOR; sv->drm_dd_major = dev->driver->major; sv->drm_dd_minor = dev->driver->minor;
mutex_unlock(&dev->master_mutex);Was going to shout "error paths" only to realise that we clobber the user provided storage, even on error. Realistically one could use that info, but from a _very_ quick look I did see any.
Seems like we have all the "used by KMS drivers" ioctls converted to DRM_UNLOCKED, so I'm just wondering: Is it worth, respinning things to:
- annotate the legacy-only ioctls (mostly as sanity-check) and do
global lock on those, dropping any DRM_UNLOCKED references.
Yeah, that would make sense as a follow-up after this series. There's already a patch to enforce lockless behaviour for all new ioctls. We'd need a DRM_LEGACY_LOCKED to annotate the few places that still need the drm bkl.
- if !legacy-only ioctl, bail out from drm_ioctl() (again, mostly a
sanity check) and dropping the boiler plate from the individual ioctls. Not too sure if this one is correct though.
The boiler-plate isn't the same everywhere, sometimes it checks DRIVER_LEGACY, sometimes also the nouveau special case and the return code isn't the same everywhere (I'm not sure where that all matters). My preference for this would be to leave it as is. -Daniel
if (drm_core_check_feature(dev, DRIVER_MODESET)) return 0;
Thanks Emil
It only updates per-file feature flags. And all the ioctl which change behaviour depending upon these flags (they're all kms features) do _not_ hold the BKL. Therefor this is pure cargo-cult and can be removed.
Note that there's a risk that the ioctl will behave inconsistently, but that's ok. The only thing it's not allowed to do is oops the kernel, and from an audit all places are safe.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 42a17ea5d801..e1b2c372f021 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -541,7 +541,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_GET_CLIENT, drm_getclient, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_GET_STATS, drm_getstats, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_GET_CAP, drm_getcap, DRM_UNLOCKED|DRM_RENDER_ALLOW), - DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, 0), + DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_UNLOCKED | DRM_MASTER),
DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
With the last round of changes all ioctls called by modern drivers now have their own locking. Everything else is only allowed for legacy drivers and hence the lack of locking doesn't matter.
One exception is nouveau, due to the DRIVER_KMS_LEGACY_CONTEXT flag. But that only works its magic on the context and bufs ioctls. And drm_bufs.c is protected with dev->struct_mutex, and drm_context.c by the same and dev->ctxlist_mutex. That should be all safe, and we can finally mandata drm-bkl-less ioctls for everyone!
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_ioctl.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index e1b2c372f021..e21a18ac968e 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -735,9 +735,8 @@ long drm_ioctl(struct file *filp, if (ksize > in_size) memset(kdata + in_size, 0, ksize - in_size);
- /* Enforce sane locking for modern driver ioctls. Core ioctls are - * too messy still. */ - if ((!drm_core_check_feature(dev, DRIVER_LEGACY) && is_driver_ioctl) || + /* Enforce sane locking for modern driver ioctls. */ + if (!drm_core_check_feature(dev, DRIVER_LEGACY) || (ioctl->flags & DRM_UNLOCKED)) retcode = func(dev, kdata, file_priv); else {
Looping when we keep track of this is silly. Only thing we have to be careful is with sampling the connector count. To avoid inconsisten results due to gcc re-computing this, use READ_ONCE.
And to avoid surprising userspace, make sure we don't copy more connectors than planned, and report the actual number of connectors copied. That way any racing hot-add/remove will be handled.
v2: Actually try to not blow up, somehow I lost the hunk that checks we don't copy too much. Noticed by Chris.
Cc: Chris Wilson chris@chris-wilson.co.uk Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_mode_config.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 2735a5847ffa..64c2971478db 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -90,10 +90,10 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_crtc *crtc; struct drm_encoder *encoder; int ret = 0; - int connector_count = 0; - int crtc_count = 0; + int connector_count = READ_ONCE(dev->mode_config.num_connector); + int crtc_count = dev->mode_config.num_crtc; int fb_count = 0; - int encoder_count = 0; + int encoder_count = dev->mode_config.num_encoder; int copied = 0; uint32_t __user *fb_id; uint32_t __user *crtc_id; @@ -131,15 +131,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data, /* mode_config.mutex protects the connector list against e.g. DP MST * connector hot-adding. CRTC/Plane lists are invariant. */ mutex_lock(&dev->mode_config.mutex); - drm_for_each_crtc(crtc, dev) - crtc_count++; - - drm_for_each_connector(connector, dev) - connector_count++; - - drm_for_each_encoder(encoder, dev) - encoder_count++; - card_res->max_height = dev->mode_config.max_height; card_res->min_height = dev->mode_config.min_height; card_res->max_width = dev->mode_config.max_width; @@ -179,6 +170,9 @@ int drm_mode_getresources(struct drm_device *dev, void *data, copied = 0; connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr; drm_for_each_connector(connector, dev) { + if (copied >= connector_count) + break; + if (put_user(connector->base.id, connector_id + copied)) { ret = -EFAULT; @@ -186,6 +180,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, } copied++; } + connector_count = copied; } card_res->count_connectors = connector_count;
On Fri, Dec 09, 2016 at 03:19:42PM +0100, Daniel Vetter wrote:
Looping when we keep track of this is silly. Only thing we have to be careful is with sampling the connector count. To avoid inconsisten results due to gcc re-computing this, use READ_ONCE.
Later on in the function we take the mutex that should prevent the values from changing.
If each block was like
mutex_lock(&mode_config->lockc);
count = dev->mode_config.num_crtc; if (card_res->count_crtcs >= count) { u32 __user id = u64_to_user_ptr(card_res->crtc_id_ptr); unsigned int copied = 0;
drm_for_each_crtc(crtc, dev) { if (copied >= count) break;
if (put_user(crtc->base.id, id + copied)) { ret = -EFAULT; goto out; }
copied++; }
count = copied; } card_res->count_crtcs = count;
...
mutex_unlock(&mode_config->lockc);
it would look a bit neater. -Chris
On Fri, Dec 09, 2016 at 09:47:56PM +0000, Chris Wilson wrote:
On Fri, Dec 09, 2016 at 03:19:42PM +0100, Daniel Vetter wrote:
Looping when we keep track of this is silly. Only thing we have to be careful is with sampling the connector count. To avoid inconsisten results due to gcc re-computing this, use READ_ONCE.
Later on in the function we take the mutex that should prevent the values from changing.
If each block was like
mutex_lock(&mode_config->lockc);
Well that mutex_lock is fiction too. It's not needed for any of the lists, and it definitely doesn't protect connector_list. Step-by-step I'd say ... -Daniel
count = dev->mode_config.num_crtc; if (card_res->count_crtcs >= count) { u32 __user id = u64_to_user_ptr(card_res->crtc_id_ptr); unsigned int copied = 0;
drm_for_each_crtc(crtc, dev) { if (copied >= count) break;
if (put_user(crtc->base.id, id + copied)) { ret = -EFAULT; goto out; } copied++;}
count = copied; } card_res->count_crtcs = count;
...
mutex_unlock(&mode_config->lockc);
it would look a bit neater. -Chris
-- Chris Wilson, Intel Open Source Technology Centre
We can be more clever than that and merge the 2 list walking loops. It's all under the one mutex critical section anyway, so nothing funny can happen.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_mode_config.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 64c2971478db..a6205a92dfee 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -84,7 +84,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_card_res *card_res = data; - struct list_head *lh; struct drm_framebuffer *fb; struct drm_connector *connector; struct drm_crtc *crtc; @@ -105,25 +104,20 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
mutex_lock(&file_priv->fbs_lock); - /* - * For the non-control nodes we need to limit the list of resources - * by IDs in the group list for this node - */ - list_for_each(lh, &file_priv->fbs) - fb_count++; - /* handle this in 4 parts */ /* FBs */ - if (card_res->count_fbs >= fb_count) { - copied = 0; - fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr; - list_for_each_entry(fb, &file_priv->fbs, filp_head) { - if (put_user(fb->base.id, fb_id + copied)) { - mutex_unlock(&file_priv->fbs_lock); - return -EFAULT; - } - copied++; + copied = 0; + fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr; + list_for_each_entry(fb, &file_priv->fbs, filp_head) { + fb_count++; + if (fb_count > card_res->count_fbs) + continue; + + if (put_user(fb->base.id, fb_id + copied)) { + mutex_unlock(&file_priv->fbs_lock); + return -EFAULT; } + copied++; } card_res->count_fbs = fb_count; mutex_unlock(&file_priv->fbs_lock);
On Fri, Dec 09, 2016 at 03:19:43PM +0100, Daniel Vetter wrote:
We can be more clever than that and merge the 2 list walking loops. It's all under the one mutex critical section anyway, so nothing funny can happen.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_mode_config.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 64c2971478db..a6205a92dfee 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -84,7 +84,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_card_res *card_res = data;
- struct list_head *lh; struct drm_framebuffer *fb; struct drm_connector *connector; struct drm_crtc *crtc;
@@ -105,25 +104,20 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
mutex_lock(&file_priv->fbs_lock);
- /*
* For the non-control nodes we need to limit the list of resources* by IDs in the group list for this node*/- list_for_each(lh, &file_priv->fbs)
fb_count++;- /* handle this in 4 parts */ /* FBs */
- if (card_res->count_fbs >= fb_count) {
copied = 0;fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;list_for_each_entry(fb, &file_priv->fbs, filp_head) {if (put_user(fb->base.id, fb_id + copied)) {mutex_unlock(&file_priv->fbs_lock);return -EFAULT;}copied++;
- copied = 0;
- fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
- list_for_each_entry(fb, &file_priv->fbs, filp_head) {
fb_count++;if (fb_count > card_res->count_fbs)continue;if (put_user(fb->base.id, fb_id + copied)) {mutex_unlock(&file_priv->fbs_lock);return -EFAULT;copied++;
if (fb_count < card_res->count_fbs && put_user(fb->base.id fb_id + fb_count) { } fb_count++; // no need for copied
I'd just like for one style :) Otoh, this seems reasonable to apply to crtc/encoders/connectors as well, and on the other we already have a counter for the others. Hmm, setting count = copied in the previous patch is thereby wrong (change in uABI). -Chris
On Fri, Dec 09, 2016 at 09:57:45PM +0000, Chris Wilson wrote:
On Fri, Dec 09, 2016 at 03:19:43PM +0100, Daniel Vetter wrote:
We can be more clever than that and merge the 2 list walking loops. It's all under the one mutex critical section anyway, so nothing funny can happen.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_mode_config.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 64c2971478db..a6205a92dfee 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -84,7 +84,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_card_res *card_res = data;
- struct list_head *lh; struct drm_framebuffer *fb; struct drm_connector *connector; struct drm_crtc *crtc;
@@ -105,25 +104,20 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
mutex_lock(&file_priv->fbs_lock);
- /*
* For the non-control nodes we need to limit the list of resources* by IDs in the group list for this node*/- list_for_each(lh, &file_priv->fbs)
fb_count++;- /* handle this in 4 parts */ /* FBs */
- if (card_res->count_fbs >= fb_count) {
copied = 0;fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;list_for_each_entry(fb, &file_priv->fbs, filp_head) {if (put_user(fb->base.id, fb_id + copied)) {mutex_unlock(&file_priv->fbs_lock);return -EFAULT;}copied++;
- copied = 0;
- fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
- list_for_each_entry(fb, &file_priv->fbs, filp_head) {
fb_count++;if (fb_count > card_res->count_fbs)continue;if (put_user(fb->base.id, fb_id + copied)) {mutex_unlock(&file_priv->fbs_lock);return -EFAULT;copied++;if (fb_count < card_res->count_fbs && put_user(fb->base.id fb_id + fb_count) { } fb_count++; // no need for copied
I'd just like for one style :) Otoh, this seems reasonable to apply to crtc/encoders/connectors as well, and on the other we already have a counter for the others. Hmm, setting count = copied in the previous patch is thereby wrong (change in uABI).
It's only a change if the connector count changed between when we read it and when we walked the list. But I guess you've convinced me that simplifying this loops more is a good idea, and this way we can use the same style for all of them. -Daniel
This was somehow lost between v3 and the merged version in Maarten's patch merged as:
commit f2d580b9a8149735cbc4b59c4a8df60173658140 Author: Maarten Lankhorst maarten.lankhorst@linux.intel.com Date: Wed May 4 14:38:26 2016 +0200
drm/core: Do not preserve framebuffer on rmfb, v4.
Actual code copied from Maarten's patch, but with the slight change to just use dev->mode_config.funcs->atomic_commit to decide whether to use the atomic path or not.
FIXME: This seems to break audio rpm refcounting somehow! See:
commit 0dcac5008fcf57cce66ef091204efbde86956c7a Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Thu Jul 14 15:16:34 2016 +0200
Revert "drm: Resurrect atomic rmfb code"
v2: Use new atomic state refcounting.
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch Link: http://patchwork.freedesktop.org/patch/msgid/1465388359-8070-24-git-send-ema... --- drivers/gpu/drm/drm_atomic.c | 66 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_crtc_internal.h | 1 + drivers/gpu/drm/drm_framebuffer.c | 6 ++++ 3 files changed, 73 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 72fa5b20baf1..6cc6c0f7609a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -2009,6 +2009,72 @@ static void complete_crtc_signaling(struct drm_device *dev, kfree(fence_state); }
+int drm_atomic_remove_fb(struct drm_framebuffer *fb) +{ + struct drm_modeset_acquire_ctx ctx; + struct drm_device *dev = fb->dev; + struct drm_atomic_state *state; + struct drm_plane *plane; + int ret = 0; + unsigned plane_mask; + + state = drm_atomic_state_alloc(dev); + if (!state) + return -ENOMEM; + + drm_modeset_acquire_init(&ctx, 0); + state->acquire_ctx = &ctx; + +retry: + plane_mask = 0; + ret = drm_modeset_lock_all_ctx(dev, &ctx); + if (ret) + goto unlock; + + drm_for_each_plane(plane, dev) { + struct drm_plane_state *plane_state; + + if (plane->state->fb != fb) + continue; + + plane_state = drm_atomic_get_plane_state(state, plane); + if (IS_ERR(plane_state)) { + ret = PTR_ERR(plane_state); + goto unlock; + } + + drm_atomic_set_fb_for_plane(plane_state, NULL); + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); + if (ret) + goto unlock; + + plane_mask |= BIT(drm_plane_index(plane)); + + plane->old_fb = plane->fb; + plane->fb = NULL; + } + + if (plane_mask) + ret = drm_atomic_commit(state); + +unlock: + if (plane_mask) + drm_atomic_clean_old_fb(dev, plane_mask, ret); + + if (ret == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + } + + if (ret || !plane_mask) + drm_atomic_state_put(state); + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + + return ret; +} + int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index cdf6860c9d22..121e250853d2 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -178,6 +178,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj, struct drm_property *property, uint64_t *val); int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +int drm_atomic_remove_fb(struct drm_framebuffer *fb);
/* drm_plane.c */ diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index cbf0c893f426..533ce991d446 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -770,6 +770,11 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) * in this manner. */ if (drm_framebuffer_read_refcount(fb) > 1) { + if (dev->mode_config.funcs->atomic_commit) { + drm_atomic_remove_fb(fb); + goto out; + } + drm_modeset_lock_all(dev); /* remove from any CRTC */ drm_for_each_crtc(crtc, dev) { @@ -787,6 +792,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) drm_modeset_unlock_all(dev); }
+out: drm_framebuffer_unreference(fb); } EXPORT_SYMBOL(drm_framebuffer_remove);
On Fri, Dec 09, 2016 at 03:19:44PM +0100, Daniel Vetter wrote:
This was somehow lost between v3 and the merged version in Maarten's patch merged as:
commit f2d580b9a8149735cbc4b59c4a8df60173658140 Author: Maarten Lankhorst maarten.lankhorst@linux.intel.com Date: Wed May 4 14:38:26 2016 +0200
drm/core: Do not preserve framebuffer on rmfb, v4.Actual code copied from Maarten's patch, but with the slight change to just use dev->mode_config.funcs->atomic_commit to decide whether to use the atomic path or not.
FIXME: This seems to break audio rpm refcounting somehow! See:
commit 0dcac5008fcf57cce66ef091204efbde86956c7a Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Thu Jul 14 15:16:34 2016 +0200
Revert "drm: Resurrect atomic rmfb code"v2: Use new atomic state refcounting.
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch Link: http://patchwork.freedesktop.org/patch/msgid/1465388359-8070-24-git-send-ema...
intel-gfx CI told me this breaks the world. I guess back to the drawing board. -Daniel
drivers/gpu/drm/drm_atomic.c | 66 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_crtc_internal.h | 1 + drivers/gpu/drm/drm_framebuffer.c | 6 ++++ 3 files changed, 73 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 72fa5b20baf1..6cc6c0f7609a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -2009,6 +2009,72 @@ static void complete_crtc_signaling(struct drm_device *dev, kfree(fence_state); }
+int drm_atomic_remove_fb(struct drm_framebuffer *fb) +{
- struct drm_modeset_acquire_ctx ctx;
- struct drm_device *dev = fb->dev;
- struct drm_atomic_state *state;
- struct drm_plane *plane;
- int ret = 0;
- unsigned plane_mask;
- state = drm_atomic_state_alloc(dev);
- if (!state)
return -ENOMEM;- drm_modeset_acquire_init(&ctx, 0);
- state->acquire_ctx = &ctx;
+retry:
- plane_mask = 0;
- ret = drm_modeset_lock_all_ctx(dev, &ctx);
- if (ret)
goto unlock;- drm_for_each_plane(plane, dev) {
struct drm_plane_state *plane_state;if (plane->state->fb != fb)continue;plane_state = drm_atomic_get_plane_state(state, plane);if (IS_ERR(plane_state)) {ret = PTR_ERR(plane_state);goto unlock;}drm_atomic_set_fb_for_plane(plane_state, NULL);ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);if (ret)goto unlock;plane_mask |= BIT(drm_plane_index(plane));plane->old_fb = plane->fb;plane->fb = NULL;- }
- if (plane_mask)
ret = drm_atomic_commit(state);+unlock:
- if (plane_mask)
drm_atomic_clean_old_fb(dev, plane_mask, ret);- if (ret == -EDEADLK) {
drm_modeset_backoff(&ctx);goto retry;- }
- if (ret || !plane_mask)
drm_atomic_state_put(state);- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
- return ret;
+}
int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index cdf6860c9d22..121e250853d2 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -178,6 +178,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj, struct drm_property *property, uint64_t *val); int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +int drm_atomic_remove_fb(struct drm_framebuffer *fb);
/* drm_plane.c */ diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index cbf0c893f426..533ce991d446 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -770,6 +770,11 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) * in this manner. */ if (drm_framebuffer_read_refcount(fb) > 1) {
if (dev->mode_config.funcs->atomic_commit) {drm_atomic_remove_fb(fb);goto out;}- drm_modeset_lock_all(dev); /* remove from any CRTC */ drm_for_each_crtc(crtc, dev) {
@@ -787,6 +792,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) drm_modeset_unlock_all(dev); }
+out: drm_framebuffer_unreference(fb); } EXPORT_SYMBOL(drm_framebuffer_remove); -- 2.10.2
Op 09-12-16 om 21:59 schreef Daniel Vetter:
On Fri, Dec 09, 2016 at 03:19:44PM +0100, Daniel Vetter wrote:
This was somehow lost between v3 and the merged version in Maarten's patch merged as:
commit f2d580b9a8149735cbc4b59c4a8df60173658140 Author: Maarten Lankhorst maarten.lankhorst@linux.intel.com Date: Wed May 4 14:38:26 2016 +0200
drm/core: Do not preserve framebuffer on rmfb, v4.Actual code copied from Maarten's patch, but with the slight change to just use dev->mode_config.funcs->atomic_commit to decide whether to use the atomic path or not.
FIXME: This seems to break audio rpm refcounting somehow! See:
commit 0dcac5008fcf57cce66ef091204efbde86956c7a Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Thu Jul 14 15:16:34 2016 +0200
Revert "drm: Resurrect atomic rmfb code"v2: Use new atomic state refcounting.
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch Link: http://patchwork.freedesktop.org/patch/msgid/1465388359-8070-24-git-send-ema...
intel-gfx CI told me this breaks the world. I guess back to the drawing board.
What happens when we add an argument to __drm_framebuffer_remove whether crtc has to be disabled or not, and make drm_framebuffer_remove always say it has to disable crtc, while being called from rmfb_work_fn tries to keep crtc enabled?
~Maarten
On Mon, Dec 12, 2016 at 09:46:59AM +0100, Maarten Lankhorst wrote:
Op 09-12-16 om 21:59 schreef Daniel Vetter:
On Fri, Dec 09, 2016 at 03:19:44PM +0100, Daniel Vetter wrote:
This was somehow lost between v3 and the merged version in Maarten's patch merged as:
commit f2d580b9a8149735cbc4b59c4a8df60173658140 Author: Maarten Lankhorst maarten.lankhorst@linux.intel.com Date: Wed May 4 14:38:26 2016 +0200
drm/core: Do not preserve framebuffer on rmfb, v4.Actual code copied from Maarten's patch, but with the slight change to just use dev->mode_config.funcs->atomic_commit to decide whether to use the atomic path or not.
FIXME: This seems to break audio rpm refcounting somehow! See:
commit 0dcac5008fcf57cce66ef091204efbde86956c7a Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Thu Jul 14 15:16:34 2016 +0200
Revert "drm: Resurrect atomic rmfb code"v2: Use new atomic state refcounting.
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch Link: http://patchwork.freedesktop.org/patch/msgid/1465388359-8070-24-git-send-ema...
intel-gfx CI told me this breaks the world. I guess back to the drawing board.
What happens when we add an argument to __drm_framebuffer_remove whether crtc has to be disabled or not, and make drm_framebuffer_remove always say it has to disable crtc, while being called from rmfb_work_fn tries to keep crtc enabled?
tbh I'm not sure what the regression this time around was. But I guess long-term we want some kind of atomic rmfb, since iirc it blocks the re-adding of ww_acquire_done? I've thrown this patch out of my pile for now, got burned too often ;-) -Daniel
On Fri, Dec 9, 2016 at 9:19 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
It's deprecated and only should be used by drivers which still use drm_platform_init, not by anyone else.
And indeed it's entirely unused and can be nuked.
This required a bit more fudging, but I guess kirin_dc_ops really wants to operate on the platform_device, not something else. Also bonus points for implementing abstraction, and then storing the vfunc in a global variable.
v2: Don't break the build soooo badly :( Note that the cleanup function is a bit confused: ade_data was never set as drvdata, and calling drm_crtc_cleanup directly is a bug - this is called indirectly through drm_mode_config_cleanup, which calls into crtc->destroy, which already has the call to drm_crtc_cleanup. Which means we can just nuke it.
Note this is the 2nd attempt after the first one failed and had to be reverted again in
commit 9cd2e854d61ccfa51686f3ed7b0c917708fc641f Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Wed Aug 17 13:59:40 2016 +0200
Revert "drm/hisilicon: Don't set drm_device->platformdev"Cc: Xinliang Liu xinliang.liu@linaro.org Cc: Xinwei Kong kong.kongxinwei@hisilicon.com Cc: Archit Taneja architt@codeaurora.org Cc: Sean Paul seanpaul@chromium.org
This time with feeling!
Reviewed-by: Sean Paul seanpaul@chromium.org
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 11 +++-------- drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 8 +++----- drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h | 4 ++-- 3 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c index afc2b5d2d5f0..3ea70459b901 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c @@ -973,9 +973,9 @@ static int ade_dts_parse(struct platform_device *pdev, struct ade_hw_ctx *ctx) return 0; }
-static int ade_drm_init(struct drm_device *dev) +static int ade_drm_init(struct platform_device *pdev) {
struct platform_device *pdev = dev->platformdev;
struct drm_device *dev = platform_get_drvdata(pdev); struct ade_data *ade; struct ade_hw_ctx *ctx; struct ade_crtc *acrtc;@@ -1034,13 +1034,8 @@ static int ade_drm_init(struct drm_device *dev) return 0; }
-static void ade_drm_cleanup(struct drm_device *dev) +static void ade_drm_cleanup(struct platform_device *pdev) {
struct platform_device *pdev = dev->platformdev;struct ade_data *ade = platform_get_drvdata(pdev);struct drm_crtc *crtc = &ade->acrtc.base;drm_crtc_cleanup(crtc);}
const struct kirin_dc_ops ade_dc_ops = { diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c index ebd5f4fe4c23..fa228b7b022c 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c @@ -42,7 +42,7 @@ static int kirin_drm_kms_cleanup(struct drm_device *dev) #endif drm_kms_helper_poll_fini(dev); drm_vblank_cleanup(dev);
dc_ops->cleanup(dev);
dc_ops->cleanup(to_platform_device(dev->dev)); drm_mode_config_cleanup(dev); devm_kfree(dev->dev, priv); dev->dev_private = NULL;@@ -104,7 +104,7 @@ static int kirin_drm_kms_init(struct drm_device *dev) kirin_drm_mode_config_init(dev);
/* display controller init */
ret = dc_ops->init(dev);
ret = dc_ops->init(to_platform_device(dev->dev)); if (ret) goto err_mode_config_cleanup;@@ -138,7 +138,7 @@ static int kirin_drm_kms_init(struct drm_device *dev) err_unbind_all: component_unbind_all(dev->dev, dev); err_dc_cleanup:
dc_ops->cleanup(dev);
dc_ops->cleanup(to_platform_device(dev->dev));err_mode_config_cleanup: drm_mode_config_cleanup(dev); devm_kfree(dev->dev, priv); @@ -209,8 +209,6 @@ static int kirin_drm_bind(struct device *dev) if (IS_ERR(drm_dev)) return PTR_ERR(drm_dev);
drm_dev->platformdev = to_platform_device(dev);ret = kirin_drm_kms_init(drm_dev); if (ret) goto err_drm_dev_unref;diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h index 1a07caf8e7f4..a0bb217c4c64 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h @@ -15,8 +15,8 @@
/* display controller init/cleanup ops */ struct kirin_dc_ops {
int (*init)(struct drm_device *dev);void (*cleanup)(struct drm_device *dev);
int (*init)(struct platform_device *pdev);void (*cleanup)(struct platform_device *pdev);};
struct kirin_drm_private {
2.10.2
On Fri, Dec 09, 2016 at 01:04:12PM -0500, Sean Paul wrote:
On Fri, Dec 9, 2016 at 9:19 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
It's deprecated and only should be used by drivers which still use drm_platform_init, not by anyone else.
And indeed it's entirely unused and can be nuked.
This required a bit more fudging, but I guess kirin_dc_ops really wants to operate on the platform_device, not something else. Also bonus points for implementing abstraction, and then storing the vfunc in a global variable.
v2: Don't break the build soooo badly :( Note that the cleanup function is a bit confused: ade_data was never set as drvdata, and calling drm_crtc_cleanup directly is a bug - this is called indirectly through drm_mode_config_cleanup, which calls into crtc->destroy, which already has the call to drm_crtc_cleanup. Which means we can just nuke it.
Note this is the 2nd attempt after the first one failed and had to be reverted again in
commit 9cd2e854d61ccfa51686f3ed7b0c917708fc641f Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Wed Aug 17 13:59:40 2016 +0200
Revert "drm/hisilicon: Don't set drm_device->platformdev"Cc: Xinliang Liu xinliang.liu@linaro.org Cc: Xinwei Kong kong.kongxinwei@hisilicon.com Cc: Archit Taneja architt@codeaurora.org Cc: Sean Paul seanpaul@chromium.org
This time with feeling!
Reviewed-by: Sean Paul seanpaul@chromium.org
Thanks for the review, applied to drm-misc (but for 4.11). -Daniel
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 11 +++-------- drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 8 +++----- drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h | 4 ++-- 3 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c index afc2b5d2d5f0..3ea70459b901 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c @@ -973,9 +973,9 @@ static int ade_dts_parse(struct platform_device *pdev, struct ade_hw_ctx *ctx) return 0; }
-static int ade_drm_init(struct drm_device *dev) +static int ade_drm_init(struct platform_device *pdev) {
struct platform_device *pdev = dev->platformdev;
struct drm_device *dev = platform_get_drvdata(pdev); struct ade_data *ade; struct ade_hw_ctx *ctx; struct ade_crtc *acrtc;@@ -1034,13 +1034,8 @@ static int ade_drm_init(struct drm_device *dev) return 0; }
-static void ade_drm_cleanup(struct drm_device *dev) +static void ade_drm_cleanup(struct platform_device *pdev) {
struct platform_device *pdev = dev->platformdev;struct ade_data *ade = platform_get_drvdata(pdev);struct drm_crtc *crtc = &ade->acrtc.base;drm_crtc_cleanup(crtc);}
const struct kirin_dc_ops ade_dc_ops = { diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c index ebd5f4fe4c23..fa228b7b022c 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c @@ -42,7 +42,7 @@ static int kirin_drm_kms_cleanup(struct drm_device *dev) #endif drm_kms_helper_poll_fini(dev); drm_vblank_cleanup(dev);
dc_ops->cleanup(dev);
dc_ops->cleanup(to_platform_device(dev->dev)); drm_mode_config_cleanup(dev); devm_kfree(dev->dev, priv); dev->dev_private = NULL;@@ -104,7 +104,7 @@ static int kirin_drm_kms_init(struct drm_device *dev) kirin_drm_mode_config_init(dev);
/* display controller init */
ret = dc_ops->init(dev);
ret = dc_ops->init(to_platform_device(dev->dev)); if (ret) goto err_mode_config_cleanup;@@ -138,7 +138,7 @@ static int kirin_drm_kms_init(struct drm_device *dev) err_unbind_all: component_unbind_all(dev->dev, dev); err_dc_cleanup:
dc_ops->cleanup(dev);
dc_ops->cleanup(to_platform_device(dev->dev));err_mode_config_cleanup: drm_mode_config_cleanup(dev); devm_kfree(dev->dev, priv); @@ -209,8 +209,6 @@ static int kirin_drm_bind(struct device *dev) if (IS_ERR(drm_dev)) return PTR_ERR(drm_dev);
drm_dev->platformdev = to_platform_device(dev);ret = kirin_drm_kms_init(drm_dev); if (ret) goto err_drm_dev_unref;diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h index 1a07caf8e7f4..a0bb217c4c64 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h @@ -15,8 +15,8 @@
/* display controller init/cleanup ops */ struct kirin_dc_ops {
int (*init)(struct drm_device *dev);void (*cleanup)(struct drm_device *dev);
int (*init)(struct platform_device *pdev);void (*cleanup)(struct platform_device *pdev);};
struct kirin_drm_private {
2.10.2
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Daniel Vetter -
Emil Velikov -
Maarten Lankhorst -
Sean Paul