[PATCH 1/7] drm/hisilicon: Don't set drm_device->platformdev

[PATCH 1/7] drm/hisilicon: Don't set drm_device->platformdev

[Daniel Vetter]

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 {
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/7] drm: Protect master->unique with dev->master_mutex

[Daniel Vetter]

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;
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/7] drm: setclientcap doesn't need the drm BKL

[Daniel Vetter]

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),
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] drm: Enforce BKL-less ioctls for modern drivers

[Daniel Vetter]

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 {
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/7] drm: Don't compute obj counts expensively in get_resources

[Daniel Vetter]

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;
 
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/7] drm: Don't walk fb list twice in getresources

[Daniel Vetter]

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);
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/7] drm: Resurrect atomic rmfb code

[Daniel Vetter]

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-email-daniel.vetter@ffwll.ch
---
 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm: Protect master->unique with dev->master_mutex

[Emil Velikov]

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_mutex
>
> Cc: 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/7] drm/hisilicon: Don't set drm_device->platformdev

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/hisilicon: Don't set drm_device->platformdev
URL   : https://patchwork.freedesktop.org/series/16619/
State : failure

== Summary ==

Series 16619v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16619/revisions/1/mbox/

Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-snb-2600)
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-bxt-t5700)
                pass       -> DMESG-WARN (fi-byt-n2820)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-hsw-4770)
                pass       -> DMESG-WARN (fi-snb-2520m)
        Subgroup basic-reload-final:
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-skl-6700hq)
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> INCOMPLETE (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> INCOMPLETE (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-snb-2600)
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-bxt-t5700)
                pass       -> DMESG-WARN (fi-byt-n2820)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-hsw-4770)
                pass       -> INCOMPLETE (fi-snb-2520m)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-skl-6260u)
Test kms_busy:
        Subgroup basic-flip-default-a:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup basic-flip-default-b:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup basic-flip-default-c:
                pass       -> DMESG-WARN (fi-skl-6260u)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup basic-busy-flip-before-cursor-varying-size:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup basic-flip-after-cursor-legacy:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup basic-flip-after-cursor-varying-size:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup basic-flip-before-cursor-legacy:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup basic-flip-before-cursor-varying-size:
                pass       -> DMESG-WARN (fi-skl-6260u)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (fi-skl-6260u)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-skl-6260u)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup hang-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup hang-read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup nonblocking-crc-pipe-a:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup nonblocking-crc-pipe-b:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup nonblocking-crc-pipe-c:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-skl-6260u)
        Subgroup read-crc-pipe-a-frame-sequence:
WARNING: Long output truncated

de43f4e755c6bf50ad53b4ccacedf9850f42eda4 drm-tip: 2016y-12m-09d-09h-01m-58s UTC integration manifest
75df55d drm: Resurrect atomic rmfb code
668b003 drm: Don't walk fb list twice in getresources
c33deea drm: Don't compute obj counts expensively in get_resources
13d0415 drm: Enforce BKL-less ioctls for modern drivers
561de8e drm: setclientcap doesn't need the drm BKL
c0b4d4a drm: Protect master->unique with dev->master_mutex
d94d6b7 drm/hisilicon: Don't set drm_device->platformdev

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3253/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm: Protect master->unique with dev->master_mutex

[Daniel Vetter]

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_mutex
> >
> > Cc: 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/hisilicon: Don't set drm_device->platformdev

[Sean Paul]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: ✗ Fi.CI.BAT: failure for series starting with [1/7] drm/hisilicon: Don't set drm_device->platformdev

[Daniel Vetter]

On Fri, Dec 09, 2016 at 03:27:30PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/7] drm/hisilicon: Don't set drm_device->platformdev
> URL   : https://patchwork.freedesktop.org/series/16619/
> State : failure
> 
> == Summary ==
> 
> Series 16619v1 Series without cover letter
> https://patchwork.freedesktop.org/api/1.0/series/16619/revisions/1/mbox/

Aka atomic rmfb broke the world, I guess it's time to give up on that idea
... I've confirmed that by throwing the series minus the last patch at
trybot, and it's happy if I leave out the atomic rmfb.
-Daniel

> 
> Test drv_module_reload:
>         Subgroup basic-reload:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>                 pass       -> DMESG-WARN (fi-ivb-3770)
>                 pass       -> DMESG-WARN (fi-hsw-4770r)
>                 pass       -> DMESG-WARN (fi-skl-6770hq)
>                 pass       -> DMESG-WARN (fi-ilk-650)
>                 pass       -> DMESG-WARN (fi-kbl-7500u)
>                 pass       -> DMESG-WARN (fi-bsw-n3050)
>                 pass       -> DMESG-WARN (fi-snb-2600)
>                 pass       -> DMESG-WARN (fi-byt-j1900)
>                 pass       -> DMESG-WARN (fi-skl-6700hq)
>                 pass       -> DMESG-WARN (fi-ivb-3520m)
>                 pass       -> DMESG-WARN (fi-bxt-t5700)
>                 pass       -> DMESG-WARN (fi-byt-n2820)
>                 pass       -> DMESG-WARN (fi-bdw-5557u)
>                 pass       -> DMESG-WARN (fi-hsw-4770)
>                 pass       -> DMESG-WARN (fi-snb-2520m)
>         Subgroup basic-reload-final:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>                 pass       -> DMESG-WARN (fi-skl-6700hq)
>         Subgroup basic-reload-inject:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>                 pass       -> DMESG-WARN (fi-ivb-3770)
>                 pass       -> DMESG-WARN (fi-hsw-4770r)
>                 pass       -> INCOMPLETE (fi-skl-6770hq)
>                 pass       -> DMESG-WARN (fi-ilk-650)
>                 pass       -> INCOMPLETE (fi-kbl-7500u)
>                 pass       -> DMESG-WARN (fi-bsw-n3050)
>                 pass       -> DMESG-WARN (fi-snb-2600)
>                 pass       -> DMESG-WARN (fi-byt-j1900)
>                 pass       -> DMESG-WARN (fi-skl-6700hq)
>                 pass       -> DMESG-WARN (fi-ivb-3520m)
>                 pass       -> DMESG-WARN (fi-bxt-t5700)
>                 pass       -> DMESG-WARN (fi-byt-n2820)
>                 pass       -> DMESG-WARN (fi-bdw-5557u)
>                 pass       -> DMESG-WARN (fi-hsw-4770)
>                 pass       -> INCOMPLETE (fi-snb-2520m)
> Test gem_exec_suspend:
>         Subgroup basic-s3:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup basic-s4-devices:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
> Test kms_busy:
>         Subgroup basic-flip-default-a:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup basic-flip-default-b:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup basic-flip-default-c:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
> Test kms_cursor_legacy:
>         Subgroup basic-busy-flip-before-cursor-legacy:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup basic-busy-flip-before-cursor-varying-size:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup basic-flip-after-cursor-legacy:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup basic-flip-after-cursor-varying-size:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup basic-flip-before-cursor-legacy:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup basic-flip-before-cursor-varying-size:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
> Test kms_flip:
>         Subgroup basic-flip-vs-dpms:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup basic-flip-vs-modeset:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup basic-flip-vs-wf_vblank:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup basic-plain-flip:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
> Test kms_frontbuffer_tracking:
>         Subgroup basic:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
> Test kms_pipe_crc_basic:
>         Subgroup hang-read-crc-pipe-a:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup hang-read-crc-pipe-b:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup hang-read-crc-pipe-c:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup nonblocking-crc-pipe-a:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup nonblocking-crc-pipe-a-frame-sequence:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup nonblocking-crc-pipe-b:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup nonblocking-crc-pipe-b-frame-sequence:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup nonblocking-crc-pipe-c:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup nonblocking-crc-pipe-c-frame-sequence:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup read-crc-pipe-a:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
>         Subgroup read-crc-pipe-a-frame-sequence:
> WARNING: Long output truncated
> 
> de43f4e755c6bf50ad53b4ccacedf9850f42eda4 drm-tip: 2016y-12m-09d-09h-01m-58s UTC integration manifest
> 75df55d drm: Resurrect atomic rmfb code
> 668b003 drm: Don't walk fb list twice in getresources
> c33deea drm: Don't compute obj counts expensively in get_resources
> 13d0415 drm: Enforce BKL-less ioctls for modern drivers
> 561de8e drm: setclientcap doesn't need the drm BKL
> c0b4d4a drm: Protect master->unique with dev->master_mutex
> d94d6b7 drm/hisilicon: Don't set drm_device->platformdev
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3253/

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm: Resurrect atomic rmfb code

[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-email-daniel.vetter@ffwll.ch

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm: Don't compute obj counts expensively in get_resources

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm: Don't walk fb list twice in getresources

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm: Don't compute obj counts expensively in get_resources

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/7] drm: Don't walk fb list twice in getresources

[Daniel Vetter]

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

Re: [PATCH 7/7] drm: Resurrect atomic rmfb code

[Maarten Lankhorst]

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-email-daniel.vetter@ffwll.ch
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/7] drm: Resurrect atomic rmfb code

[Daniel Vetter]

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

Re: [PATCH 1/7] drm/hisilicon: Don't set drm_device->platformdev

[Daniel Vetter]

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
> >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx