[Intel-gfx] [PATCH] backlight: Avoid double fbcon backlight handling

[Chris Wilson]

On Thu, Aug 04, 2016 at 11:50:27AM +0200, Daniel Vetter wrote:
> On Thu, Aug 04, 2016 at 12:02:23PM +0300, Jani Nikula wrote:
> > On Tue, 12 Jul 2016, Daniel Vetter  wrote:
> > > On Thu, Jun 30, 2016 at 12:30:56PM +0100, Chris Wilson wrote:
> > >> Backlights controlled by i915.ko and only associated with its connectors
> > >> and also only associated with the intel_drmfb fbcon, controlled by
> > >> i915.ko. In this situation, we already handle adjusting the backlight
> > >> when the fbcon is blanked/unblanked and do not require backlight trying
> > >> to do the same.
> > >> 
> > >> Attempting to register with the fbdev as a client causes lockdep to warn
> > >> about a dependency cycle:
> > >
> > > The fbdev notifier strikes again!
> > >
> > > Last time I looked into this I think the proper solution would be to split
> > > the backlight part from the other fbdev notifier (which is used by fbcon
> > > for reacting to fbdev device reg/unreg events).
> > >
> > > I think that would fix this too, with the added bonus of slightly
> > > untangling the fbcon locking mess. And it's also the one part of
> > > untangling this mess which should be possible without any trouble - I've
> > > simply never done it since entirely getting rid of the fbdev notifier for
> > > fbcon is a lot more work.
> > 
> > So what do we do with this? It fixes a problem upstream. There's no
> > Fixes: to identify the bad commit. Any idea on that? It's either this or
> > we dig out the bad commit (Chris probably knows which one?) and revert.
> 
> The real trouble is the drm_for_each_connector in
> drm_connector_register_all(). This introduced the new depency. The proper
> fix imo is to fix up the connector_list locking, but for 4.8 we could do
> the same hack+comment like we do in unregister_all. It's not the only
> place that's broken anyway, and much less invasive than this here.

You still have the underlying issue of multiple drivers trying to
control the same piece of hardware, causing duplicate work (at best).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v5 4/5] Documentation: add doc for sync_file_get_fence()

[Chris Wilson]

On Thu, Aug 04, 2016 at 11:24:13PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Document the new function added to sync_file.c
> 
> v2: Adapt to fence_array
> 
> Signed-off-by: Gustavo Padovan 
> Acked-by: Christian König 
> ---
>  Documentation/sync_file.txt | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/sync_file.txt b/Documentation/sync_file.txt
> index e8e2eba..ae2dbad1 100644
> --- a/Documentation/sync_file.txt
> +++ b/Documentation/sync_file.txt
> @@ -64,6 +64,21 @@ The sync_file fd now can be sent to userspace.
>  If the creation process fail, or the sync_file needs to be released by any
>  other reason fput(sync_file->file) should be used.
>  
> +Receiving Sync Files from Userspace
> +---
> +
> +When userspace needs to send an in-fence to the driver it pass file 
> descriptor

s/pass/passes/

> +of the Sync File to the kernel. The kernel then can retrieve the fences

/then can/can then/

> +from it.
> +
> +Interface:
> + struct fence *sync_file_get_fence(int fd);
> +
> +
> +The function return a struct fence pointer referencing the fence(s) in the 
> Sync
> +File.

+ The returned reference is owned by the caller and must be disposed of
afterwards using fence_put(). In case of error, a NULL is returned
instead.

Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v5 5/5] dma-buf/sync_file: only enable fence signalling on poll()

[Chris Wilson]

On Thu, Aug 04, 2016 at 11:24:14PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Signalling doesn't need to be enabled at sync_file creation, it is only
> required if userspace waiting the fence to signal through poll().
> 
> Thus we delay fence_add_callback() until poll is called. It only adds the
> callback the first time poll() is called. This avoid re-adding the same
> callback multiple times.
> 
> v2: rebase and update to work with new fence support for sync_file
> 
> v3: use atomic operation to set enabled and protect fence_add_callback()
> 
> v4: use user bit from fence flags (comment from Chris Wilson)
> 
> Signed-off-by: Gustavo Padovan 
> ---
> + if (!test_and_set_bit(POLL_ENABLED, _file->fence->flags)) {
> + if (fence_add_callback(sync_file->fence, _file->cb,
> +fence_check_cb_func) < 0)
> + wake_up_all(_file->wq);
> + }
> +
>   status = fence_is_signaled(sync_file->fence);

status can only be true/false here.
return fence_is_signaled(sync_file->fence) ? POLLIN : 0;

Reviwed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v5 5/5] dma-buf/sync_file: only enable fence signalling on poll()

[Chris Wilson]

On Fri, Aug 05, 2016 at 08:28:15AM +0100, Chris Wilson wrote:
> On Thu, Aug 04, 2016 at 11:24:14PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > Signalling doesn't need to be enabled at sync_file creation, it is only
> > required if userspace waiting the fence to signal through poll().
> > 
> > Thus we delay fence_add_callback() until poll is called. It only adds the
> > callback the first time poll() is called. This avoid re-adding the same
> > callback multiple times.
> > 
> > v2: rebase and update to work with new fence support for sync_file
> > 
> > v3: use atomic operation to set enabled and protect fence_add_callback()
> > 
> > v4: use user bit from fence flags (comment from Chris Wilson)
> > 
> > Signed-off-by: Gustavo Padovan 
> > ---
> > +   if (!test_and_set_bit(POLL_ENABLED, _file->fence->flags)) {
> > +   if (fence_add_callback(sync_file->fence, _file->cb,
> > +  fence_check_cb_func) < 0)
> > +   wake_up_all(_file->wq);
> > +   }
> > +
> > status = fence_is_signaled(sync_file->fence);
> 
> status can only be true/false here.
>   return fence_is_signaled(sync_file->fence) ? POLLIN : 0;
> 
> Reviwed-by: Chris Wilson 

Reviewed-by

/me goes back to drinking his coffee
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Chris Wilson]

On Fri, Aug 05, 2016 at 10:06:08AM +0200, Daniel Vetter wrote:
> On Fri, Aug 05, 2016 at 12:01:25AM +0100, Russell King - ARM Linux wrote:
> > On Thu, Aug 04, 2016 at 06:13:18PM +0100, Jose Abreu wrote:
> > > Hi Russell,
> > > 
> > > So, I didn't use framebuffer console but used X instead and it is
> > > working as it should. I think we can drop this patch. I am now
> > > making interoperability with DVI and I am facing the following
> > > scenario:
> > > - I start the driver
> > > - An EDID is sent which tells the driver that HDMI is NOT
> > > supported;
> > > - The driver configures itself to a DVI mode;
> > > 
> > > Until this point everything is working as it should. But:
> > > 
> > > - Now I send an EDID which tells the driver that HDMI is
> > > supported;
> > > - As the EDID has the same preferred mode the user will not
> > > reconfigure the mode and there will be no change to HDMI mode.
> > 
> > The EDID should still be read, but as you say, userspace doesn't take
> > any action because it sees that the mode parameters are still the same,
> > as you have identified.
> > 
> > > The missing change to HDMI mode will cause the test to fail. The
> > > workaround that I am using is to reconfigure to another video
> > > mode and then configure to the preferred one but I think this
> > > could be fixed in the driver, right?
> > 
> > This one is extremely awkward - to fix it, we would need to check to
> > see whether we reconfigure the hardware each time we read the EDID,
> > and I don't think that's a particularly nice thing to do.
> > 
> > I'd like to hear what other DRM developers think about this issue.
> 
> I pondored whether we should have a counter on each connector for probe
> state, which helpers would increment whenever something changes, like:
> - connector_status (as tracked by probe helpers)
> - anything in the edid changes (when setting it
>   drm_mode_connector_update_edid_property)
> - other changes (like sink state changes in dpcd or whatever)
> 
> That way userspace would be able to reliably spot such changes and do a
> new modeset.

Yes, please. I have had similar wishes for state changes and overall
modeset counters.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

problem with 731c7d3, "main drm pull request for 4.8"

[Chris Wilson]

On Thu, Aug 04, 2016 at 02:08:46PM -0400, Mike Marshall wrote:
> [1.291797] [drm] Initialized drm 1.1.0 20060810
> [1.352761] [TTM] Zone  kernel: Available graphics memory: 502128 kiB
> [1.353248] [TTM] Initializing pool allocator
> [1.353660] [TTM] Initializing DMA pool allocator
> [1.355491] BUG: unable to handle kernel NULL pointer dereference
> at 0018
> [1.356038] IP: [] drm_pick_crtcs+0x125/0x280
> [drm_kms_helper]
> [1.356038] PGD 0
> [1.356038] Oops:  [#1] SMP
> [1.356038] Modules linked in: cirrus(+) drm_kms_helper ttm drm
> i2c_core ata_generic pata_acpi
> [1.356038] CPU: 0 PID: 181 Comm: systemd-udevd Not tainted
> 4.7.0-00810-gc1ece76 #9
> [1.356038] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2007
> [1.356038] task: 880036f4 task.stack: 880036fc
> [1.356038] RIP: 0010:[]  []
> drm_pick_crtcs+0x125/0x280 [drm_kms_helper]
> [1.356038] RSP: 0018:880036fc3898  EFLAGS: 00010217
> [1.356038] RAX: a00208f0 RBX:  RCX: 
> 1000
> [1.356038] RDX:  RSI: 81c574c0 RDI: 
> 880037172800
> [1.356038] RBP: 880036fc3900 R08: 0001cc90 R09: 
> 880036d829f8
> [1.356038] R10: 88003e001c80 R11: 880036d829d8 R12: 
> 880037172400
> [1.356038] R13: 1000 R14: 880037172800 R15: 
> 880036d829d0
> [1.356038] FS:  7f907d15d880() GS:88003fc0()
> knlGS:
> [1.356038] CS:  0010 DS:  ES:  CR0: 80050033
> [1.356038] CR2: 0018 CR3: 36f6d000 CR4: 
> 06f0
> [1.356038] Stack:
> [1.356038]  0008 880036d829e0 880036d829d8
> 0001
> [1.356038]   1003 880036d829e0
> 880036d829f8
> [1.356038]  0001 880037172400 0001
> 880036d829d0
> [1.356038] Call Trace:
> [1.356038]  [] drm_setup_crtcs+0x339/0xa00
> [drm_kms_helper]
> [1.356038]  [] ? mark_held_locks+0x66/0x90
> [1.356038]  [] ? __mutex_unlock_slowpath+0xd9/0x1a0
> [1.356038]  []
> drm_fb_helper_initial_config+0x81/0x3a8 [drm_kms_helper]
> [1.356038]  [] ? drm_modeset_unlock_all+0x66/0xc0 [drm]
> [1.356038]  [] cirrus_fbdev_init+0xa0/0xb0 [cirrus]
> [1.356038]  [] cirrus_modeset_init+0x18b/0x1e0 [cirrus]
> [1.356038]  [] cirrus_driver_load+0xbc/0x100 [cirrus]
> [1.356038]  [] drm_dev_register+0xa9/0xd0 [drm]

This is unlikely to be cirrus specific, but fallout from the
drm_dev_register reorganisation. Could you bisect down into that merge?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH] drm: avoid "possible bad bitmask?" warning

[Chris Wilson]

On Tue, Aug 09, 2016 at 06:35:10PM +0100, Dave Gordon wrote:
> Recent versions of gcc say this:
> 
> include/drm/i915_drm.h:96:34: warning: result of ‘65535 << 20’
> requires 37 bits to represent, but ‘int’ only has 32 bits
> [-Wshift-overflow=]
> 
> Reported-by: David Binderman 
> Signed-off-by: Dave Gordon 
> Cc: Dave Airlie 
> ---
>  include/drm/i915_drm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index b1755f8..4e1b274 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -93,6 +93,6 @@ extern bool i915_gpu_turbo_disable(void);
>  #defineI845_TSEG_SIZE_1M (3 << 1)
>  
>  #define INTEL_BSM 0x5c
> -#define   INTEL_BSM_MASK (0x << 20)
> +#define   INTEL_BSM_MASK (-(1u << 20))

Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/5] drm/amdgpu: Remove call to reservation_object_test_signaled_rcu before wait

[Chris Wilson]

Since fence_wait_timeout_reservation_object_wait_timeout_rcu() with a
timeout of 0 becomes reservation_object_test_signaled_rcu(), we do not
need to handle such conversion in the caller. The only challenge are
those callers that wish to differentiate the error code between the
nonblocking busy check and potentially blocking wait.

Signed-off-by: Chris Wilson 
Cc: Alex Deucher 
Cc: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 88fbed2389c0..a3e6f883ac2c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -407,10 +407,8 @@ int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, 
void *data,
return -ENOENT;
}
robj = gem_to_amdgpu_bo(gobj);
-   if (timeout == 0)
-   ret = reservation_object_test_signaled_rcu(robj->tbo.resv, 
true);
-   else
-   ret = reservation_object_wait_timeout_rcu(robj->tbo.resv, true, 
true, timeout);
+   ret = reservation_object_wait_timeout_rcu(robj->tbo.resv, true, true,
+ timeout);

/* ret == 0 means not signaled,
 * ret > 0 means signaled
-- 
2.8.1

[PATCH 2/5] drm: Remove manual call to reservation_object_test_signaled_rcu before wait

[Chris Wilson]

Since fence_wait_timeout_reservation_object_wait_timeout_rcu() with a
timeout of 0 becomes reservation_object_test_signaled_rcu(), we do not
need to handle such conversion in the caller. The only challenge are
those callers that wish to differentiate the error code between the
nonblocking busy check and potentially blocking wait.

Signed-off-by: Chris Wilson 
Cc: Lucas Stach 
Cc: Russell King <linux+etnaviv at armlinux.org.uk>
Cc: Christian Gmeiner 
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 24 ++--
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c 
b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 5ce3603e6eac..9ffca2478e02 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -409,20 +409,16 @@ int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 
op,
struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
struct drm_device *dev = obj->dev;
bool write = !!(op & ETNA_PREP_WRITE);
-   int ret;
-
-   if (op & ETNA_PREP_NOSYNC) {
-   if (!reservation_object_test_signaled_rcu(etnaviv_obj->resv,
- write))
-   return -EBUSY;
-   } else {
-   unsigned long remain = etnaviv_timeout_to_jiffies(timeout);
-
-   ret = reservation_object_wait_timeout_rcu(etnaviv_obj->resv,
- write, true, remain);
-   if (ret <= 0)
-   return ret == 0 ? -ETIMEDOUT : ret;
-   }
+   unsigned long remain =
+   op & ETNA_PREP_NOSYNC ? 0 : etnaviv_timeout_to_jiffies(timeout);
+   long lret;
+
+   lret = reservation_object_wait_timeout_rcu(etnaviv_obj->resv,
+  write, true, remain);
+   if (lret < 0)
+   return lret;
+   else if (lret == 0)
+   return remain == 0 ? -EBUSY : -ETIMEDOUT;

if (etnaviv_obj->flags & ETNA_BO_CACHED) {
if (!etnaviv_obj->sgt) {
-- 
2.8.1

[PATCH 4/5] drm/nouveau: Remove call to reservation_object_test_signaled_rcu before wait

[Chris Wilson]

Since fence_wait_timeout_reservation_object_wait_timeout_rcu() with a
timeout of 0 becomes reservation_object_test_signaled_rcu(), we do not
need to handle such conversion in the caller. The only challenge are
those callers that wish to differentiate the error code between the
nonblocking busy check and potentially blocking wait.

Signed-off-by: Chris Wilson 
Cc: Ben Skeggs 
---
 drivers/gpu/drm/nouveau/nouveau_gem.c | 21 +
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c 
b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 72e2399bce39..b90e21ff1ed8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -861,6 +861,7 @@ nouveau_gem_ioctl_cpu_prep(struct drm_device *dev, void 
*data,
struct nouveau_bo *nvbo;
bool no_wait = !!(req->flags & NOUVEAU_GEM_CPU_PREP_NOWAIT);
bool write = !!(req->flags & NOUVEAU_GEM_CPU_PREP_WRITE);
+   long lret;
int ret;

gem = drm_gem_object_lookup(file_priv, req->handle);
@@ -868,19 +869,15 @@ nouveau_gem_ioctl_cpu_prep(struct drm_device *dev, void 
*data,
return -ENOENT;
nvbo = nouveau_gem_object(gem);

-   if (no_wait)
-   ret = reservation_object_test_signaled_rcu(nvbo->bo.resv, 
write) ? 0 : -EBUSY;
-   else {
-   long lret;
+   lret = reservation_object_wait_timeout_rcu(nvbo->bo.resv, write, true,
+  no_wait ? 0 :30 * HZ);
+   if (!lret)
+   ret = -EBUSY;
+   else if (lret > 0)
+   ret = 0;
+   else
+   ret = lret;

-   lret = reservation_object_wait_timeout_rcu(nvbo->bo.resv, 
write, true, 30 * HZ);
-   if (!lret)
-   ret = -EBUSY;
-   else if (lret > 0)
-   ret = 0;
-   else
-   ret = lret;
-   }
nouveau_bo_sync_for_cpu(nvbo);
drm_gem_object_unreference_unlocked(gem);

-- 
2.8.1

[PATCH 3/5] drm/msm: Remove call to reservation_object_test_signaled_rcu before wait

[Chris Wilson]

Since fence_wait_timeout_reservation_object_wait_timeout_rcu() with a
timeout of 0 becomes reservation_object_test_signaled_rcu(), we do not
need to handle such conversion in the caller. The only challenge are
those callers that wish to differentiate the error code between the
nonblocking busy check and potentially blocking wait.

Signed-off-by: Chris Wilson 
Cc: Rob Clark 
---
 drivers/gpu/drm/msm/msm_gem.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 6cd4af443139..ab07eb6de25e 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -584,18 +584,16 @@ int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t 
op, ktime_t *timeout)
 {
struct msm_gem_object *msm_obj = to_msm_bo(obj);
bool write = !!(op & MSM_PREP_WRITE);
-
-   if (op & MSM_PREP_NOSYNC) {
-   if (!reservation_object_test_signaled_rcu(msm_obj->resv, write))
-   return -EBUSY;
-   } else {
-   int ret;
-
-   ret = reservation_object_wait_timeout_rcu(msm_obj->resv, write,
-   true, timeout_to_jiffies(timeout));
-   if (ret <= 0)
-   return ret == 0 ? -ETIMEDOUT : ret;
-   }
+   unsigned long remain =
+   op & MSG_PREP_NOSYNC ? 0 : timeout_to_jiffies(timeout);
+   long ret;
+
+   ret = reservation_object_wait_timeout_rcu(msm_obj->resv, write,
+ true,  remain);
+   if (ret == 0)
+   return remain == 9 ? -EBUSY : -ETIMEDOUT;
+   else if (ret < 0)
+   return ret;

/* TODO cache maintenance */

-- 
2.8.1

[PATCH 5/5] drm/vmwgfx: Remove call to reservation_object_test_signaled_rcu before wait

[Chris Wilson]

Since fence_wait_timeout_reservation_object_wait_timeout_rcu() with a
timeout of 0 becomes reservation_object_test_signaled_rcu(), we do not
need to handle such conversion in the caller. The only challenge are
those callers that wish to differentiate the error code between the
nonblocking busy check and potentially blocking wait.

Signed-off-by: Chris Wilson 
Cc: Sinclair Yeh 
Cc: Thomas Hellstrom 
---
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
index 6a328d507a28..afda2a57a094 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
@@ -574,10 +574,8 @@ static int vmw_user_dmabuf_synccpu_grab(struct 
vmw_user_dma_buffer *user_bo,
bool nonblock = !!(flags & drm_vmw_synccpu_dontblock);
long lret;

-   if (nonblock)
-   return reservation_object_test_signaled_rcu(bo->resv, 
true) ? 0 : -EBUSY;
-
-   lret = reservation_object_wait_timeout_rcu(bo->resv, true, 
true, MAX_SCHEDULE_TIMEOUT);
+   lret = reservation_object_wait_timeout_rcu(bo->resv, true, true,
+  nonblock ? 0 
:MAX_SCHEDULE_TIMEOUT);
if (!lret)
return -EBUSY;
else if (lret < 0)
-- 
2.8.1

[PATCH] dma-buf: Restart reservation_object_get_fences_rcu() after writes

[Chris Wilson]

In order to be completely generic, we have to double check the read
seqlock after acquiring a reference to the fence. If the driver is
allocating fences from a SLAB_DESTROY_BY_RCU, or similar freelist, then
within an RCU grace period a fence may be freed and reallocated. The RCU
read side critical section does not prevent this reallocation, instead
we have to inspect the reservation's seqlock to double check if the
fences have been reassigned as we were acquiring our reference.
Here it is the combination of the write barrier in the seqlock, and of
the full barrier after a successful fence_get_rcu() that ensure that as
long as the seqlock is stable we hold the right references to the
fences.

Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
Cc: Maarten Lankhorst 
Cc: Christian König 
Cc: Alex Deucher 
Cc: Sumit Semwal 
---
 drivers/dma-buf/reservation.c | 73 +++
 1 file changed, 32 insertions(+), 41 deletions(-)

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index 9566a62ad8e3..8b1dd90b1a4e 100644
--- a/drivers/dma-buf/reservation.c
+++ b/drivers/dma-buf/reservation.c
@@ -280,18 +280,24 @@ int reservation_object_get_fences_rcu(struct 
reservation_object *obj,
  unsigned *pshared_count,
  struct fence ***pshared)
 {
-   unsigned shared_count = 0;
-   unsigned retry = 1;
-   struct fence **shared = NULL, *fence_excl = NULL;
-   int ret = 0;
+   struct fence **shared = NULL;
+   struct fence *fence_excl;
+   unsigned int shared_count;
+   int ret = 1;

-   while (retry) {
+   do {
struct reservation_object_list *fobj;
-   unsigned seq;
+   unsigned int seq;
+   unsigned int i;

-   seq = read_seqcount_begin(>seq);
+   shared_count = i = 0;

rcu_read_lock();
+   seq = read_seqcount_begin(>seq);
+
+   fence_excl = rcu_dereference(obj->fence_excl);
+   if (fence_excl && !fence_get_rcu(fence_excl))
+   goto unlock;

fobj = rcu_dereference(obj->fence);
if (fobj) {
@@ -309,52 +315,37 @@ int reservation_object_get_fences_rcu(struct 
reservation_object *obj,
}

ret = -ENOMEM;
-   shared_count = 0;
break;
}
shared = nshared;
-   memcpy(shared, fobj->shared, sz);
shared_count = fobj->shared_count;
-   } else
-   shared_count = 0;
-   fence_excl = rcu_dereference(obj->fence_excl);
-
-   retry = read_seqcount_retry(>seq, seq);
-   if (retry)
-   goto unlock;
-
-   if (!fence_excl || fence_get_rcu(fence_excl)) {
-   unsigned i;

for (i = 0; i < shared_count; ++i) {
-   if (fence_get_rcu(shared[i]))
-   continue;
-
-   /* uh oh, refcount failed, abort and retry */
-   while (i--)
-   fence_put(shared[i]);
-
-   if (fence_excl) {
-   fence_put(fence_excl);
-   fence_excl = NULL;
-   }
-
-   retry = 1;
-   break;
+   shared[i] = rcu_dereference(fobj->shared[i]);
+   if (!fence_get_rcu(shared[i]))
+   break;
}
-   } else
-   retry = 1;
+   }
+
+   if (i != shared_count || read_seqcount_retry(>seq, seq)) {
+   while (i--)
+   fence_put(shared[i]);
+   fence_put(fence_excl);
+   goto unlock;
+   }

+   ret = 0;
 unlock:
rcu_read_unlock();
-   }
-   *pshared_count = shared_count;
-   if (shared_count)
-   *pshared = shared;
-   else {
-   *pshared = NULL;
+   } while (ret);
+
+   if (!shared_count) {
kfree(shared);
+   shared = NULL;
}
+
+   *pshared_count = shared_count;
+   *pshared = shared;
*pfence_excl = fence_excl;

return ret;
-- 
2.8.1

[PATCH 2/3] drm/i915: Track pinned VMA

[Chris Wilson]

Treat the VMA as the primary struct responsible for tracking bindings
into the GPU's VM. That is we want to treat the VMA returned after we
pin an object into the VM as the cookie we hold and eventually release
when unpinning. Doing so eliminates the ambiguity in pinning the object
and then searching for the relevant pin later.

v2: Joonas' stylistic nitpicks.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_debugfs.c|   2 +-
 drivers/gpu/drm/i915/i915_drv.h|  60 ++--
 drivers/gpu/drm/i915/i915_gem.c| 225 +++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  60 
 drivers/gpu/drm/i915/i915_gem_fence.c  |  14 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c|  67 +
 drivers/gpu/drm/i915/i915_gem_gtt.h|  14 --
 drivers/gpu/drm/i915/i915_gem_request.c|   2 +-
 drivers/gpu/drm/i915/i915_gem_request.h|   2 +-
 drivers/gpu/drm/i915/i915_gem_stolen.c |   2 +-
 drivers/gpu/drm/i915/i915_gem_tiling.c |   2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c  |  58 +++-
 drivers/gpu/drm/i915/intel_display.c   |  55 ---
 drivers/gpu/drm/i915/intel_drv.h   |   5 +-
 drivers/gpu/drm/i915/intel_fbc.c   |   2 +-
 drivers/gpu/drm/i915/intel_fbdev.c |  19 +--
 drivers/gpu/drm/i915/intel_guc_loader.c|  21 +--
 drivers/gpu/drm/i915/intel_overlay.c   |  32 ++--
 drivers/gpu/drm/i915/intel_sprite.c|   8 +-
 19 files changed, 255 insertions(+), 395 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 3a1d34b334a0..28f14192fce5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -105,7 +105,7 @@ static char get_tiling_flag(struct drm_i915_gem_object *obj)

 static char get_global_flag(struct drm_i915_gem_object *obj)
 {
-   return i915_gem_obj_to_ggtt(obj) ? 'g' : ' ';
+   return i915_gem_object_to_ggtt(obj, NULL) ?  'g' : ' ';
 }

 static char get_pin_mapped_flag(struct drm_i915_gem_object *obj)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9766a1d62d81..bfa2bdae0f7d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3076,7 +3076,7 @@ struct drm_i915_gem_object 
*i915_gem_object_create_from_data(
 void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file);
 void i915_gem_free_object(struct drm_gem_object *obj);

-int __must_check
+struct i915_vma * __must_check
 i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
 const struct i915_ggtt_view *view,
 u64 size,
@@ -3272,12 +3272,11 @@ i915_gem_object_set_to_gtt_domain(struct 
drm_i915_gem_object *obj,
  bool write);
 int __must_check
 i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
-int __must_check
+struct i915_vma * __must_check
 i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 u32 alignment,
 const struct i915_ggtt_view *view);
-void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
- const struct i915_ggtt_view 
*view);
+void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
 int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
int align);
 int i915_gem_open(struct drm_device *dev, struct drm_file *file);
@@ -3297,63 +3296,34 @@ struct drm_gem_object *i915_gem_prime_import(struct 
drm_device *dev,
 struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *gem_obj, int flags);

-u64 i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
- const struct i915_ggtt_view *view);
-u64 i915_gem_obj_offset(struct drm_i915_gem_object *o,
-   struct i915_address_space *vm);
-static inline u64
-i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
-{
-   return i915_gem_obj_ggtt_offset_view(o, _ggtt_view_normal);
-}
-
-bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
- const struct i915_ggtt_view *view);
-bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
-   struct i915_address_space *vm);
-
 struct i915_vma *
 i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
-   struct i915_address_space *vm);
-struct i915_vma *
-i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
- const struct i915_ggtt_view *view);
+struct i915_address_space *vm,
+const struct i915_ggtt_view *view);

 struct i915_vma *
 i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
- struct i915_address_space *vm

[PATCH 3/3] drm/i915: Introduce i915_ggtt_offset()

[Chris Wilson]

This little helper only exists to safely discard the upper unused 32bits
of the general 64-bit VMA address - as we know that all Global GTT
currently are less than 4GiB in size and so that the upper bits must be
zero. In many places, we use a u32 for the global GTT offset and we want
to document where we are discarding the full VMA offset.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_debugfs.c|  6 +++---
 drivers/gpu/drm/i915/i915_drv.h|  2 +-
 drivers/gpu/drm/i915/i915_gem.c|  6 +++---
 drivers/gpu/drm/i915/i915_gem_context.c|  6 --
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.h|  7 +++
 drivers/gpu/drm/i915/i915_guc_submission.c | 15 ---
 drivers/gpu/drm/i915/intel_display.c   | 11 +--
 drivers/gpu/drm/i915/intel_fbdev.c |  6 +++---
 drivers/gpu/drm/i915/intel_guc_loader.c|  6 +++---
 drivers/gpu/drm/i915/intel_lrc.c   | 20 +++-
 drivers/gpu/drm/i915/intel_overlay.c   |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c| 28 ++--
 13 files changed, 64 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 28f14192fce5..f67c53baaa75 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2008,7 +2008,7 @@ static void i915_dump_lrc_obj(struct seq_file *m,

if (vma->flags & I915_VMA_GLOBAL_BIND)
seq_printf(m, "\tBound in GGTT at 0x%08x\n",
-  lower_32_bits(vma->node.start));
+  i915_ggtt_offset(vma));

if (i915_gem_object_get_pages(vma->obj)) {
seq_puts(m, "\tFailed to get pages for context object\n\n");
@@ -2020,8 +2020,8 @@ static void i915_dump_lrc_obj(struct seq_file *m,
u32 *reg_state = kmap_atomic(page);

for (j = 0; j < 0x600 / sizeof(u32) / 4; j += 4) {
-   seq_printf(m, "\t[0x%08llx] 0x%08x 0x%08x 0x%08x 
0x%08x\n",
-  vma->node.start + 4096 + (j * 4),
+   seq_printf(m, "\t[0x%04x] 0x%08x 0x%08x 0x%08x 
0x%08x\n",
+  j * 4,
   reg_state[j], reg_state[j + 1],
   reg_state[j + 2], reg_state[j + 3]);
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bfa2bdae0f7d..0a58d36923f1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3323,7 +3323,7 @@ static inline unsigned long
 i915_gem_object_ggtt_offset(struct drm_i915_gem_object *o,
const struct i915_ggtt_view *view)
 {
-   return i915_gem_object_to_ggtt(o, view)->node.start;
+   return i915_ggtt_offset(i915_gem_object_to_ggtt(o, view));
 }

 /* i915_gem_fence.c */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 19d81f28cbbe..54d277eac631 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -758,7 +758,7 @@ i915_gem_gtt_pread(struct drm_device *dev,

i915_gem_object_pin_pages(obj);
} else {
-   node.start = vma->node.start;
+   node.start = i915_ggtt_offset(vma);
node.allocated = false;
ret = i915_gem_object_put_fence(obj);
if (ret)
@@ -1062,7 +1062,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,

i915_gem_object_pin_pages(obj);
} else {
-   node.start = vma->node.start;
+   node.start = i915_ggtt_offset(vma);
node.allocated = false;
ret = i915_gem_object_put_fence(obj);
if (ret)
@@ -1703,7 +1703,7 @@ int i915_gem_fault(struct vm_area_struct *area, struct 
vm_fault *vmf)
goto err_unpin;

/* Finally, remap it using the new GTT offset */
-   pfn = ggtt->mappable_base + vma->node.start;
+   pfn = ggtt->mappable_base + i915_ggtt_offset(vma);
pfn >>= PAGE_SHIFT;

if (unlikely(view.type == I915_GGTT_VIEW_PARTIAL)) {
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index e566167d9441..98d2956f91f4 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -631,7 +631,8 @@ mi_set_context(struct drm_i915_gem_request *req, u32 
hw_flags)

intel_ring_emit(ring, MI_NOOP);
intel_ring_emit(ring, MI_SET_CONTEXT);
-   intel_ring_emit(ring, req->ctx->engine[RCS].state->node.start | flags);
+   intel_ring_emit(ring,
+   i915_ggtt_offset(req->ctx->engine[RCS].state) | flags);
/*
 * w/a: MI_SET_CONT

[PATCH 2/3] dma-buf: Restart reservation_object_wait_timeout_rcu() after writes

[Chris Wilson]

In order to be completely generic, we have to double check the read
seqlock after acquiring a reference to the fence. If the driver is
allocating fences from a SLAB_DESTROY_BY_RCU, or similar freelist, then
within an RCU grace period a fence may be freed and reallocated. The RCU
read side critical section does not prevent this reallocation, instead
we have to inspect the reservation's seqlock to double check if the
fences have been reassigned as we were acquiring our reference.

Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
Cc: Maarten Lankhorst 
Cc: Christian König 
Cc: Alex Deucher 
Cc: Sumit Semwal 
---
 drivers/dma-buf/reservation.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index 7a1e517787e1..352bf6cde7b9 100644
--- a/drivers/dma-buf/reservation.c
+++ b/drivers/dma-buf/reservation.c
@@ -388,9 +388,6 @@ retry:
if (fobj)
shared_count = fobj->shared_count;

-   if (read_seqcount_retry(>seq, seq))
-   goto unlock_retry;
-
for (i = 0; i < shared_count; ++i) {
struct fence *lfence = rcu_dereference(fobj->shared[i]);

@@ -413,9 +410,6 @@ retry:
if (!shared_count) {
struct fence *fence_excl = rcu_dereference(obj->fence_excl);

-   if (read_seqcount_retry(>seq, seq))
-   goto unlock_retry;
-
if (fence_excl &&
!test_bit(FENCE_FLAG_SIGNALED_BIT, _excl->flags)) {
if (!fence_get_rcu(fence_excl))
@@ -430,6 +424,11 @@ retry:

rcu_read_unlock();
if (fence) {
+   if (read_seqcount_retry(>seq, seq)) {
+   fence_put(fence);
+   goto retry;
+   }
+
ret = fence_wait_timeout(fence, intr, ret);
fence_put(fence);
if (ret > 0 && wait_all && (i + 1 < shared_count))
-- 
2.8.1

[PATCH 3/3] dma-buf: Restart reservation_object_test_signaled_rcu() after writes

[Chris Wilson]

In order to be completely generic, we have to double check the read
seqlock after acquiring a reference to the fence. If the driver is
allocating fences from a SLAB_DESTROY_BY_RCU, or similar freelist, then
within an RCU grace period a fence may be freed and reallocated. The RCU
read side critical section does not prevent this reallocation, instead
we have to inspect the reservation's seqlock to double check if the
fences have been reassigned as we were acquiring our reference.

Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
Cc: Maarten Lankhorst 
Cc: Christian König 
Cc: Alex Deucher 
Cc: Sumit Semwal 
---
 drivers/dma-buf/reservation.c | 30 ++
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index 352bf6cde7b9..3e158d0c4998 100644
--- a/drivers/dma-buf/reservation.c
+++ b/drivers/dma-buf/reservation.c
@@ -474,12 +474,13 @@ bool reservation_object_test_signaled_rcu(struct 
reservation_object *obj,
  bool test_all)
 {
unsigned seq, shared_count;
-   int ret = true;
+   int ret;

+   rcu_read_lock();
 retry:
+   ret = true;
shared_count = 0;
seq = read_seqcount_begin(>seq);
-   rcu_read_lock();

if (test_all) {
unsigned i;
@@ -490,47 +491,36 @@ retry:
if (fobj)
shared_count = fobj->shared_count;

-   if (read_seqcount_retry(>seq, seq))
-   goto unlock_retry;
-
for (i = 0; i < shared_count; ++i) {
struct fence *fence = rcu_dereference(fobj->shared[i]);

ret = reservation_object_test_signaled_single(fence);
if (ret < 0)
-   goto unlock_retry;
+   goto retry;
else if (!ret)
break;
}

-   /*
-* There could be a read_seqcount_retry here, but nothing cares
-* about whether it's the old or newer fence pointers that are
-* signaled. That race could still have happened after checking
-* read_seqcount_retry. If you care, use ww_mutex_lock.
-*/
+   if (read_seqcount_retry(>seq, seq))
+   goto retry;
}

if (!shared_count) {
struct fence *fence_excl = rcu_dereference(obj->fence_excl);

-   if (read_seqcount_retry(>seq, seq))
-   goto unlock_retry;
-
if (fence_excl) {
ret = reservation_object_test_signaled_single(
fence_excl);
if (ret < 0)
-   goto unlock_retry;
+   goto retry;
+
+   if (read_seqcount_retry(>seq, seq))
+   goto retry;
}
}

rcu_read_unlock();
return ret;
-
-unlock_retry:
-   rcu_read_unlock();
-   goto retry;
 }
 EXPORT_SYMBOL_GPL(reservation_object_test_signaled_rcu);

-- 
2.8.1

[PATCH 2/3] drm/i915: Track pinned VMA

[Chris Wilson]

Apologies, please ignore these 2. Didn't pass the right start point to
git-send-email.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/5] drm/msm: Remove call to reservation_object_test_signaled_rcu before wait

[Chris Wilson]

On Thu, Aug 11, 2016 at 11:19:50PM +0530, Sumit Semwal wrote:
> Hi Chris,
> 
> On 11 August 2016 at 20:16, Chris Wilson  wrote:
> > Since fence_wait_timeout_reservation_object_wait_timeout_rcu() with a
> > timeout of 0 becomes reservation_object_test_signaled_rcu(), we do not
> > need to handle such conversion in the caller. The only challenge are
> > those callers that wish to differentiate the error code between the
> > nonblocking busy check and potentially blocking wait.
> >
> > Signed-off-by: Chris Wilson 
> > Cc: Rob Clark 
> > ---
> >  drivers/gpu/drm/msm/msm_gem.c | 22 ++
> >  1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> > index 6cd4af443139..ab07eb6de25e 100644
> > --- a/drivers/gpu/drm/msm/msm_gem.c
> > +++ b/drivers/gpu/drm/msm/msm_gem.c
> > @@ -584,18 +584,16 @@ int msm_gem_cpu_prep(struct drm_gem_object *obj, 
> > uint32_t op, ktime_t *timeout)
> >  {
> > struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > bool write = !!(op & MSM_PREP_WRITE);
> > -
> > -   if (op & MSM_PREP_NOSYNC) {
> > -   if (!reservation_object_test_signaled_rcu(msm_obj->resv, 
> > write))
> > -   return -EBUSY;
> > -   } else {
> > -   int ret;
> > -
> > -   ret = reservation_object_wait_timeout_rcu(msm_obj->resv, 
> > write,
> > -   true, timeout_to_jiffies(timeout));
> > -   if (ret <= 0)
> > -   return ret == 0 ? -ETIMEDOUT : ret;
> > -   }
> > +   unsigned long remain =
> > +   op & MSG_PREP_NOSYNC ? 0 : timeout_to_jiffies(timeout);
> > +   long ret;
> > +
> > +   ret = reservation_object_wait_timeout_rcu(msm_obj->resv, write,
> > + true,  remain);
> > +   if (ret == 0)
> > +   return remain == 9 ? -EBUSY : -ETIMEDOUT;
> 
> Am sure this must be a typo - 0 instead of 9?

Eek. It's close, but not that close.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] dma-buf: Introduce fence_get_rcu_safe()

[Chris Wilson]

This variant of fence_get_rcu() takes an RCU protected pointer to a
fence and carefully returns a reference to the fence ensuring that it is
not reallocated as it does. This is required when mixing fences and
SLAB_DESTROY_BY_RCU - although it serves a more pedagogical function atm

Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
Cc: Sumit Semwal 
---
 include/linux/fence.h | 51 ++-
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/include/linux/fence.h b/include/linux/fence.h
index 5f50ab273c38..e93f7e70d0fe 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -187,6 +187,16 @@ void fence_release(struct kref *kref);
 void fence_free(struct fence *fence);

 /**
+ * fence_put - decreases refcount of the fence
+ * @fence: [in]fence to reduce refcount of
+ */
+static inline void fence_put(struct fence *fence)
+{
+   if (fence)
+   kref_put(>refcount, fence_release);
+}
+
+/**
  * fence_get - increases refcount of the fence
  * @fence: [in]fence to increase refcount of
  *
@@ -214,13 +224,44 @@ static inline struct fence *fence_get_rcu(struct fence 
*fence)
 }

 /**
- * fence_put - decreases refcount of the fence
- * @fence: [in]fence to reduce refcount of
+ * fence_get_rcu_safe  - acquire a reference to an RCU tracked fence
+ * @fence: [in]pointer to fence to increase refcount of
+ *
+ * Function returns NULL if no refcount could be obtained, or the fence.
+ * This function handles acquiring a reference to a fence that may be
+ * reallocated within the RCU grace period (such as with SLAB_DESTROY_BY_RCU),
+ * so long as the caller is using RCU on the pointer to the fence.
+ *
+ * The caller is required to hold the RCU read lock.
  */
-static inline void fence_put(struct fence *fence)
+static inline struct fence *fence_get_rcu_safe(struct fence * __rcu *fencep)
 {
-   if (fence)
-   kref_put(>refcount, fence_release);
+   do {
+   struct fence *fence;
+
+   fence = rcu_dereference(*fencep);
+   if (!fence || !fence_get_rcu(fence))
+   return NULL;
+
+   /* The atomic_inc_not_zero() inside fence_get_rcu()
+* provides a full memory barrier upon success (such as now).
+* This is paired with the write barrier from assigning
+* to the __rcu protected fence pointer so that if that
+* pointer still matches the current fence, we know we
+* have successfully acquire a reference to it. If it no
+* longer matches, we are holding a reference to some other
+* reallocated pointer. This is possible if the allocator
+* is using a freelist like SLAB_DESTROY_BY_RCU where the
+* fence remains valid for the RCU grace period, but it
+* may be reallocated. When using such allocators, we are
+* responsible for ensuring the reference we get is to
+* the right fence, as below.
+*/
+   if (fence == rcu_access_pointer(*fencep))
+   return fence;
+
+   fence_put(fence);
+   } while (1);
 }

 int fence_signal(struct fence *fence);
-- 
2.8.1

[PATCH 1/2] drm: Introduce DRM_DEV_* log messages

[Chris Wilson]

On Fri, Aug 12, 2016 at 01:00:53PM -0400, Sean Paul wrote:
> This patch consolodates all the various log functions/macros into
> one uber function, drm_log. It also introduces some new DRM_DEV_*
> variants that print the device name to delineate multiple devices
> of the same type.

> +void drm_log(const struct device *dev, const char *level, unsigned int 
> category,
> +  const char *function_name, const char *prefix,
> +  const char *format, ...)
>  {
> + if (dev)
> + printk("%s%s [" DRM_NAME ":%s]%s %pV", level,
> +dev_name(dev), function_name, prefix, );
> + else
> + printk("%s[" DRM_NAME ":%s]%s %pV", level, function_name,
> +prefix, );

My hope was that we would migrate towards dev_printk so that our log
messages would have better conformity, especially between our messages
and those printed by subsystems on our behalf (using our struct device).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2 1/2] drm: Introduce DRM_DEV_* log messages

[Chris Wilson]

On Fri, Aug 12, 2016 at 01:30:00PM -0400, Sean Paul wrote:
> This patch consolidates all the various log functions/macros into
> one uber function, drm_log. It also introduces some new DRM_DEV_*
> variants that print the device name to delineate multiple devices
> of the same type.
> 
> Signed-off-by: Sean Paul 
> ---
> 
> Changes in v2:
>   - Use dev_printk for the dev variant (Chris Wilson)
> 
> 
>  drivers/gpu/drm/drm_drv.c |  31 +--
>  include/drm/drmP.h| 133 
> --
>  2 files changed, 82 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 57ce973..edd3291 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -63,37 +63,30 @@ static struct idr drm_minors_idr;
>  
>  static struct dentry *drm_debugfs_root;
>  
> -void drm_err(const char *format, ...)
> +void drm_log(const struct device *dev, const char *level, unsigned int 
> category,

I would have called this drm_printk() to match the function it wraps.

> +  const char *function_name, const char *prefix,
> +  const char *format, ...)
>  {
>   struct va_format vaf;
>   va_list args;
>  
> - va_start(args, format);
> -
> - vaf.fmt = format;
> - vaf.va = 
> -
> - printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV",
> -__builtin_return_address(0), );
> -
> - va_end(args);
> -}
> -EXPORT_SYMBOL(drm_err);
> -
> -void drm_ut_debug_printk(const char *function_name, const char *format, ...)
> -{
> - struct va_format vaf;
> - va_list args;
> + if (category != DRM_UT_NONE && !(drm_debug & category))
> + return;
>  
>   va_start(args, format);
>   vaf.fmt = format;
>   vaf.va = 
>  
> - printk(KERN_DEBUG "[" DRM_NAME ":%s] %pV", function_name, );
> + if (dev)
> + dev_printk(level, dev, "[" DRM_NAME ":%s]%s %pV", function_name,
> +prefix, );
> + else
> + printk("%s[" DRM_NAME ":%s]%s %pV", level, function_name,
> +prefix, );

lgtm.

> -#define DRM_ERROR(fmt, ...)  \
> - drm_err(fmt, ##__VA_ARGS__)
> +#define DRM_DEV_ERROR(dev, fmt, ...) \
> + drm_log(dev, KERN_ERR, DRM_UT_NONE, __func__, " *ERROR*", fmt,  \
> + ##__VA_ARGS__)
> +#define DRM_ERROR(fmt, ...) DRM_DEV_ERROR(NULL, fmt, ##__VA_ARGS__)

And these look like a reasonable solution given the constraints.

Out of curiosity, how much did the kernel build grow by adding a NULL
parameter everywhere?

Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2 1/2] drm: Introduce DRM_DEV_* log messages

[Chris Wilson]

On Fri, Aug 12, 2016 at 09:26:32PM +0200, Lukas Wunner wrote:
> On Fri, Aug 12, 2016 at 07:39:38PM +0100, Chris Wilson wrote:
> > On Fri, Aug 12, 2016 at 01:30:00PM -0400, Sean Paul wrote:
> > > This patch consolidates all the various log functions/macros into
> > > one uber function, drm_log. It also introduces some new DRM_DEV_*
> > > variants that print the device name to delineate multiple devices
> > > of the same type.
> > > 
> > > Signed-off-by: Sean Paul 
> > > ---
> > > 
> > > Changes in v2:
> > >   - Use dev_printk for the dev variant (Chris Wilson)
> > > 
> > > 
> > >  drivers/gpu/drm/drm_drv.c |  31 +--
> > >  include/drm/drmP.h| 133 
> > > --
> > >  2 files changed, 82 insertions(+), 82 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > index 57ce973..edd3291 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -63,37 +63,30 @@ static struct idr drm_minors_idr;
> > >  
> > >  static struct dentry *drm_debugfs_root;
> > >  
> > > -void drm_err(const char *format, ...)
> > > +void drm_log(const struct device *dev, const char *level, unsigned int 
> > > category,
> > 
> > I would have called this drm_printk() to match the function it wraps.
> 
> lxr.free-electrons.com says dev_info() is used in 2056 files whereas
> dev_printk() is only used in 90 files. And dev_log() doesn't exist.
> So drm_info() would arguably make the most sense.

dev_printk is the underlying mechanism, dev_log() is a curry function
calling dev_printk with some parameters already provided.

Speaking of which, if we did separate drm_printk() and drm_dev_printk(),
if drm_printk just called drm_dev_printk(NULL, ...) we would barely grow
the build.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v3 1/2] drm: Introduce DRM_DEV_* log messages

[Chris Wilson]

On Fri, Aug 12, 2016 at 04:29:37PM -0400, Sean Paul wrote:
> This patch consolidates all the various log functions/macros into
> one uber function, drm_printk. It also introduces some new DRM_DEV_*
> variants that use dev_printk to print the device name, which helps
> delineate multiple devices of the same type.
> 
> Signed-off-by: Sean Paul 
> ---
> 
> Changes in v2:
> - Use dev_printk for the dev variant (Chris Wilson)
> 
> Changes in v3:
>   - Rename drm_log to drm_dev_printk (Chris Wilson)
>   - Break out drm_printk from drm_dev_printk to reduce
> image growth due to passing NULL around (Chris Wilson)
> 
>  drivers/gpu/drm/drm_drv.c |  25 ++---
>  include/drm/drmP.h| 140 
> +++---
>  2 files changed, 101 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 57ce973..e141ead 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -63,37 +63,46 @@ static struct idr drm_minors_idr;
>  
>  static struct dentry *drm_debugfs_root;
>  
> -void drm_err(const char *format, ...)
> +void drm_dev_printk(const struct device *dev, const char *level,
> + unsigned int category, const char *function_name,
> + const char *prefix, const char *format, ...)
>  {
>   struct va_format vaf;
>   va_list args;
>  
> - va_start(args, format);
> + if (category != DRM_UT_NONE && !(drm_debug & category))
> + return;
>  
> + va_start(args, format);
>   vaf.fmt = format;
>   vaf.va = 
>  
> - printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV",
> -__builtin_return_address(0), );
> + dev_printk(level, dev, "[" DRM_NAME ":%s]%s %pV", function_name, prefix,
> +);

dev_printk does handle NULL dev, that's a relief!

>  
>   va_end(args);
>  }
> -EXPORT_SYMBOL(drm_err);
> +EXPORT_SYMBOL(drm_dev_printk);
>  
> -void drm_ut_debug_printk(const char *function_name, const char *format, ...)
> +void drm_printk(const char *level, unsigned int category,
> + const char *function_name, const char *prefix,
> + const char *format, ...)
>  {
>   struct va_format vaf;
>   va_list args;
>  
> + if (category != DRM_UT_NONE && !(drm_debug & category))
> + return;
> +
>   va_start(args, format);
>   vaf.fmt = format;
>   vaf.va = 
>  
> -     printk(KERN_DEBUG "[" DRM_NAME ":%s] %pV", function_name, );
> + printk("%s[" DRM_NAME ":%s]%s %pV", level, function_name, prefix, );

Ok, I just tried to make a common drm_dev_printk_emit() and made a right
mess. A pair of functions is definitely better.

Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v3 01/11] drm/i915: Add i915 perf infrastructure

[Chris Wilson]

_state(struct drm_device *dev);
>  extern int i915_restore_state(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_perf.c 
> b/drivers/gpu/drm/i915/i915_perf.c
> new file mode 100644
> index 000..d0ed645
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -0,0 +1,430 @@
> +/*
> + * Copyright © 2015-2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *   Robert Bragg 
> + */
> +
> +#include 
> +#include 
> +
> +#include "i915_drv.h"
> +
> +struct perf_open_properties {
> + u32 sample_flags;
> +
> + u64 single_context:1;
> + u64 ctx_handle;
> +};
> +
> +static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> +  struct file *file,
> +  char __user *buf,
> +  size_t count,
> +  loff_t *ppos)
> +{
> + struct i915_perf_read_state state = { count, 0, buf };
> + int ret = stream->ops->read(stream, );
> +
> + /* If we've successfully copied any data then reporting that
> +  * takes precedence over any internal error status, so the
> +  * data isn't lost
> +  */
> + return state.read ? state.read : (ret ? ret : -EAGAIN);

return state.read ?: ret ?: -EAGAIN;

Alternatively you could follow the standard pattern for read. Dare I ask
what is going to go into state that needs the obfuscation?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v3 03/11] drm/i915: return EACCES for check_cmd() failures

[Chris Wilson]

On Mon, Aug 15, 2016 at 03:41:20PM +0100, Robert Bragg wrote:
> check_cmd() is checking whether a command adheres to certain
> restrictions that ensure it's safe to execute within a privileged batch
> buffer. Returning false implies a privilege problem, not that the
> command is invalid.
> 
> The distinction makes the difference between allowing the buffer to be
> executed as an unprivileged batch buffer or returning an EINVAL error to
> userspace without executing anything.

Ah, but you choose to actually execute it instead. We can't allow that
either.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2] dma-buf: Wait on the reservation object when sync'ing before CPU access

[Chris Wilson]

Rendering operations to the dma-buf are tracked implicitly via the
reservation_object (dmabuf->resv). This is used to allow poll() to
wait upon outstanding rendering (or just query the current status of
rendering). The dma-buf sync ioctl allows userspace to prepare the
dma-buf for CPU access, which should include waiting upon rendering.
(Some drivers may need to do more work to ensure that the dma-buf mmap
is coherent as well as complete.)

v2: Always wait upon the reservation object implicitly. We choose to do
it after the native handler in case it can do so more efficiently.

Testcase: igt/prime_vgem
Testcase: igt/gem_concurrent_blit # *vgem*
Signed-off-by: Chris Wilson 
Cc: Sumit Semwal 
Cc: Daniel Vetter 
Cc: Eric Anholt 
Cc: linux-media at vger.kernel.org
Cc: dri-devel at lists.freedesktop.org
Cc: linaro-mm-sig at lists.linaro.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/dma-buf/dma-buf.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index ddaee60ae52a..cf04d249a6a4 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -586,6 +586,22 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment 
*attach,
 }
 EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);

+static int __dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
+ enum dma_data_direction direction)
+{
+   bool write = (direction == DMA_BIDIRECTIONAL ||
+ direction == DMA_TO_DEVICE);
+   struct reservation_object *resv = dmabuf->resv;
+   long ret;
+
+   /* Wait on any implicit rendering fences */
+   ret = reservation_object_wait_timeout_rcu(resv, write, true,
+ MAX_SCHEDULE_TIMEOUT);
+   if (ret < 0)
+   return ret;
+
+   return 0;
+}

 /**
  * dma_buf_begin_cpu_access - Must be called before accessing a dma_buf from 
the
@@ -608,6 +624,13 @@ int dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
if (dmabuf->ops->begin_cpu_access)
ret = dmabuf->ops->begin_cpu_access(dmabuf, direction);

+   /* Ensure that all fences are waited upon - but we first allow
+* the native handler the chance to do so more efficiently if it
+* chooses. A double invocation here will be reasonably cheap no-op.
+*/
+   if (ret == 0)
+   ret = __dma_buf_begin_cpu_access(dmabuf, direction);
+
return ret;
 }
 EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
-- 
2.8.1

[PATCH v2] drm/i915: Use common RPS scheme for Cherryview

[Chris Wilson]

Cherryview uses a custom static definition of its RPS parameters (for
automatically driving GPU frequency selection) - yet still couples into
the waitboosting scheme of the common RPS setup.

The rationale given in commit 8fb55197e64d ("drm/i915: Agressive
downclocking on Baytrail") was that Cherryview might have to take the
common powerwell (unlike Baytrail it has multiple powerwells) to read the
RPS registers more often. However, we have reports that the current values
are not working well for kodi (the frequency stays too low). Lets use the
common values and see if we can tune them appropriately to benefit
everyone.

Signed-off-by: Chris Wilson 
Cc: fritsch at kodi.tv
Cc: Deepak S 
Cc: Ville Syrjälä 
Cc: Rodrigo Vivi 
Cc: Daniel Vetter 
---
 drivers/gpu/drm/i915/intel_pm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 20794804f3bb..a140fe075e1b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4837,8 +4837,7 @@ static void valleyview_set_rps(struct drm_i915_private 
*dev_priv, u8 val)

if (val != dev_priv->rps.cur_freq) {
vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
-   if (!IS_CHERRYVIEW(dev_priv))
-   gen6_set_rps_thresholds(dev_priv, val);
+   gen6_set_rps_thresholds(dev_priv, val);
}

dev_priv->rps.cur_freq = val;
-- 
2.8.1

[PATCH v2] drm/i915: Use common RPS scheme for Cherryview

[Chris Wilson]

Apolgies, I changed trees between resending after the first git-send-email
bounced off the wrong address.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2] dma-buf: Wait on the reservation object when sync'ing before CPU access

[Chris Wilson]

Rendering operations to the dma-buf are tracked implicitly via the
reservation_object (dmabuf->resv). This is used to allow poll() to
wait upon outstanding rendering (or just query the current status of
rendering). The dma-buf sync ioctl allows userspace to prepare the
dma-buf for CPU access, which should include waiting upon rendering.
(Some drivers may need to do more work to ensure that the dma-buf mmap
is coherent as well as complete.)

v2: Always wait upon the reservation object implicitly. We choose to do
it after the native handler in case it can do so more efficiently.

Testcase: igt/prime_vgem
Testcase: igt/gem_concurrent_blit # *vgem*
Signed-off-by: Chris Wilson 
Cc: Sumit Semwal 
Cc: Daniel Vetter 
Cc: Eric Anholt 
Cc: linux-media at vger.kernel.org
Cc: dri-devel at lists.freedesktop.org
Cc: linaro-mm-sig at lists.linaro.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/dma-buf/dma-buf.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index ddaee60ae52a..cf04d249a6a4 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -586,6 +586,22 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment 
*attach,
 }
 EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);

+static int __dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
+ enum dma_data_direction direction)
+{
+   bool write = (direction == DMA_BIDIRECTIONAL ||
+ direction == DMA_TO_DEVICE);
+   struct reservation_object *resv = dmabuf->resv;
+   long ret;
+
+   /* Wait on any implicit rendering fences */
+   ret = reservation_object_wait_timeout_rcu(resv, write, true,
+ MAX_SCHEDULE_TIMEOUT);
+   if (ret < 0)
+   return ret;
+
+   return 0;
+}

 /**
  * dma_buf_begin_cpu_access - Must be called before accessing a dma_buf from 
the
@@ -608,6 +624,13 @@ int dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
if (dmabuf->ops->begin_cpu_access)
ret = dmabuf->ops->begin_cpu_access(dmabuf, direction);

+   /* Ensure that all fences are waited upon - but we first allow
+* the native handler the chance to do so more efficiently if it
+* chooses. A double invocation here will be reasonably cheap no-op.
+*/
+   if (ret == 0)
+   ret = __dma_buf_begin_cpu_access(dmabuf, direction);
+
return ret;
 }
 EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
-- 
2.8.1

[PATCH v3 06/11] drm/i915: Enable i915 perf stream for Haswell OA unit

[Chris Wilson]

On Mon, Aug 15, 2016 at 03:41:23PM +0100, Robert Bragg wrote:
>  int __must_check i915_gem_evict_something(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index bd13d08..b4de357 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -134,6 +134,24 @@ static int get_context_size(struct drm_i915_private 
> *dev_priv)
>   return ret;
>  }
>  
> +int i915_gem_context_pin_legacy_rcs_state(struct drm_i915_private *dev_priv,
> +   struct i915_gem_context *ctx)
> +{
> + int ret;
> +
> + lockdep_assert_held(>i915->drm.struct_mutex);
> +
> + ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state,
> + ctx->ggtt_alignment,
> + 0);
> + if (ret)
> + return ret;
> +
> + i915_oa_legacy_context_pin_notify(dev_priv, ctx);
> +
> + return 0;
> +}

I am still not all at all happy with this. I hope the recent changes to
do the vma tracking make it clear.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v3 01/11] drm/i915: Add i915 perf infrastructure

[Chris Wilson]

On Tue, Aug 16, 2016 at 03:59:24PM +0100, Robert Bragg wrote:
>On Mon, Aug 15, 2016 at 3:57 PM, Chris Wilson
>  Alternatively you could follow the standard pattern for read. Dare I ask
>  what is going to go into state that needs the obfuscation?
> 
>I had dug around a bit when I was trying to decide how to handle the
>corner cases here and found some precedent for prioritize reporting any
>data copied over an error for a read().

Reporting completed bytes before the error is correct. I was referring
to going between passing the return value as a mixture of state and return,
when it just appears to be following the usual pattern of read(). i.e. I
could find anything to support why your internal read callback required
a different signature. It looks unusual, so I am expecting it to do
unusual things.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/2] drm: Allow drivers to modify plane_state in prepare_fb/cleanup_fb

[Chris Wilson]

The drivers have to modify the atomic plane state during the prepare_fb
callback so they track allocations, reservations and dependencies for
this atomic operation involving this fb. In particular, how else do we
set the plane->fence from the framebuffer!

Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 4 ++--
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 4 ++--
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 4 ++--
 drivers/gpu/drm/i915/intel_display.c| 4 ++--
 drivers/gpu/drm/i915/intel_drv.h| 4 ++--
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c   | 4 ++--
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c   | 4 ++--
 drivers/gpu/drm/omapdrm/omap_plane.c| 4 ++--
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 ++--
 drivers/gpu/drm/tegra/dc.c  | 4 ++--
 include/drm/drm_modeset_helper_vtables.h| 4 ++--
 11 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c 
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index 146809a97a07..72e6b7dd457b 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -755,7 +755,7 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane 
*p,
 }

 static int atmel_hlcdc_plane_prepare_fb(struct drm_plane *p,
-   const struct drm_plane_state *new_state)
+   struct drm_plane_state *new_state)
 {
/*
 * FIXME: we should avoid this const -> non-const cast but it's
@@ -780,7 +780,7 @@ static int atmel_hlcdc_plane_prepare_fb(struct drm_plane *p,
 }

 static void atmel_hlcdc_plane_cleanup_fb(struct drm_plane *p,
-   const struct drm_plane_state *old_state)
+struct drm_plane_state *old_state)
 {
/*
 * FIXME: we should avoid this const -> non-const cast but it's
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c 
b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
index e50467a0deb0..2a3e92976700 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
@@ -171,13 +171,13 @@ static void fsl_dcu_drm_plane_atomic_update(struct 
drm_plane *plane,

 static void
 fsl_dcu_drm_plane_cleanup_fb(struct drm_plane *plane,
-const struct drm_plane_state *new_state)
+struct drm_plane_state *new_state)
 {
 }

 static int
 fsl_dcu_drm_plane_prepare_fb(struct drm_plane *plane,
-const struct drm_plane_state *new_state)
+struct drm_plane_state *new_state)
 {
return 0;
 }
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c 
b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
index 91188f33b1d9..6417158dad61 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
@@ -818,14 +818,14 @@ static void ade_disable_channel(struct ade_plane *aplane)
 }

 static int ade_plane_prepare_fb(struct drm_plane *plane,
-   const struct drm_plane_state *new_state)
+   struct drm_plane_state *new_state)
 {
/* do nothing */
return 0;
 }

 static void ade_plane_cleanup_fb(struct drm_plane *plane,
-const struct drm_plane_state *old_state)
+struct drm_plane_state *old_state)
 {
/* do nothing */
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 8a203b5f347e..123112c240e0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14439,7 +14439,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
  */
 int
 intel_prepare_plane_fb(struct drm_plane *plane,
-  const struct drm_plane_state *new_state)
+  struct drm_plane_state *new_state)
 {
struct drm_device *dev = plane->dev;
struct drm_framebuffer *fb = new_state->fb;
@@ -14525,7 +14525,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
  */
 void
 intel_cleanup_plane_fb(struct drm_plane *plane,
-  const struct drm_plane_state *old_state)
+  struct drm_plane_state *old_state)
 {
struct drm_device *dev = plane->dev;
struct intel_plane_state *old_intel_state;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1c700b0c3cea..774aab342f40 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1251,9 +1251,9 @@ void intel_finish_page_flip_cs(struct drm_i915_private 
*dev_priv, int pipe);
 void intel_finish_page_flip_mmio(struct drm_i915_private *dev_priv

[PATCH 2/2] drm/i915: Replace intel_plane->wait_req with plane->fence

[Chris Wilson]

Now that we subclass our request from struct fence, we start using the
common primitives more freely and so avoid hand-rolling routines already
provided for by the helpers.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  3 --
 drivers/gpu/drm/i915/intel_display.c  | 52 +++
 drivers/gpu/drm/i915/intel_drv.h  |  1 -
 3 files changed, 5 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c 
b/drivers/gpu/drm/i915/intel_atomic_plane.c
index b82de3072d4f..b41bf380f2ab 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -77,14 +77,12 @@ intel_plane_duplicate_state(struct drm_plane *plane)
struct intel_plane_state *intel_state;

intel_state = kmemdup(plane->state, sizeof(*intel_state), GFP_KERNEL);
-
if (!intel_state)
return NULL;

state = _state->base;

__drm_atomic_helper_plane_duplicate_state(plane, state);
-   intel_state->wait_req = NULL;

return state;
 }
@@ -101,7 +99,6 @@ void
 intel_plane_destroy_state(struct drm_plane *plane,
  struct drm_plane_state *state)
 {
-   WARN_ON(state && to_intel_plane_state(state)->wait_req);
drm_atomic_helper_plane_destroy_state(plane, state);
 }

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 123112c240e0..1b5f653d595b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13944,9 +13944,7 @@ static int intel_atomic_prepare_commit(struct 
drm_device *dev,
   bool nonblock)
 {
struct drm_i915_private *dev_priv = to_i915(dev);
-   struct drm_plane_state *plane_state;
struct drm_crtc_state *crtc_state;
-   struct drm_plane *plane;
struct drm_crtc *crtc;
int i, ret;

@@ -13969,27 +13967,6 @@ static int intel_atomic_prepare_commit(struct 
drm_device *dev,
ret = drm_atomic_helper_prepare_planes(dev, state);
mutex_unlock(>struct_mutex);

-   if (!ret && !nonblock) {
-   for_each_plane_in_state(state, plane, plane_state, i) {
-   struct intel_plane_state *intel_plane_state =
-   to_intel_plane_state(plane_state);
-
-   if (!intel_plane_state->wait_req)
-   continue;
-
-   ret = i915_wait_request(intel_plane_state->wait_req,
-   true, NULL, NULL);
-   if (ret) {
-   /* Any hang should be swallowed by the wait */
-   WARN_ON(ret == -EIO);
-   mutex_lock(>struct_mutex);
-   drm_atomic_helper_cleanup_planes(dev, state);
-   mutex_unlock(>struct_mutex);
-   break;
-   }
-   }
-   }
-
return ret;
 }

@@ -14076,27 +14053,12 @@ static void intel_atomic_commit_tail(struct 
drm_atomic_state *state)
struct drm_crtc_state *old_crtc_state;
struct drm_crtc *crtc;
struct intel_crtc_state *intel_cstate;
-   struct drm_plane *plane;
-   struct drm_plane_state *plane_state;
bool hw_check = intel_state->modeset;
unsigned long put_domains[I915_MAX_PIPES] = {};
unsigned crtc_vblank_mask = 0;
-   int i, ret;
-
-   for_each_plane_in_state(state, plane, plane_state, i) {
-   struct intel_plane_state *intel_plane_state =
-   to_intel_plane_state(plane_state);
-
-   if (!intel_plane_state->wait_req)
-   continue;
-
-   ret = i915_wait_request(intel_plane_state->wait_req,
-   true, NULL, NULL);
-   /* EIO should be eaten, and we can't get interrupted in the
-* worker, and blocking commits have waited already. */
-   WARN_ON(ret);
-   }
+   int i;

+   drm_atomic_helper_wait_for_fences(dev, state);
drm_atomic_helper_wait_for_dependencies(state);

if (intel_state->modeset) {
@@ -14506,9 +14468,9 @@ intel_prepare_plane_fb(struct drm_plane *plane,
}

if (ret == 0) {
-   to_intel_plane_state(new_state)->wait_req =
-   i915_gem_active_get(>last_write,
-   >base.dev->struct_mutex);
+   new_state->fence =
+   _gem_active_get(>last_write,
+
>base.dev->struct_mutex)->fence;
}

return ret;
@@ -14529,7 +14491,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 {
stru

[PATCH] drm: Avoid calling dev_printk(.dev = NULL)

[Chris Wilson]

Since dev_printk likes to print "(NULL device *):" when passed in a NULL
pointer, we have to manually call printk() ourselves.

Fixes: c4e68a583202 ("drm: Introduce DRM_DEV_* log messages")
Signed-off-by: Chris Wilson 
Cc: Eric Engestrom 
Cc: Sean Paul 
---
 drivers/gpu/drm/drm_drv.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index a7f628298365..acf6a5f38920 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -79,8 +79,11 @@ void drm_dev_printk(const struct device *dev, const char 
*level,
vaf.fmt = format;
vaf.va = 

-   dev_printk(level, dev, DRM_PRINTK_FMT, function_name, prefix,
-  );
+   if (dev)
+   dev_printk(level, dev, DRM_PRINTK_FMT, function_name, prefix,
+  );
+   else
+   printk("%s" DRM_PRINTK_FMT, level, function_name, prefix, );

va_end(args);
 }
-- 
2.9.3

[PATCH v4 07/11] drm/i915: advertise available metrics via sysfs

[Chris Wilson]

On Thu, Aug 18, 2016 at 11:42:37PM +0100, Robert Bragg wrote:
> Each metric set is given a sysfs entry like:
> 
> /sys/class/drm/card0/metrics//id
> 
> This allows userspace to enumerate the specific sets that are available
> for the current system. The 'id' file contains an unsigned integer that
> can be used to open the associated metric set via
> DRM_IOCTL_I915_PERF_OPEN. The  is a globally unique ID for a
> specific OA unit register configuration that can be reliably used by
> userspace as a key to lookup corresponding counter meta data and
> normalization equations.
> 
> The guid registry is currently maintained as part of gputop along with
> the XML metric set descriptions and code generation scripts, ref:
> 
>  https://github.com/rib/gputop
>  > gputop-data/guids.xml
>  > scripts/update-guids.py
>  > gputop-data/oa-*.xml
>  > scripts/i915-perf-kernelgen.py
> 
>  $ make -C gputop-data -f Makefile.xml SYSFS=1 WHITELIST=RenderBasic
> 
> Signed-off-by: Robert Bragg 
> ---
>  drivers/gpu/drm/i915/i915_drv.c|  5 +
>  drivers/gpu/drm/i915/i915_drv.h|  4 
>  drivers/gpu/drm/i915/i915_oa_hsw.c | 45 +
>  drivers/gpu/drm/i915/i915_oa_hsw.h |  4 
>  drivers/gpu/drm/i915/i915_perf.c   | 46 
> ++
>  5 files changed, 104 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 92f668e..0f5f51b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1172,6 +1172,9 @@ static void i915_driver_register(struct 
> drm_i915_private *dev_priv)
>* cannot run before the connectors are registered.
>*/
>   intel_fbdev_initial_config_async(dev);
> +
> + /* Depends on sysfs having been initialized */
> + i915_perf_register(dev_priv);

That's only true if drm_dev_register() returns 0. So this needs to be in
that branch, presumably.

> +int
> +i915_perf_init_sysfs_hsw(struct drm_i915_private *dev_priv)
> +{
> + int ret;
> +
> + ret = sysfs_create_group(dev_priv->perf.metrics_kobj, 
> _render_basic);
> + if (ret)
> + goto error_render_basic;
> +
> + return 0;
> +
> +error_render_basic:
> + return ret;
> +}
> +
> +void
> +i915_perf_deinit_sysfs_hsw(struct drm_i915_private *dev_priv)

I'd like these to be consistent with the phase, i.e.

perf_register_sysfs
perf_unregister_sysfs

> +void i915_perf_register(struct drm_i915_private *dev_priv)
> +{
> + if (!dev_priv->perf.initialized)
> + return;
> +
> + /* To be sure we're synchronized with an attempted
> +  * i915_perf_open_ioctl(); considering that we register after
> +  * being exposed to userspace.
> +  */
> + mutex_lock(_priv->perf.lock);
> +
> + dev_priv->perf.metrics_kobj =
> + kobject_create_and_add("metrics",
> +    _priv->drm.primary->kdev->kobj);
> + if (!dev_priv->perf.metrics_kobj)
> + goto exit;
> +
> + if (i915_perf_init_sysfs_hsw(dev_priv)) {

If you say hsw only, I expect to see a local check or comment saying we
are on Haswell.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm: Reject page_flip for !DRIVER_MODESET

[Chris Wilson]

On Sat, Aug 20, 2016 at 12:22:11PM +0200, Daniel Vetter wrote:
> Somehow this one slipped through, which means drivers without modeset
> support can be oopsed (since those also don't call
> drm_mode_config_init, which means the crtc lookup will chase an
> uninitalized idr).
> 
> Reported-by: Alexander Potapenko 
> Cc: Alexander Potapenko 
> Cc: stable at vger.kernel.org
> Signed-off-by: Daniel Vetter 

That explains the oops.

Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm: Wrap direct calls to driver->gem_free_object from CMA

[Chris Wilson]

Since the introduction of (struct_mutex) lockless GEM bo freeing, there
are a pair of driver vfuncs for freeing the GEM bo, of which the driver
may choose to only implement driver->gem_object_free_unlocked (and so
avoid taking the struct_mutex along the free path). However, the CMA GEM
helpers were still calling driver->gem_free_object directly, now NULL,
and promptly dying on the fancy new lockless drivers. Oops.

Robert Foss bisected this to b82caafcf2303 (drm/vc4: Use lockless gem BO
free callback) on his vc4 device, but that just serves as an enabler for
9f0ba539d13ae (drm/gem: support BO freeing without dev->struct_mutex).

Reported-by: Robert Foss 
Fixes: 9f0ba539d13ae (drm/gem: support BO freeing without dev->struct_mutex)
Signed-off-by: Chris Wilson 
Cc: Robert Foss 
Cc: Daniel Vetter 
Cc: Eric Anholt 
Cc: Alex Deucher 
Cc: Lucas Stach 
Cc: stable at vger.kernel.org
---
 drivers/gpu/drm/drm_fb_cma_helper.c  |  2 +-
 drivers/gpu/drm/drm_gem_cma_helper.c | 12 +++-
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c 
b/drivers/gpu/drm/drm_fb_cma_helper.c
index 172cafe11c71..5075fae3c4e2 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -445,7 +445,7 @@ err_cma_destroy:
 err_fb_info_destroy:
drm_fb_helper_release_fbi(helper);
 err_gem_free_object:
-   dev->driver->gem_free_object(>base);
+   drm_gem_object_unreference_unlocked(>base);
return ret;
 }
 EXPORT_SYMBOL(drm_fbdev_cma_create_with_funcs);
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c 
b/drivers/gpu/drm/drm_gem_cma_helper.c
index e1ab008b3f08..1d6c335584ec 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -121,7 +121,7 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct 
drm_device *drm,
return cma_obj;

 error:
-   drm->driver->gem_free_object(_obj->base);
+   drm_gem_object_unreference_unlocked(_obj->base);
return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_create);
@@ -162,18 +162,12 @@ drm_gem_cma_create_with_handle(struct drm_file *file_priv,
 * and handle has the id what user can see.
 */
ret = drm_gem_handle_create(file_priv, gem_obj, handle);
-   if (ret)
-   goto err_handle_create;
-
/* drop reference from allocate - handle holds it now. */
drm_gem_object_unreference_unlocked(gem_obj);
+   if (ret)
+   return ERR_PTR(ret);

return cma_obj;
-
-err_handle_create:
-   drm->driver->gem_free_object(gem_obj);
-
-   return ERR_PTR(ret);
 }

 /**
-- 
2.8.1

[PATCH] drm: Restore double clflush on the last partial cacheline

[Chris Wilson]

This effectively reverts

commit afcd950cafea6e27b739fe7772cbbeed37d05b8b
Author: Chris Wilson 
Date:   Wed Jun 10 15:58:01 2015 +0100

drm: Avoid the double clflush on the last cache line in 
drm_clflush_virt_range()

as we have observed issues with serialisation of the clflush operations
on Baytrail+ Atoms with partial updates. Applying the double flush on the
last cacheline forces that clflush to be ordered with respect to the
previous clflush, and the mfence then protects against prefetches crossing
the clflush boundary.

The same issue can be demonstrated in userspace with igt/gem_exec_flush.

Fixes: afcd950cafea6 (drm: Avoid the double clflush on the last cache...)
Testcase: igt/gem_concurrent_blit
Testcase: igt/gem_partial_pread_pwrite
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92845
Signed-off-by: Chris Wilson 
Cc: dri-devel at lists.freedesktop.org
Cc: Akash Goel 
Cc: Imre Deak 
Cc: Daniel Vetter 
Cc: Jason Ekstrand 
Cc: stable at vger.kernel.org
---
 drivers/gpu/drm/drm_cache.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
index 6743ff7dccfa..7f4a6c550319 100644
--- a/drivers/gpu/drm/drm_cache.c
+++ b/drivers/gpu/drm/drm_cache.c
@@ -136,6 +136,7 @@ drm_clflush_virt_range(void *addr, unsigned long length)
mb();
for (; addr < end; addr += size)
clflushopt(addr);
+   clflushopt(end - 1); /* force serialisation */
mb();
return;
}
-- 
2.8.1

[PATCH v8 11/12] drm/i915: Add more Haswell OA metric sets

[Chris Wilson]

rtised so userspace 
> and
> +  * so shouldn't have been requested
> +  */
> + return -EINVAL;
> + }
> +
> + dev_priv->perf.oa.b_counter_regs =
> + b_counter_config_compute_basic;
> + dev_priv->perf.oa.b_counter_regs_len =
> + ARRAY_SIZE(b_counter_config_compute_basic);
> +
> +     return 0;

>  int
>  i915_perf_register_sysfs_hsw(struct drm_i915_private *dev_priv)
>  {
> @@ -178,9 +685,49 @@ i915_perf_register_sysfs_hsw(struct drm_i915_private 
> *dev_priv)
>   if (ret)
>   goto error_render_basic;
>   }
> + if (get_compute_basic_mux_config(dev_priv, _len)) {

Why not use the derived state in dev_priv->perf.oa.mux_regs? Then we
only expose what is initialised.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 1/2] drm/i915: Remove redundant reprobe in i915_drm_resume

[Chris Wilson]

On Thu, Nov 03, 2016 at 11:42:37AM -0400, Lyude wrote:
> Weine's investigation on benchmarking the suspend/resume process pointed
> out a lot of the time in suspend/resume is being spent reprobing. While
> the reprobing process is a lengthy one for good reason, we don't need to
> hold up the entire suspend/resume process while we wait for it to
> finish. Luckily as it turns out, we already trigger a full connector
> reprobe in i915_hpd_poll_init_work(), so we can just ditch reprobing in
> i915_drm_resume() entirely.
> 
> This won't lead to less time spent resuming just yet since now the
> bottleneck will be waiting for the mode_config lock in
> drm_kms_helper_poll_enable(), since that will be held as long as
> i915_hpd_poll_init_work() is reprobing all of the connectors. But we'll
> address that in the next patch.
> 
> Signed-off-by: Lyude 
> Cc: David Weinehall 
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index bfb2efd..532cc0f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1602,8 +1602,6 @@ static int i915_drm_resume(struct drm_device *dev)
>* notifications.
>* */
>   intel_hpd_init(dev_priv);
> - /* Config may have changed between suspend and resume */
> - drm_helper_hpd_irq_event(dev);

The comment is still apt. This code is known to be broken since it
doesn't detect a change in monitors (e.g. a change in external connectors
from docking) between suspend and resend. We still have to send the uevent.

+   drm_kms_helper_hotplug_event(dev);

which also depends upon us actually reseting the connector->status to
unknown in drm_mode_config_reset(), Daniel!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 1/2] drm/i915: Remove redundant reprobe in i915_drm_resume

[Chris Wilson]

On Thu, Nov 03, 2016 at 12:11:55PM -0400, Lyude Paul wrote:
> On Thu, 2016-11-03 at 12:11 -0400, Lyude Paul wrote:
> > On Thu, 2016-11-03 at 16:02 +0000, Chris Wilson wrote:
> > > 
> > > On Thu, Nov 03, 2016 at 11:42:37AM -0400, Lyude wrote:
> > > > 
> > > > 
> > > > Weine's investigation on benchmarking the suspend/resume process pointed
> > > > out a lot of the time in suspend/resume is being spent reprobing. While
> > > > the reprobing process is a lengthy one for good reason, we don't need to
> > > > hold up the entire suspend/resume process while we wait for it to
> > > > finish. Luckily as it turns out, we already trigger a full connector
> > > > reprobe in i915_hpd_poll_init_work(), so we can just ditch reprobing in
> > > > i915_drm_resume() entirely.
> > > > 
> > > > This won't lead to less time spent resuming just yet since now the
> > > > bottleneck will be waiting for the mode_config lock in
> > > > drm_kms_helper_poll_enable(), since that will be held as long as
> > > > i915_hpd_poll_init_work() is reprobing all of the connectors. But we'll
> > > > address that in the next patch.
> > > > 
> > > > Signed-off-by: Lyude 
> > > > Cc: David Weinehall 
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c | 2 --
> > > >  1 file changed, 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index bfb2efd..532cc0f 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -1602,8 +1602,6 @@ static int i915_drm_resume(struct drm_device *dev)
> > > >     * notifications.
> > > >     * */
> > > >    intel_hpd_init(dev_priv);
> > > > -   /* Config may have changed between suspend and resume */
> > > > -   drm_helper_hpd_irq_event(dev);
> > > 
> > > The comment is still apt. This code is known to be broken since it
> > > doesn't detect a change in monitors (e.g. a change in external connectors
> > > from docking) between suspend and resend. We still have to send the 
> > > uevent.
> > > 
> > > + drm_kms_helper_hotplug_event(dev);
> > 
> > I might not have explained myself very well. The way things should look with
> > this patch is like this:
> > 
> > i915_drm_resume()
> >  -> intel_hpd_init()
> >    -> sets dev_priv->hotplug.poll_enabled to true
> Whoops, s/true/false/
> 
> >    -> schedules dev_priv->hotplug.poll_init_work
> >  -> continue resume…
> > 
> > at the same time:
> > 
> > i915_hpd_poll_init_work() gets scheduled and starts
> >  -> since dev_priv->hotplug.poll_enabled == false, 
> > drm_helper_hpd_irq_event()
> > is called
> >   -> drm_helper_hpd_irq_event() reprobes connectors
> >    -> if anything changed, drm_kms_helper_hotplug_event() gets called.

drm_helper_hpd_irq_event() does not detect a change in monitors, for
example, so there is no uevent in that case.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] dma-buf/fence-array: enable_signaling from wq

[Chris Wilson]

On Thu, Nov 03, 2016 at 04:31:14PM -0400, Rob Clark wrote:
> Currently with fence-array, we have a potential deadlock situation.  If we
> fence_add_callback() on an array-fence, the array-fence's lock is aquired
> first, and in it's ->enable_signaling() callback, it will install cb's on
> it's array-member fences, so the array-member's lock is acquired second.
> 
> But in the signal path, the array-member's lock is acquired first, and the
> array-fence's lock acquired second.

Could mention that this is only an issue if the same fence->lock
appeared more than once in the array. Which is possible.

> diff --git a/drivers/dma-buf/dma-fence-array.c 
> b/drivers/dma-buf/dma-fence-array.c
> index 67eb7c8..274bbb5 100644
> --- a/drivers/dma-buf/dma-fence-array.c
> +++ b/drivers/dma-buf/dma-fence-array.c
> @@ -43,9 +43,10 @@ static void dma_fence_array_cb_func(struct dma_fence *f,
>   dma_fence_put(>base);
>  }
>  
> -static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
> +static void enable_signaling_worker(struct work_struct *w)
>  {
> - struct dma_fence_array *array = to_dma_fence_array(fence);
> + struct dma_fence_array *array =
> + container_of(w, struct dma_fence_array, 
> enable_signaling_worker);
>   struct dma_fence_array_cb *cb = (void *)([1]);
>   unsigned i;
>  
> @@ -63,11 +64,18 @@ static bool dma_fence_array_enable_signaling(struct 
> dma_fence *fence)
>   if (dma_fence_add_callback(array->fences[i], [i].cb,
>  dma_fence_array_cb_func)) {
>   dma_fence_put(>base);
> - if (atomic_dec_and_test(>num_pending))
> - return false;
> + if (atomic_dec_and_test(>num_pending)) {
> + dma_fence_signal(>base);
> + return;
> + }
>   }
>   }
> +}
>  
> +static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
> +{
> + struct dma_fence_array *array = to_dma_fence_array(fence);
> + queue_work(system_unbound_wq, >enable_signaling_worker);

I think you need a dma_fence_get(fence) here with a corresponding put in
the worker. Sadly I still can't poke a hole in the lockdep warning.

What I might suggest is that we try

static bool is_signaled(struct dma_fence *fence)
{
if (test_bit(SIGNALED_BIT, >flags))
return true;

return fence->ops->signaled && fence->ops->signaled(fence);
}

static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
{
struct dma_fence_array *array = to_dma_fence_array(fence);
int num_pending = atomic_read(>num_pending);
int i;

for (i = 0; i < array->num_fences; i++)
if (is_signaled(array->fences[i]) && !--num_pending) {
    atomic_set(>num_pending, 0);
return false;
}

dma_fence_get(>base);
queue_work(system_unbound_wq, >enable_signaling_worker);
}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] dma-buf/fence-array: enable_signaling from wq

[Chris Wilson]

On Thu, Nov 03, 2016 at 07:34:02PM -0400, Rob Clark wrote:
> On Thu, Nov 3, 2016 at 5:41 PM, Chris Wilson  
> wrote:
> > static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
> > {
> > struct dma_fence_array *array = to_dma_fence_array(fence);
> > int num_pending = atomic_read(>num_pending);
> > int i;
> >
> > for (i = 0; i < array->num_fences; i++)
> > if (is_signaled(array->fences[i]) && !--num_pending) {
> > atomic_set(>num_pending, 0);
> > return false;
> > }
> >
> > dma_fence_get(>base);
> > queue_work(system_unbound_wq, >enable_signaling_worker);
> > }
> 
> hmm, I guess just to try to avoid the wq?

Yeah, not all fences are capable of reporting the current status, and
some others may only report signaled() after enable_signaling, but for
i915/nouveau even msm can report the current status without the extra
step. For those it seems worth skipping the wq.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

redraw issues on i915 since 4.9-rc

[Chris Wilson]

On Fri, Nov 04, 2016 at 08:40:47PM +0900, Norbert Preining wrote:
> Dear all,
> 
> since 4.9-rc series started I see heavy redraw problems on i915. Starting
> or resizing for example the digikam window messes up completely the content.
> 
> I have seen this at least since rc2 (I often wait till rc2), and 
> confirm that 4.8.0 does not exhibit these problems.
> 
> A screenshot of the redraw problems is attached.

The fencing looks correct. To me it looks like we filled the vma with
the wrong set of pages from the parent object.

https://cgit.freedesktop.org/drm-intel/ #drm-intel-nightly contains one
interesting patch wrt to the partial vma->pages
https://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-next-queued=d2a84a76a3b970fa32e6eda3d85e7782f831379e
Do you mind testing -nightly to see if I'm barking up the wrong tree?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

redraw issues on i915 since 4.9-rc

[Chris Wilson]

On Sat, Nov 05, 2016 at 02:25:19AM +0900, Norbert Preining wrote:
> Hi Chris,
> 
> > https://cgit.freedesktop.org/drm-intel/
> 
> I pulled from there and rebuild my kernel. Rebooting and everything
> is fine. Looks much better!!!
> 
> > https://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-next-queued=d2a84a76a3b970fa32e6eda3d85e7782f831379e
> 
> Do you want me to test this patch only on top of master? (If it applies!!!)

It won't apply directly, but you could try testing that commit and its
parent to see if my hunch was correct.

Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

redraw issues on i915 since 4.9-rc

[Chris Wilson]

On Sat, Nov 05, 2016 at 01:27:30PM +0900, Norbert Preining wrote:
> > It won't apply directly, but you could try testing that commit and its
> > parent to see if my hunch was correct.
> 
> Unfortunately parent commit was also ok. I am trying to bisect, but
> somehow git tells me something about "...merge commit..." - will see
> how it goes.

Git should be fairly sensible and stick to the drm side of the merges
(just remember that the kernel version will jump around a lot, an issue
if your grub isn't setup to boot the last installed kernel as opposed to
the highest version). Since my guess was wrong, any clues you can find
to point me in the direction will be very useful.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v7 0/3] drm: add explict fencing

[Chris Wilson]

On Tue, Nov 08, 2016 at 03:54:47PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Hi,
> 
> This is yet another version of the DRM fences patches. Please refer
> to the cover letter[1] in a previous version to check for more details.

Explicit fencing is not a superset of the implicit fences. The driver
may be using implicit fences (on a reservation object) to serialise
asynchronous operations wrt to each other (such as dispatching threads
to flush cpu caches to memory, manipulating page tables and the like
before the flip).  Since the user doesn't know about these operations,
they are not included in the explicit fence they provide, at which point
we can't trust their fence to the exclusion of the implicit fences...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v7 0/3] drm: add explict fencing

[Chris Wilson]

On Tue, Nov 08, 2016 at 12:32:56PM +0100, Daniel Vetter wrote:
> On Tue, Nov 08, 2016 at 10:35:08AM +0000, Chris Wilson wrote:
> > On Tue, Nov 08, 2016 at 03:54:47PM +0900, Gustavo Padovan wrote:
> > > From: Gustavo Padovan 
> > > 
> > > Hi,
> > > 
> > > This is yet another version of the DRM fences patches. Please refer
> > > to the cover letter[1] in a previous version to check for more details.
> > 
> > Explicit fencing is not a superset of the implicit fences. The driver
> > may be using implicit fences (on a reservation object) to serialise
> > asynchronous operations wrt to each other (such as dispatching threads
> > to flush cpu caches to memory, manipulating page tables and the like
> > before the flip).  Since the user doesn't know about these operations,
> > they are not included in the explicit fence they provide, at which point
> > we can't trust their fence to the exclusion of the implicit fences...
> 
> My thoughts are that in atomic_check drivers just fill in the fence from
> the reservation_object (i.e. the uapi implicit fencing part). If there's
> any additional work that's queued up in ->prepare_fb then I guess the
> driver needs to track that internally, but _only_ for kernel-internally
> queued work.

That's not a trivial task to work out which of the fence contexts within
the reservation object are required and which are to be replaced by the
explicit fence, esp. when you have to consider external fences.

> The reason for that is that with explicit fencing we want to allow
> userspace to overwrite any existing implicit fences that might hang
> around.

I'm just suggesting the danger of that when userspace doesn't know
everything and the current interfaces do not allow for userspace to know,
we only tell userspace about its own action (more or less).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm: Add stackdepot include for DRM_DEBUG_MM

[Chris Wilson]

0day found that stackdepot.h doesn't get automatically included on all
architectures, so remember to add our #include.

Reported-by: kbuild test robot 
Fixes: 5705670d0463 ("drm: Track drm_mm allocators and show leaks on shutdown")
Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
---
 drivers/gpu/drm/drm_mm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 89b891a4847f..632473beb40c 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -105,6 +105,8 @@ static struct drm_mm_node 
*drm_mm_search_free_in_range_generic(const struct drm_
enum drm_mm_search_flags flags);

 #ifdef CONFIG_DRM_DEBUG_MM
+#include 
+
 #define STACKDEPTH 32
 #define BUFSZ 4096

-- 
2.10.2

[PATCH v7 0/3] drm: add explict fencing

[Chris Wilson]

On Tue, Nov 08, 2016 at 01:43:40PM +0100, Daniel Vetter wrote:
> On Tue, Nov 08, 2016 at 11:45:51AM +0000, Chris Wilson wrote:
> > On Tue, Nov 08, 2016 at 12:32:56PM +0100, Daniel Vetter wrote:
> > > On Tue, Nov 08, 2016 at 10:35:08AM +, Chris Wilson wrote:
> > > > On Tue, Nov 08, 2016 at 03:54:47PM +0900, Gustavo Padovan wrote:
> > > > > From: Gustavo Padovan 
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > This is yet another version of the DRM fences patches. Please refer
> > > > > to the cover letter[1] in a previous version to check for more 
> > > > > details.
> > > > 
> > > > Explicit fencing is not a superset of the implicit fences. The driver
> > > > may be using implicit fences (on a reservation object) to serialise
> > > > asynchronous operations wrt to each other (such as dispatching threads
> > > > to flush cpu caches to memory, manipulating page tables and the like
> > > > before the flip).  Since the user doesn't know about these operations,
> > > > they are not included in the explicit fence they provide, at which point
> > > > we can't trust their fence to the exclusion of the implicit fences...
> > > 
> > > My thoughts are that in atomic_check drivers just fill in the fence from
> > > the reservation_object (i.e. the uapi implicit fencing part). If there's
> > > any additional work that's queued up in ->prepare_fb then I guess the
> > > driver needs to track that internally, but _only_ for kernel-internally
> > > queued work.
> > 
> > That's not a trivial task to work out which of the fence contexts within
> > the reservation object are required and which are to be replaced by the
> > explicit fence, esp. when you have to consider external fences.
> 
> Hm, what kind of async kernel tasks are you thinking off? Atm I don't know
> of anyone who does e.g. clflush through the gpu. And ttm bo placement
> moves for display should be explicit enough that drivers will deal with
> them correctly. At least that seems to have been the conclusion from the
> long amdgpu thread.

Now that we (i915) serialise on an reservation_object (obj->resv), we
have floated ideas to use that to serialise async tasks (such as
offloading the 100ms clflush to a (cpu) worker, a gpu task would pose a
similar problem with a fence inserted that is not exposed to userspace).
Also tempted to look at using async tasks + fences to do GTT updates
but that is not a common pain point at the moment, and cases where it
is the GTT thrashing itself is the issue.

So how does i915 deal with ttm bo fences?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm: Add stackdepot include for DRM_DEBUG_MM

[Chris Wilson]

On Tue, Nov 08, 2016 at 01:46:15PM +0100, Daniel Vetter wrote:
> On Tue, Nov 08, 2016 at 11:56:01AM +0000, Chris Wilson wrote:
> > 0day found that stackdepot.h doesn't get automatically included on all
> > architectures, so remember to add our #include.
> > 
> > Reported-by: kbuild test robot 
> > Fixes: 5705670d0463 ("drm: Track drm_mm allocators and show leaks on 
> > shutdown")
> > Signed-off-by: Chris Wilson 
> > Cc: Daniel Vetter 
> 
> Thx for the fast fixup, applied to drm-misc.

And I misread the kbuild result. Apparently we all build drm.ko as a
builtin and kbuild doesn't.

depot_save_stack is not exported :(

Is

-   depends on DRM
+   depends on DRM=y

a good temporary? Will have to munge i915 similarly. :(
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/2] drm/i915: Restrict DRM_DEBUG_MM automatic selection

[Chris Wilson]

DRM_DEBUG_MM is only valid if the DRM.ko is builtin as currently
depot_save_stack is not exported.

Fixes: 5c7fcf2db027 ("drm/i915: Enable drm_mm debug when enabling 
DRM_I915_DEBUG")
Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/Kconfig.debug | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/Kconfig.debug 
b/drivers/gpu/drm/i915/Kconfig.debug
index 69edf196ed03..51ba630a134b 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -21,7 +21,7 @@ config DRM_I915_DEBUG
 select PREEMPT_COUNT
 select X86_MSR # used by igt/pm_rpm
 select DRM_VGEM # used by igt/prime_vgem (dmabuf interop checks)
-select DRM_DEBUG_MM
+select DRM_DEBUG_MM if DRM=y
 default n
 help
   Choose this option to turn on extra driver debugging that may affect
-- 
2.10.2

[PATCH 2/2] drm: Restrict stackdepot usage to builtin drm.ko

[Chris Wilson]

I misread the kbuild result thinking that we had missed the include
(which we had for completeness anyway), what kbuild was actually warning
me about was that depot_save_stack was not exported.

Temporarily fix this by only selecting STACKDEPOT iff drm.ko is builtin

Reported-by: kbuild test robot 
Fixes: 5705670d0463 ("drm: Track drm_mm allocators and show leaks on shutdown")
Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 25e8e3793d29..d6ee0592b625 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -36,7 +36,7 @@ config DRM_DP_AUX_CHARDEV
 config DRM_DEBUG_MM
bool "Insert extra checks and debug info into the DRM range managers"
default n
-   depends on DRM
+   depends on DRM=y
select STACKDEPOT
help
  Enable allocation tracking of memory manager and leak detection on
-- 
2.10.2

[PATCH] drm/i915: avoid harmless empty-body warning

[Chris Wilson]

On Tue, Nov 08, 2016 at 02:58:17PM +0100, Arnd Bergmann wrote:
> The newly added assert_kernel_context_is_current introduces a warning
> when built with W=1:
> 
> drivers/gpu/drm/i915/i915_gem.c: In function 
> ‘assert_kernel_context_is_current’:
> drivers/gpu/drm/i915/i915_gem.c:4417:63: error: suggest braces around empty 
> body in an ‘else’ statement [-Werror=empty-body]
> 
> Changing the GEM_BUG_ON() macro from an empty definition to "do { } while (0)"
> makes the macro more robust to use and avoids the warning.
> 
> Fixes: 3033acab07f9 ("drm/i915: Queue the idling context switch after all 
> other timelines")
> Signed-off-by: Arnd Bergmann 
Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[drm-intel:topic/drm-misc 1/2] backtracetest.c:undefined reference to `save_stack_trace'

[Chris Wilson]

On Wed, Nov 09, 2016 at 10:01:37PM +0800, kbuild test robot wrote:
> tree:   git://anongit.freedesktop.org/drm-intel topic/drm-misc
> head:   77d150b90d58ae6a43bf67af28feb26384d06cd6
> commit: cd456f8d06d2036e1e013136c3fbf5721d04f6d7 [1/2] drm: Restrict 
> stackdepot usage to builtin drm.ko
> config: m68k-allyesconfig (attached as .config)
> compiler: m68k-linux-gcc (GCC) 4.9.0
> reproduce:
> wget 
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
>  -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> git checkout cd456f8d06d2036e1e013136c3fbf5721d04f6d7
> # save the attached .config to linux build tree
> make.cross ARCH=m68k 
> 
> All errors (new ones prefixed by >>):
> 
>kernel/built-in.o: In function `backtrace_regression_test':
> >> backtracetest.c:(.text+0x550d4): undefined reference to `save_stack_trace'
>mm/built-in.o: In function `set_track':
>slub.c:(.text+0x41414): undefined reference to `save_stack_trace'
>drivers/built-in.o: In function `save_stack.isra.7':
> >> drm_mm.c:(.text+0x16be82): undefined reference to `save_stack_trace'

Anyone got an idea for this one? m68k is missing save_stack_trace().
There's no arch CONFIG_HAS_SAVE_STACK_TRACE, it looks like an oversight
in arch/m68k.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm: Make DRM_DEBUG_MM depend on STACKTRACE_SUPPORT

[Chris Wilson]

0day continues to complain about trying to save a stacktrace for the
users of the drm_mm range allocator. This time, it is that m68k has no
save_stack_trace(), which is apparently guarded by STACKTRACE_SUPPORT.
Make it depend so!

Reported-by: kbuild test robot 
Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
---
 drivers/gpu/drm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index dd3d9ae97fe4..9f10c148a4b0 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -37,6 +37,7 @@ config DRM_DEBUG_MM
bool "Insert extra checks and debug info into the DRM range managers"
default n
depends on DRM=y
+   depends on STACKTRACE_SUPPORT
select STACKDEPOT
help
  Enable allocation tracking of memory manager and leak detection on
-- 
2.10.2

4.9-rc1 lockdep warning suggesting a deadlock between nouveau and i915 with prime video outputs active

[Chris Wilson]

On Wed, Nov 09, 2016 at 06:02:40PM +0100, Hans de Goede wrote:
> Hi,
> 
> I noticed this while testing the displayport output attached
> to the nvidia GPU on a Thinkpad P50 while the intel GPU
> was driving the LCD panel (and was the primary GPU according
> to Xorg). This was while logging into gnome-3 with Xorg-1.19-rc2,
> from gdm (also running on top of Xorg-1.19-rc2).

Worth trying


diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 6efdba4993fc..3e4dc782812d 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -505,7 +505,9 @@ int drm_dev_init(struct drm_device *dev,

spin_lock_init(>buf_lock);
spin_lock_init(>event_lock);
-   mutex_init(>struct_mutex);
+   __mutex_init(>struct_mutex,
+"struct_mutex",
+>struct_mutex_class);
mutex_init(>filelist_mutex);
mutex_init(>ctxlist_mutex);
mutex_init(>master_mutex);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index ae49c3174d24..ed2e53534c2e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -808,6 +808,7 @@ struct drm_device {

/** \name Locks */
/*@{ */
+   struct lock_class_key struct_mutex_class;
struct mutex struct_mutex;  /**< For others */
struct mutex master_mutex;  /**< For drm_minor::master and 
drm_file::is_master */
/*@} */

to test for a false positive first.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2] drm: Add missing mutex_destroy in drm_dev_init/release

[Chris Wilson]

On Thu, Nov 10, 2016 at 03:50:35PM +0200, Joonas Lahtinen wrote:
> Add 3 missing mutex_destroy to drm_dev_init teardown and
> drm_dev_release.
> 
> v2:
> - Also include drm_dev_release
> 
> Signed-off-by: Joonas Lahtinen 
Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/2] drm/i915: Update i915_driver_load kerneldoc

[Chris Wilson]

On Thu, Nov 10, 2016 at 03:36:34PM +0200, Joonas Lahtinen wrote:
> Update i915_driver_load kerneldoc to match code.
> 
> Cc: Chris Wilson 
> Signed-off-by: Joonas Lahtinen 
Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] dma-buf: Replace reservation shared fence array with a compressed radix tree

[Chris Wilson]

The primary operation on the shared fence arrays is insertion and
retrieval.  Retrieval is reasonably fast, as we just copy the array, but
insertion into the shared fence array is slow as we must iterate over all
current fences to discard an older fence.  (Note also that since the
array is not autopruning, we cannot hook in the fence signal callback due
to locking constraints, thus it can grow very, very large.)  This, coupled
with the atomics to acquire and release the shared fence, causes a severe
performance degradation and scaling bottleneck, as felt by i915.ko in
commit d07f0e59b2c7 ("drm/i915: Move GEM activity tracking into a common
struct reservation_object").  To speed up insertion, we want a direct
replacement of the per-context slot. Enter the radix tree.

The kernel already has a couple of options for integer radix trees, but
both are not suitable for us for a couple of reasons.  First we need a
per-tree preallocation in order to ensure that adding a fence does not
fail.  Both radixtree and idr only offer preallocation under a preempt
disabled critical section, unsuitable for our existing users.  idr has
an internal __idr_pre_get that could be exported - but idr does not yet
support inserting entries at an arbitrary location.  Secondly, neither
tree supports full u64 integers as used by the fence context identifiers
(though radixtree would support that on 64bit platforms).  And finally,
if we ignore the API shortcomings, radixtree still appears too high in
the profiles!

So what are our requirements for the shared fence array?

- guaranteed insertion
- fast insertion
- RCU-safe, fast traversal
- 64bit identifiers

To guarantee insertion, we need to preallocate enough storage to satisfy
a later insertion.  The reservation object API requires every
add_shared_fence to preceded by a call to reserve_shared_fence.  For an
uncompressed radix tree we would need to preallocate enough layers to cover
the full 64bit height, but with a compressed radix tree we only require a
maximum of 2 spare layers.

The compressed tree also offers a useful property wrt to the pattern of
fences used.  The fence contexts are allocated as a pure cyclic atomic64,
i.e. it is sparse and ever incrementing.  However, timelines tend to
cluster (i.e. they will allocate a cluster of fence contexts for
themselves and primarily use these).  So we may see that we have a
mixture of low fence contents and a group of very high contexts (e.g.
a group at id<16 and a group at id>65536).  The compressed tree avoids
having to allocate the intermediate layers (reducing the pointer dancing
on lookup and traversal) - still not as dense as the current compact
array, but under typical conditions as good as.  (However, the current
array requires contiguous allocations - a scarce resource for the ever
expanding array!)  The density could be improved by switching from a
simple dangerously wrapping atomic64 to an ida for fence context
allocation.

Signed-off-by: Chris Wilson 
Cc: Sumit Semwal 
Cc: Alex Deucher 
Cc: "Christian König" 
Cc: David Airlie 
Cc: Lucas Stach 
Cc: Russell King <linux+etnaviv at armlinux.org.uk>
Cc: Christian Gmeiner 
Cc: Daniel Vetter 
Cc: Jani Nikula 
Cc: Rob Clark 
Cc: Ben Skeggs 
Cc: Chunming Zhou 
Cc: Ken Wang 
Cc: Monk Liu 
Cc: Sinclair Yeh 
Cc: Thomas Hellstrom 
Cc: "Michel Dänzer" 
Cc: Flora Cui 
Cc: dri-devel at lists.freedesktop.org
---
 drivers/dma-buf/Kconfig|  13 +
 drivers/dma-buf/Makefile   |   1 +
 drivers/dma-buf/dma-buf.c  |  35 +-
 drivers/dma-buf/reservation.c  | 650 +
 drivers/dma-buf/test-reservation.c | 308 
 drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c   |  14 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem.c  |  15 +-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c  |  14 +-
 drivers/gpu/drm/i915/i915_gem.c|  17 +-
 drivers/gpu/drm/msm/msm_gem.c  |  29 +-
 drivers/gpu/drm/nouveau/nouveau_fence.c|  15 +-
 drivers/gpu/drm/nouveau/nouveau_gem.c  |  21 +-
 drivers/gpu/drm/qxl/qxl_debugfs.c  |   9 +-
 drivers/gpu/drm/radeon/radeon_sync.c   |  12 +-
 drivers/gpu/drm/ttm/ttm_bo.c   |  10 +-
 include/linux/reservation.h| 154 --
 tools/testing/selftests/dma-buf/reservation.sh |  11 +
 17 files changed, 856 insertions(+), 472 deletions(-)
 create mode 100644 drivers/dma-buf/test-reservation.c
 create mode 100644 tools/testing/selftests/dma-buf/reservation.sh

diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index ed3b785bae37..c74481c715b8 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -30,4 +30,17 @@ config SW_SYNC
  WARNING: improper use of this can result in deadlocking kernel
  drivers from userspace. Intended for test and debug only.

+co

[PATCH] dma-buf: Use fence_get_rcu_safe() for retrieving the exclusive fence

[Chris Wilson]

The current code is subject to a race where we may try to acquire a
reference on a stale fence:

[13703.335118] WARNING: CPU: 1 PID: 14975 at ./include/linux/kref.h:46 
i915_gem_object_wait+0x1a3/0x1c0
[13703.335184] Modules linked in:
[13703.335202] CPU: 1 PID: 14975 Comm: gem_concurrent_ Not tainted 4.9.0-rc4+ 
#26
[13703.335216] Hardware name:  /, BIOS 
PYBSWCEL.86A.0027.2015.0507.1758 05/07/2015
[13703.335233]  c90002f5bcc8 812807de  

[13703.335257]  c90002f5bd08 81073811 002e8000 
88026bf7c780
[13703.335279]  7fff 0001 88027045a550 
88026bf7c780
[13703.335301] Call Trace:
[13703.335316]  [] dump_stack+0x4d/0x6f
[13703.335331]  [] __warn+0xc1/0xe0
[13703.335343]  [] warn_slowpath_null+0x18/0x20
[13703.335355]  [] i915_gem_object_wait+0x1a3/0x1c0
[13703.335367]  [] i915_gem_set_domain_ioctl+0xcc/0x330
[13703.335386]  [] drm_ioctl+0x1cb/0x410
[13703.335400]  [] ? 
i915_gem_obj_prepare_shmem_write+0x1d0/0x1d0
[13703.335416]  [] ? drm_ioctl+0x2bb/0x410
[13703.335429]  [] do_vfs_ioctl+0x8f/0x5c0
[13703.335442]  [] SyS_ioctl+0x3c/0x70
[13703.335456]  [] entry_SYSCALL_64_fastpath+0x17/0x98
[13703.335558] ---[ end trace fd24176416ba6981 ]---
[13703.382778] general protection fault:  [#1] SMP
[13703.382802] Modules linked in:
[13703.382816] CPU: 1 PID: 14967 Comm: gem_concurrent_ Tainted: GW  
 4.9.0-rc4+ #26
[13703.382828] Hardware name:  /, BIOS 
PYBSWCEL.86A.0027.2015.0507.1758 05/07/2015
[13703.382841] task: 880275458000 task.stack: c90002f18000
[13703.382849] RIP: 0010:[]  [] 
i915_gem_request_retire+0x2b4/0x320
[13703.382870] RSP: 0018:c90002f1bbc8  EFLAGS: 00010293
[13703.382878] RAX: dead0200 RBX: 88026bf7dce8 RCX: dead0100
[13703.382887] RDX: dead0100 RSI: 88026bf7c930 RDI: 88026bf7dd00
[13703.382897] RBP: c90002f1bbf8 R08:  R09: 88026b89a000
[13703.382905] R10: 0001 R11: 88026bbe8fe0 R12: 88026bf7c000
[13703.382913] R13: 880275af8000 R14: 88026bf7c180 R15: dead0200
[13703.382922] FS:  7f89e787d740() GS:88027fd0() 
knlGS:
[13703.382934] CS:  0010 DS:  ES:  CR0: 80050033
[13703.382942] CR2: 7f9053d2e000 CR3: 00026d414000 CR4: 001006e0
[13703.382951] Stack:
[13703.382958]  880275413000 c90002f1bde8 880275af8000 
880274e8a600
[13703.382976]  880276a06000 c90002f1bde8 c90002f1bc38 
813b48c5
[13703.382995]  c90002f1bc00 c90002f1bde8 88026972a440 

[13703.383021] Call Trace:
[13703.383032]  [] i915_gem_request_alloc+0xa5/0x350
[13703.383043]  [] i915_gem_do_execbuffer.isra.41+0x7b3/0x18b0
[13703.383055]  [] ? i915_gem_object_get_sg+0x25c/0x2b0
[13703.383065]  [] ? i915_gem_object_get_page+0x1d/0x50
[13703.383076]  [] ? i915_gem_pwrite_ioctl+0x66c/0x6d0
[13703.383086]  [] i915_gem_execbuffer2+0x95/0x1e0
[13703.383096]  [] drm_ioctl+0x1cb/0x410
[13703.383105]  [] ? i915_gem_execbuffer+0x2d0/0x2d0
[13703.383117]  [] ? hrtimer_start_range_ns+0x1a0/0x310
[13703.383128]  [] do_vfs_ioctl+0x8f/0x5c0
[13703.383140]  [] ? SyS_timer_settime+0x118/0x1a0
[13703.383150]  [] SyS_ioctl+0x3c/0x70
[13703.383162]  [] entry_SYSCALL_64_fastpath+0x17/0x98
[13703.383172] Code: 49 39 c6 48 8d 70 e8 48 8d 5f e8 75 16 eb 47 48 8d 43 18 
48 8b 53 18 48 89 de 49 39 c6 48 8d 5a e8 74 33 48 8b 56 08 48 8b 46 10 <48> 89 
42 08 48 89 10 f6 46 38 01 48 89 4e 08 4c 89 7e 10 74 cf
[13703.383557] RIP  [] i915_gem_request_retire+0x2b4/0x320
[13703.383570]  RSP 
[13703.383586] ---[ end trace fd24176416ba6982 ]---

This is fixed by using the kref_get_unless_zero() as a full memory
barrier to validate the fence is still the current exclusive fence before
returning it back to the caller. (Note the fix only requires using
dma_fence_get_rcu() and correct handling, but we may as well use the
helper rather than inline equivalent code.)

Signed-off-by: Chris Wilson 
Cc: Sumit Semwal seq);
+
+   if (!rcu_access_pointer(obj->fence_excl))
+   return NULL;
+
rcu_read_lock();
-   fence = rcu_dereference(obj->fence_excl);
-   if (read_seqcount_retry(>seq, seq)) {
-   rcu_read_unlock();
-   goto retry;
-   }
-   fence = dma_fence_get(fence);
+   fence = dma_fence_get_rcu_safe(>fence_excl);
rcu_read_unlock();
+
return fence;
 }

-- 
2.10.2

[PATCH] dma-buf: Use fence_get_rcu_safe() for retrieving the exclusive fence

[Chris Wilson]

On Mon, Nov 14, 2016 at 11:55:40AM +, Chris Wilson wrote:
> The current code is subject to a race where we may try to acquire a
> reference on a stale fence:

>From i915.ko pov, this
Fixes: d07f0e59b2c7 ("drm/i915: Move GEM activity tracking into a common struct 
reservation_object")
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v11 2/3] drm/fence: add fence timeline to drm_crtc

[Chris Wilson]

On Tue, Nov 15, 2016 at 10:57:35AM +0900, Gustavo Padovan wrote:
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 11780a9..0870de1 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -32,6 +32,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -739,9 +741,52 @@ struct drm_crtc {
>*/
>   struct drm_crtc_crc crc;
>  #endif
> +
> + /**
> +  * @fence_context:
> +  *
> +  * timeline context used for fence operations.
> +  */
> + unsigned int fence_context;
> +
> + /**
> +  * @fence_lock:
> +  *
> +  * spinlock to protect the fences in the fence_context.
> +  */
> +
> + spinlock_t fence_lock;
> + /**
> +  * @fence_seqno:
> +  *
> +  * Seqno variable used as monotonic counter for the fences
> +  * created on the CRTC's timeline.
> +  */
> + unsigned long fence_seqno;
> +
> + /**
> +  * @timeline_name:
> +  *
> +  * The name of the CRTC's fence timeline.
> +  */
> + char timeline_name[32];
>  };
>  
>  /**
> + * dma_crtc_fence_ops - fence ops for the drm_crtc timeline
> + *
> + * It contains the dma_fence_ops that should be called by the dma_fence
> + * code. CRTC core should use this ops when initializing fences.
> + */
> +extern const struct dma_fence_ops drm_crtc_fence_ops;
> +
> +static inline struct drm_crtc *fence_to_crtc(struct dma_fence *fence)
> +{
> + BUG_ON(fence->ops != _crtc_fence_ops);
> + return container_of(fence->lock, struct drm_crtc, fence_lock);
> +}

If you are planning to export this for use by drivers, you are missing
the EXPORT_SYMBOL(drm_crtc_fence_ops).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v11 2/3] drm/fence: add fence timeline to drm_crtc

[Chris Wilson]

On Tue, Nov 15, 2016 at 08:25:55AM +, Chris Wilson wrote:
> On Tue, Nov 15, 2016 at 10:57:35AM +0900, Gustavo Padovan wrote:
> >  /**
> > + * dma_crtc_fence_ops - fence ops for the drm_crtc timeline
> > + *
> > + * It contains the dma_fence_ops that should be called by the dma_fence
> > + * code. CRTC core should use this ops when initializing fences.
> > + */
> > +extern const struct dma_fence_ops drm_crtc_fence_ops;
> > +
> > +static inline struct drm_crtc *fence_to_crtc(struct dma_fence *fence)
> > +{
> > +   BUG_ON(fence->ops != _crtc_fence_ops);
> > +   return container_of(fence->lock, struct drm_crtc, fence_lock);
> > +}
> 
> If you are planning to export this for use by drivers, you are missing
> the EXPORT_SYMBOL(drm_crtc_fence_ops).

My suggestion would not to have the BUG_ON() here (saves one
checkpatch.pl complaint in exchange for a slightly more mysterious GPF).
Also please consider calling this dma_fence_to_crtc() as otherwise it
conflicts with those with plans to use struct fences on their
crtc/states.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v11 2/3] drm/fence: add fence timeline to drm_crtc

[Chris Wilson]

On Tue, Nov 15, 2016 at 05:42:35PM +0900, Gustavo Padovan wrote:
> 2016-11-15 Chris Wilson :
> 
> > On Tue, Nov 15, 2016 at 10:57:35AM +0900, Gustavo Padovan wrote:
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index 11780a9..0870de1 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -32,6 +32,8 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -739,9 +741,52 @@ struct drm_crtc {
> > >*/
> > >   struct drm_crtc_crc crc;
> > >  #endif
> > > +
> > > + /**
> > > +  * @fence_context:
> > > +  *
> > > +  * timeline context used for fence operations.
> > > +  */
> > > + unsigned int fence_context;
> > > +
> > > + /**
> > > +  * @fence_lock:
> > > +  *
> > > +  * spinlock to protect the fences in the fence_context.
> > > +  */
> > > +
> > > + spinlock_t fence_lock;
> > > + /**
> > > +  * @fence_seqno:
> > > +  *
> > > +  * Seqno variable used as monotonic counter for the fences
> > > +  * created on the CRTC's timeline.
> > > +  */
> > > + unsigned long fence_seqno;
> > > +
> > > + /**
> > > +  * @timeline_name:
> > > +  *
> > > +  * The name of the CRTC's fence timeline.
> > > +  */
> > > + char timeline_name[32];
> > >  };
> > >  
> > >  /**
> > > + * dma_crtc_fence_ops - fence ops for the drm_crtc timeline
> > > + *
> > > + * It contains the dma_fence_ops that should be called by the dma_fence
> > > + * code. CRTC core should use this ops when initializing fences.
> > > + */
> > > +extern const struct dma_fence_ops drm_crtc_fence_ops;
> > > +
> > > +static inline struct drm_crtc *fence_to_crtc(struct dma_fence *fence)
> > > +{
> > > + BUG_ON(fence->ops != _crtc_fence_ops);
> > > + return container_of(fence->lock, struct drm_crtc, fence_lock);
> > > +}
> > 
> > If you are planning to export this for use by drivers, you are missing
> > the EXPORT_SYMBOL(drm_crtc_fence_ops).
> 
> Drivers should not be using this, at least for now.

You've put into a central header, with kerneldoc on the ops, just inviting
people to use it...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 04/10] drm: Extract drm_drv.h

[Chris Wilson]

y locking schemes. Use this hook instead of @gem_free_object.
> -  */
> - void (*gem_free_object_unlocked) (struct drm_gem_object *obj);
> -
> - int (*gem_open_object) (struct drm_gem_object *, struct drm_file *);
> - void (*gem_close_object) (struct drm_gem_object *, struct drm_file *);
> -
> - /**
> -  * Hook for allocating the GEM object struct, for use by core
> -  * helpers.
> -  */
> - struct drm_gem_object *(*gem_create_object)(struct drm_device *dev,
> - size_t size);
> -
> - /* prime: */
> - /* export handle -> fd (see drm_gem_prime_handle_to_fd() helper) */
> - int (*prime_handle_to_fd)(struct drm_device *dev, struct drm_file 
> *file_priv,
> - uint32_t handle, uint32_t flags, int *prime_fd);
> - /* import fd -> handle (see drm_gem_prime_fd_to_handle() helper) */
> - int (*prime_fd_to_handle)(struct drm_device *dev, struct drm_file 
> *file_priv,
> - int prime_fd, uint32_t *handle);
> - /* export GEM -> dmabuf */
> - struct dma_buf * (*gem_prime_export)(struct drm_device *dev,
> - struct drm_gem_object *obj, int flags);
> - /* import dmabuf -> GEM */
> - struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
> - struct dma_buf *dma_buf);
> - /* low-level interface used by drm_gem_prime_{import,export} */
> - int (*gem_prime_pin)(struct drm_gem_object *obj);
> - void (*gem_prime_unpin)(struct drm_gem_object *obj);
> - struct reservation_object * (*gem_prime_res_obj)(
> - struct drm_gem_object *obj);
> - struct sg_table *(*gem_prime_get_sg_table)(struct drm_gem_object *obj);
> - struct drm_gem_object *(*gem_prime_import_sg_table)(
> - struct drm_device *dev,
> - struct dma_buf_attachment *attach,
> - struct sg_table *sgt);
> - void *(*gem_prime_vmap)(struct drm_gem_object *obj);
> - void (*gem_prime_vunmap)(struct drm_gem_object *obj, void *vaddr);
> - int (*gem_prime_mmap)(struct drm_gem_object *obj,
> - struct vm_area_struct *vma);
> -
> - /* vga arb irq handler */
> - void (*vgaarb_irq)(struct drm_device *dev, bool state);
> -
> - /* dumb alloc support */
> - int (*dumb_create)(struct drm_file *file_priv,
> -struct drm_device *dev,
> -struct drm_mode_create_dumb *args);
> - int (*dumb_map_offset)(struct drm_file *file_priv,
> -struct drm_device *dev, uint32_t handle,
> -uint64_t *offset);
> - int (*dumb_destroy)(struct drm_file *file_priv,
> - struct drm_device *dev,
> - uint32_t handle);
> -
> - /* Driver private ops for this object */
> - const struct vm_operations_struct *gem_vm_ops;
> -
> - int major;
> - int minor;
> - int patchlevel;
> - char *name;
> - char *desc;
> - char *date;
> -
> - u32 driver_features;
> - int dev_priv_size;
> - const struct drm_ioctl_desc *ioctls;
> - int num_ioctls;
> - const struct file_operations *fops;
> -
> - /* List of devices hanging off this driver with stealth attach. */
> - struct list_head legacy_dev_list;
> -};
> -
>  enum drm_minor_type {
>   DRM_MINOR_PRIMARY,
>   DRM_MINOR_CONTROL,
> @@ -1007,11 +727,6 @@ void drm_clflush_virt_range(void *addr, unsigned long 
> length);
>   * DMA quiscent + idle. DMA quiescent usually requires the hardware lock.
>   */
>  
> -/* drm_drv.c */
> -void drm_put_dev(struct drm_device *dev);
> -void drm_unplug_dev(struct drm_device *dev);
> -extern unsigned int drm_debug;
> -
>       /* Debugfs support */
>  #if defined(CONFIG_DEBUG_FS)
>  extern int drm_debugfs_create_files(const struct drm_info_list *files,
> @@ -1064,18 +779,6 @@ extern void drm_pci_free(struct drm_device *dev, struct 
> drm_dma_handle * dmah);
>  extern void drm_sysfs_hotplug_event(struct drm_device *dev);
>  
>  
> -struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> -  struct device *parent);
> -int drm_dev_init(struct drm_device *dev,
> -  struct drm_driver *driver,
> -  struct device *parent);
> -void drm_dev_ref(struct drm_device *dev);
> -void drm_dev_unref(struct drm_device *dev);
> -int drm_dev_register(struct drm_device *dev, unsigned long flags);
> -void drm_dev_unregister(struct drm_device *dev);
> -
> -struct drm_minor *drm_minor_acquire(unsigned int minor_id);
> -void drm_minor_release(struct drm_minor *minor);
>  
>  /*@}*/
>  
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> new file mode 100644
> index ..98f930a76e6d
> --- /dev/null
> +++ b/include/drm/drm_drv.h
> @@ -0,0 +1,337 @@
> +/*
> + * Copyright 2016 Intel Corp.

Careful, it's a mix of some new and lots old. To be on the safe side, it
should retain all the copyright statements of the original.

Otherwise, pretty sure it was mechanical,
Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 06/10] drm: Consolidate dumb buffer docs

[Chris Wilson]

On Mon, Nov 14, 2016 at 12:58:21PM +0100, Daniel Vetter wrote:
> Put the callback docs into struct drm_driver, and the small overview
> into a DOC comment.
> 
> Signed-off-by: Daniel Vetter 
Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 08/10] drm: Extract drm_mode_config.[hc]

[Chris Wilson]

On Mon, Nov 14, 2016 at 12:58:23PM +0100, Daniel Vetter wrote:
> And shuffle the kernel-doc structure a bit since drm_crtc.[hc] now
> only contains CRTC-related functions and structures.
> 
> Signed-off-by: Daniel Vetter 
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
> b/drivers/gpu/drm/drm_crtc_internal.h
> index 3d23a473ec35..dad212140d56 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -40,18 +40,25 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>   int x, int y,
>   const struct drm_display_mode *mode,
>   const struct drm_framebuffer *fb);
> -
> -void drm_fb_release(struct drm_file *file_priv);
> +int drm_crtc_register_all(struct drm_device *dev);
> +void drm_crtc_unregister_all(struct drm_device *dev);
>  
>  /* IOCTLs */
> -int drm_mode_getresources(struct drm_device *dev,
> -   void *data, struct drm_file *file_priv);
>  int drm_mode_getcrtc(struct drm_device *dev,
>void *data, struct drm_file *file_priv);
>  int drm_mode_setcrtc(struct drm_device *dev,
>void *data, struct drm_file *file_priv);
>  
> +
> +/* drm_mode_config.c */
> +/* IOCTLs */
> +int drm_mode_getresources(struct drm_device *dev,
> +   void *data, struct drm_file *file_priv);
> +
> +
>  /* drm_dumb_buffers.c */
> +int drm_modeset_register_all(struct drm_device *dev);
> +void drm_modeset_unregister_all(struct drm_device *dev);
>  

I was worried for a moment.

Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 10/10] drm: Drop externs from drm_crtc.h

[Chris Wilson]

On Mon, Nov 14, 2016 at 12:58:25PM +0100, Daniel Vetter wrote:
> Just noise.
> 
> Signed-off-by: Daniel Vetter 

Sometimes it is interesting. Practice across the kernel is mixed, but
convergence on not putting extern.

Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 05/10] drm: Clean up kerneldoc for struct drm_driver

[Chris Wilson]

On Mon, Nov 14, 2016 at 12:58:20PM +0100, Daniel Vetter wrote:
> Just cleans up what's there, still plenty missing.
> 
> Signed-off-by: Daniel Vetter 

I read it, seems to match my limited understanding of kerneldoc.
Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 07/10] drm/print: Move kerneldoc next to definition

[Chris Wilson]

On Mon, Nov 14, 2016 at 12:58:22PM +0100, Daniel Vetter wrote:
> kerneldoc expects the comment next to definitions, otherwise it can't
> pick up exported vs. internal stuff.
> 
> This fixes a warning from the doc build done with:
> 
> $ make DOCBOOKS="" htmldocs
> 
> Fixes: d8187177b0b1 ("drm: add helper for printing to log or seq_file")
> Cc: Rob Clark 
> Cc: Sean Paul 
> Signed-off-by: Daniel Vetter 

Oh well, I liked the practice of having interface descriptions in the
headers. If they are in the body, I may as well just read the code.

Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 09/10] drm: Move tile group code into drm_connector.c

[Chris Wilson]

On Mon, Nov 14, 2016 at 12:58:24PM +0100, Daniel Vetter wrote:
> And also put the overview section into the KMS Properties part of the
> docs, instead of randomly-placed within the helpers - this is part of
> the uabi.
> 
> With this patch I think drm_crtc.[hc] is cleaned up and entirely
> documented.
> 
> Signed-off-by: Daniel Vetter 

Code motion looks ok, placement inside the rst I'll take you at your
word.
Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 01/10] drm: Extract drm_dumb_buffers.c

[Chris Wilson]

le info
> - *
> - * This destroys the userspace handle for the given dumb backing storage 
> buffer.
> - * Since buffer objects must be reference counted in the kernel a buffer 
> object
> - * won't be immediately freed if a framebuffer modeset object still uses it.
> - *
> - * Called by the user via ioctl.
> - *
> - * Returns:
> - * Zero on success, negative errno on failure.
> - */
> -int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
> - void *data, struct drm_file *file_priv)
> -{
> - struct drm_mode_destroy_dumb *args = data;
> -
> - if (!dev->driver->dumb_destroy)
> - return -ENOSYS;
> -
> - return dev->driver->dumb_destroy(file_priv, dev, args->handle);
> -}
> -
> -/**
>   * drm_mode_config_init - initialize DRM mode_configuration structure
>   * @dev: DRM device
>   *
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
> b/drivers/gpu/drm/drm_crtc_internal.h
> index b076ef58635d..3d23a473ec35 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -43,14 +43,6 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>  
>  void drm_fb_release(struct drm_file *file_priv);
>  
> -/* dumb buffer support IOCTLs */
> -int drm_mode_create_dumb_ioctl(struct drm_device *dev,
> -void *data, struct drm_file *file_priv);
> -int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
> -  void *data, struct drm_file *file_priv);
> -int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
> - void *data, struct drm_file *file_priv);
> -
>  /* IOCTLs */
>  int drm_mode_getresources(struct drm_device *dev,
> void *data, struct drm_file *file_priv);
> @@ -59,6 +51,16 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  int drm_mode_setcrtc(struct drm_device *dev,
>void *data, struct drm_file *file_priv);
>  
> +/* drm_dumb_buffers.c */
> +
> +/* IOCTLs */
> +int drm_mode_create_dumb_ioctl(struct drm_device *dev,
> +void *data, struct drm_file *file_priv);
> +int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
> +  void *data, struct drm_file *file_priv);
> +int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv);
> +
>  /* drm_color_mgmt.c */
>  
>  /* IOCTLs */
> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c 
> b/drivers/gpu/drm/drm_dumb_buffers.c
> new file mode 100644
> index ..4b4364b61c8d
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> @@ -0,0 +1,135 @@
> +/*
> + * Copyright (c) 2016 Intel Corporation

I've mentioned this elsewhere, but we should also retain the original
copyright statements for the code we are copying.

Otherwise,
Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 02/10] drm/i915: Fixup kerneldoc includes

[Chris Wilson]

On Mon, Nov 14, 2016 at 12:58:17PM +0100, Daniel Vetter wrote:
> Would be great if everony could add

everyone

> $ make DOCBOOKS="" htmldocs
> 
> to their build scripts to catch these. 0day should also report them,
> not sure why it failed to spot this.

"make IGNORE_DOCBOOKS=1 SPHINXOPTS=-W htmldocs" is that outdated?

I don't often run it since it is too slow when checking hundreds of
patches. Any chance of an incremental patch checker?

> Fixes: b42fe9ca0a1e ("drm/i915: Split out i915_vma.c")
> Cc: Tvrtko Ursulin 
> Cc: Chris Wilson 
> Cc: Joonas Lahtinen 
> Signed-off-by: Daniel Vetter 
Reviewed-by: Chris Wilson 

(I'm not even going to ask how it appears three times in the docs and
how that all works :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 03/10] doc/dma-buf: Fix up include directives

[Chris Wilson]

On Mon, Nov 14, 2016 at 12:58:18PM +0100, Daniel Vetter wrote:
> Would be great if everony could add
> 
> $ make DOCBOOKS="" htmldocs
> 
> to their build scripts to catch these. 0day should also report them,
> not sure why it failed to spot this.
> 
> Fixes: f54d1867005c ("dma-buf: Rename struct fence to dma_fence")
> Cc: Chris Wilson 
> Cc: Gustavo Padovan 
> Cc: Sumit Semwal 
> Cc: Christian König 
> Signed-off-by: Daniel Vetter 

Weird, I caught some of the stale Documents/, obviously that didn't
trigger a warning that I needed to do a more careful check.

Reviewed: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 01/10] drm: Extract drm_dumb_buffers.c

[Chris Wilson]

On Tue, Nov 15, 2016 at 12:47:31PM +0100, Daniel Vetter wrote:
> On Tue, Nov 15, 2016 at 10:42:08AM +0000, Chris Wilson wrote:
> > On Mon, Nov 14, 2016 at 12:58:16PM +0100, Daniel Vetter wrote:
> > > diff --git a/drivers/gpu/drm/drm_dumb_buffers.c 
> > > b/drivers/gpu/drm/drm_dumb_buffers.c
> > > new file mode 100644
> > > index ..4b4364b61c8d
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> > > @@ -0,0 +1,135 @@
> > > +/*
> > > + * Copyright (c) 2016 Intel Corporation
> > 
> > I've mentioned this elsewhere, but we should also retain the original
> > copyright statements for the code we are copying.
> 
> Given that we're super-not-dutiful with updating them I figured point at
> git log with rename detection is good enough. But fixed (same for the
> later ones).

I agree that git gives more traceablity to authorship (if not
necessarily to whom that author has transfered the copyright for the work),
but I feel if we are adding a blanket copyright clause we should
recognise the validity of the earlier ones as well.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] dma-buf: Provide wrappers for reservation's lock

[Chris Wilson]

Joonas complained that writing ww_mutex_lock(>lock, ctx) was too
intrusive compared to reservation_object_lock(resv, ctx);

Signed-off-by: Chris Wilson 
Cc: Sumit Semwal 
Cc: Joonas Lahtinen 
---
 include/linux/reservation.h | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/include/linux/reservation.h b/include/linux/reservation.h
index ed238961e1bf..9cfc0d857862 100644
--- a/include/linux/reservation.h
+++ b/include/linux/reservation.h
@@ -129,6 +129,40 @@ reservation_object_fini(struct reservation_object *obj)
 }

 /**
+ * reservation_object_lock - lock the reservation object
+ * @obj: the reservation object
+ * @ctx: the locking context
+ *
+ * Locks the reservation object for exclusive access and modification. Note,
+ * that the lock is only against other writers, readers will run concurrently
+ * with a writer under RCU. The seqlock is used to notify readers if they
+ * overlap with a writer.
+ *
+ * As the reservation object may be locked by multiple parties in an
+ * undefined order, a #ww_acquire_ctx is passed to unwind if a cycle
+ * is detected. See ww_mutex_lock() and ww_acquire_init(). A reservation
+ * object may be locked by itself by passing NULL as @ctx.
+ */
+static inline int
+reservation_object_lock(struct reservation_object *obj,
+   struct ww_acquire_ctx *ctx)
+{
+   return ww_mutex_lock(>lock, ctx);
+}
+
+/**
+ * reservation_object_unlock - unlock the reservation object
+ * @obj: the reservation object
+ *
+ * Unlocks the reservation object following exclusive access.
+ */
+static inline void
+reservation_object_unlock(struct reservation_object *obj)
+{
+   ww_mutex_unlock(>lock);
+}
+
+/**
  * reservation_object_get_excl - get the reservation object's
  * exclusive fence, with update-side lock held
  * @obj: the reservation object
-- 
2.10.2

[PATCH v12 3/3] drm/fence: add out-fences support

[Chris Wilson]

return ret;
> + } else if (property == config->prop_out_fence_ptr) {
> + s64 __user *fence_ptr = u64_to_user_ptr(val);
> +
> + if (!fence_ptr)
> + return 0;
> +
> + if (put_user(-1, fence_ptr))
> + return -EFAULT;
> +
> + set_out_fence_for_crtc(state->state, crtc, fence_ptr);
>   } else if (crtc->funcs->atomic_set_property)
>   return crtc->funcs->atomic_set_property(crtc, state, property, 
> val);
>   else
> @@ -536,6 +563,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>   *val = (state->ctm) ? state->ctm->base.id : 0;
>   else if (property == config->gamma_lut_property)
>   *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> + else if (property == config->prop_out_fence_ptr)
> + *val = 0;
>   else if (crtc->funcs->atomic_get_property)
>   return crtc->funcs->atomic_get_property(crtc, state, property, 
> val);
>   else
> @@ -1664,11 +1693,9 @@ int drm_atomic_debugfs_init(struct drm_minor *minor)
>   */
>  
>  static struct drm_pending_vblank_event *create_vblank_event(
> - struct drm_device *dev, struct drm_file *file_priv,
> - struct dma_fence *fence, uint64_t user_data)
> + struct drm_device *dev, uint64_t user_data)
>  {
>   struct drm_pending_vblank_event *e = NULL;
> - int ret;
>  
>   e = kzalloc(sizeof *e, GFP_KERNEL);
>   if (!e)
> @@ -1678,17 +1705,6 @@ static struct drm_pending_vblank_event 
> *create_vblank_event(
>   e->event.base.length = sizeof(e->event);
>   e->event.user_data = user_data;
>  
> - if (file_priv) {
> - ret = drm_event_reserve_init(dev, file_priv, >base,
> -  >event.base);
> - if (ret) {
> - kfree(e);
> - return NULL;
> - }
> - }
> -
> - e->base.fence = fence;
> -
>   return e;
>  }
>  
> @@ -1793,6 +1809,165 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>  
> +static struct dma_fence *get_crtc_fence(struct drm_crtc *crtc)
> +{

return drm_crtc_fence_create(crtc);

(or possibly, drm_crtc_fence_get(), drm_crtc_timeline_advance() or
somesuch if we need finer control over fence_seqno)

Or if you want to embed it,

struct our_fence *fence;

fence = kzalloc(sizeof(*fence), GFP_KERNEL);
if (!fence)
return NULL;

drm_crtc_fence_init(crtc, >base);

return >base;


Looks tidier than dumping all the fence construction here

> + dma_fence_init(fence, _crtc_fence_ops, >fence_lock,
> +crtc->fence_context, ++crtc->fence_seqno);

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2 1/4] drm: Define drm_mm_for_each_node_in_range()

[Chris Wilson]

Some clients would like to iterate over every node within a certain
range. Make a nice little macro for them to hide the mixing of the
rbtree search and linear walk.

Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
---
 drivers/gpu/drm/drm_mm.c | 11 ++-
 include/drm/drm_mm.h |  8 +---
 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 632473beb40c..f8eebbde376e 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -174,19 +174,12 @@ INTERVAL_TREE_DEFINE(struct drm_mm_node, rb,
 START, LAST, static inline, drm_mm_interval_tree)

 struct drm_mm_node *
-drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last)
+__drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last)
 {
return drm_mm_interval_tree_iter_first(>interval_tree,
   start, last);
 }
-EXPORT_SYMBOL(drm_mm_interval_first);
-
-struct drm_mm_node *
-drm_mm_interval_next(struct drm_mm_node *node, u64 start, u64 last)
-{
-   return drm_mm_interval_tree_iter_next(node, start, last);
-}
-EXPORT_SYMBOL(drm_mm_interval_next);
+EXPORT_SYMBOL(__drm_mm_interval_first);

 static void drm_mm_interval_tree_add_node(struct drm_mm_node *hole_node,
  struct drm_mm_node *node)
diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
index 41ddafe92b2f..fca5f313cf18 100644
--- a/include/drm/drm_mm.h
+++ b/include/drm/drm_mm.h
@@ -308,10 +308,12 @@ void drm_mm_takedown(struct drm_mm *mm);
 bool drm_mm_clean(struct drm_mm *mm);

 struct drm_mm_node *
-drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last);
+__drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last);

-struct drm_mm_node *
-drm_mm_interval_next(struct drm_mm_node *node, u64 start, u64 last);
+#define drm_mm_for_each_node_in_range(node, mm, start, end)\
+   for (node = __drm_mm_interval_first((mm), (start), (end)-1);\
+node && node->start < (end);   \
+node = list_next_entry(node, node_list))   \

 void drm_mm_init_scan(struct drm_mm *mm,
  u64 size,
-- 
2.10.2

[PATCH] drm: Define drm_mm_for_each_node_in_range()

[Chris Wilson]

Some clients would like to iterate over every node within a certain
range. Make a nice little macro for them to hide the mixing of the
rbtree search and linear walk.

v2: Blurb

Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
---
 drivers/gpu/drm/drm_mm.c | 11 ++-
 include/drm/drm_mm.h | 22 +++---
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 632473beb40c..f8eebbde376e 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -174,19 +174,12 @@ INTERVAL_TREE_DEFINE(struct drm_mm_node, rb,
 START, LAST, static inline, drm_mm_interval_tree)

 struct drm_mm_node *
-drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last)
+__drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last)
 {
return drm_mm_interval_tree_iter_first(>interval_tree,
   start, last);
 }
-EXPORT_SYMBOL(drm_mm_interval_first);
-
-struct drm_mm_node *
-drm_mm_interval_next(struct drm_mm_node *node, u64 start, u64 last)
-{
-   return drm_mm_interval_tree_iter_next(node, start, last);
-}
-EXPORT_SYMBOL(drm_mm_interval_next);
+EXPORT_SYMBOL(__drm_mm_interval_first);

 static void drm_mm_interval_tree_add_node(struct drm_mm_node *hole_node,
  struct drm_mm_node *node)
diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
index 41ddafe92b2f..6aab836c5338 100644
--- a/include/drm/drm_mm.h
+++ b/include/drm/drm_mm.h
@@ -308,10 +308,26 @@ void drm_mm_takedown(struct drm_mm *mm);
 bool drm_mm_clean(struct drm_mm *mm);

 struct drm_mm_node *
-drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last);
+__drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last);

-struct drm_mm_node *
-drm_mm_interval_next(struct drm_mm_node *node, u64 start, u64 last);
+/**
+ * drm_mm_for_each_node_in_range - iterator to walk over a range of
+ * allocated nodes
+ * @node: drm_mm_node structure to assign to in each iteration step
+ * @mm: drm_mm allocator to walk
+ * @start: starting offset, the first node will overlap this
+ * @end: ending offset, the last node will start before this (but may overlap)
+ *
+ * This iterator walks over all nodes in the range allocator that start
+ * between @start and @end. It is implemented similar to list_for_each(),
+ * but using the internal interval tree to accelerate the search for the
+ * starting node, and so not safe against removal of elements. It assumes
+ * that @end is within (or is the upper limit of) the drm_mm allocator.
+ */
+#define drm_mm_for_each_node_in_range(node, mm, start, end)\
+   for (node = __drm_mm_interval_first((mm), (start), (end)-1);\
+node && node->start < (end);   \
+node = list_next_entry(node, node_list))   \

 void drm_mm_init_scan(struct drm_mm *mm,
  u64 size,
-- 
2.10.2

[Intel-gfx] [PATCH 3/3] drm/dp/mst: Track available time slots in DP Multi-Stream Transport Packet

[Chris Wilson]

On Thu, Nov 17, 2016 at 06:03:48PM -0800, Dhinakaran Pandiyan wrote:
>  static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>   struct drm_dp_vcpi *vcpi, int pbn)
>  {
> - int num_slots;
> + int req_slots;
>   int ret;
>  
> - num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> + req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
>  
> - if (num_slots > mgr->avail_slots)
> - return -ENOSPC;
> + mutex_lock(>lock);
> + if (req_slots > mgr->avail_slots) {
> + ret = -ENOSPC;
> + goto out;
> + }

You are not atomically reducing the mgr->avail_slots, leading to
possible overallocation of multiple streams?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/vgem: Allow root to inject hanging fences onto dmabufs

[Chris Wilson]

On Wed, Jul 20, 2016 at 08:39:43PM +0100, Chris Wilson wrote:
> When performing driver testing, one factor we want to test is how we
> handle a foreign fence that is never signaled. We can wait on that fence
> indefinitely, in which case the driver appears hung, or we can take some
> remedial action (with risks regarding the state of any shared content).
> Whatever the action choosen by the driver, in order to perform the test
> we want to disable the safety feature of vgem fence (which is then used
> to test implicit dma-buf fencing). This is regarded as a highly
> dangerous feature and so hidden behind an expert config option and only
> available to root when enabled.
> 
> Signed-off-by: Chris Wilson 

Ping. This is invaluable for my testing (some tests are impossible
without it).

The only comment was by Kristian asking why this should be root-only
which nicely contrasts the earlier *demand* that this should be root-only
and removed from the default set of vgem capabilities.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2] Revert "dma-buf/sync-file: Avoid enable fence signaling if poll(.timeout=0)"

[Chris Wilson]

On Fri, Nov 18, 2016 at 05:26:43PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> This reverts commit ecebca79f6976ddaddfd054d699272515869ea28.
> 
> Do not enable fence callback on poll() when using fence_array causes the
> fence_array to not signal.
> 
> For now we will revert the change and enable signaling everytime time
> poll is called with timeout=0 as well.
> 
> Cc: Chris Wilson 
> Signed-off-by: Gustavo Padovan 

Acked-by: Chris Wilson 

I have some patches to use a bit on fence_array->flags to indicate where
we can use this shortcut. I'm hoping someone has a better idea.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH] drm/vgem: Allow root to inject hanging fences onto dmabufs

[Chris Wilson]

On Fri, Nov 18, 2016 at 10:40:02AM +0100, Daniel Vetter wrote:
> On Fri, Nov 18, 2016 at 08:49:37AM +0000, Chris Wilson wrote:
> > On Wed, Jul 20, 2016 at 08:39:43PM +0100, Chris Wilson wrote:
> > > When performing driver testing, one factor we want to test is how we
> > > handle a foreign fence that is never signaled. We can wait on that fence
> > > indefinitely, in which case the driver appears hung, or we can take some
> > > remedial action (with risks regarding the state of any shared content).
> > > Whatever the action choosen by the driver, in order to perform the test
> > > we want to disable the safety feature of vgem fence (which is then used
> > > to test implicit dma-buf fencing). This is regarded as a highly
> > > dangerous feature and so hidden behind an expert config option and only
> > > available to root when enabled.
> > > 
> > > Signed-off-by: Chris Wilson 
> > 
> > Ping. This is invaluable for my testing (some tests are impossible
> > without it).
> 
> Hm, I thought I voted for a module option (so that tests can frob at
> runtime) that auto-taints the kernel. And then maybe always allow it. I
> just don't like when Kconfig settings can result in testcase skips, makes
> it uncertain whether QA did QA or not.

We know when it is disabled, and so when it skips. We also control the
kconfig set by QA. There was very strong objections to having yet
another dangling fence that I thought making it available in default
builds was not desired.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] v4.9-rc3: graphical artefacts in X

[Chris Wilson]

On Fri, Nov 18, 2016 at 12:02:56PM +0100, Pavel Machek wrote:
> Hi!
> 
> With v4.9, if I maximize "nowcast -x" application, I get broken
> display (as if someone split the window into rectangles and shuffled
> them a bit). Switching virtual desktops either fixes it or breaks it,
> depending in how fast I switch. (Yes, strange).

The fix should have landed in v4.9-rc5
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/5] drm: Set DRM connector link status property

[Chris Wilson]

On Fri, Nov 18, 2016 at 06:58:59PM -0800, Manasi Navare wrote:
> In the usual working scenarios, this property is "Good".
> If something fails during modeset, the DRM driver can
> set the link status to "Bad", prune the mode list based on the
> link rate/lane count fallback values and send  hotplug uevent
> so that userspace that is aware of this property can take an
> appropriate action by reprobing connectors and re triggering
> a modeset to improve user experience and avoid black screens.
> In case of userspace that is not aware of this link status
> property, the user experience will be unchanged.
> 
> The reason for adding the property is to handle link training failures,
> but it is not limited to DP or link training. For example, if we
> implement asynchronous setcrtc, we can use this to report any failures
> in that.
> 
> v3:
> * Updated kernel doc even more (Daniel Vetter)
> v2:
> * Update kernel doc, add lockdep_assert_held (Daniel Vetter)
> Cc: dri-devel at lists.freedesktop.org
> Cc: Jani Nikula 
> Cc: Daniel Vetter 
> Cc: Ville Syrjala 
> Signed-off-by: Manasi Navare 
> ---
>  drivers/gpu/drm/drm_connector.c | 37 +
>  include/drm/drm_connector.h |  2 ++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index cfb5cf7..b5ca70f 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1019,6 +1019,43 @@ int drm_mode_connector_update_edid_property(struct 
> drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
>  
> +/**
> + * drm_mode_connector_set_link_status_property - Set link status property of 
> a connector
> + * @connector: drm connector
> + * @link_status: new value of link status property (0: Good, 1: Bad)
> + *
> + * In usual working scenario, this link status property will always be set to
> + * "GOOD". If something fails during or after a mode set, the kernel driver 
> should
> + * set this link status property to "BAD" and prune the mode list based on 
> new
> + * information. The caller then needs to send a hotplug uevent for userspace 
> to
> + *  re-check the valid modes through GET_CONNECTOR_IOCTL and retry modeset.
> + *
> + * Note that a lot of existing userspace do not handle this property.
> + * Drivers can therefore not rely on userspace to fix up everything and
> + * should try to handle issues (like just re-training a link) without
> + * userspace's intervention. This should only be used when the current mode
> + * fails and userspace must select a different display mode.
> + * The DRM driver can chose to not modify property and keep link status
> + * as "GOOD" always to keep the user experience same as it currently is.
> + *
> + * The reason for adding this property is to handle link training failures, 
> but
> + * it is not limited to DP or link training. For example, if we implement
> + * asynchronous setcrtc, this property can be used to report any failures in 
> that.
> + */
> +void drm_mode_connector_set_link_status_property(struct drm_connector 
> *connector,
> +  uint64_t link_status)
> +{
> + struct drm_device *dev = connector->dev;
> +
> + /* Make sure the mutex is grabbed */
> + lockdep_assert_held(>mode_config.mutex);

The comment is just duplicating the code; (i++; /* post-increment i */).
Comments are about why the code exists.

Common practice is to leave a blank line between function preconditions
and the body of actual code.

For example it might be worth explaining why link_status is duplicated
on the connector and in its property (i.e. that is near impossible to
retrieve the value from the property).

> + connector->link_status = link_status;
> + drm_object_property_set_value(>base,
> +   dev->mode_config.link_status_property,
> +   link_status);
> +}
> +EXPORT_SYMBOL(drm_mode_connector_set_link_status_property);

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed

[Chris Wilson]

; > > > means
> > > > > if it said ALLOW_MODESET. So if it can't tolerate the modeset it 
> > > > > should
> > > > > probably say as much?
> > > > 
> > > > Yeah, agreed. Also, if the link is bad then we pretty much have to do a
> > > > modeset to recover it, otherwise you'll be forever stuck with a bad
> > > > screen.
> > > > 
> > > > What we could try is to gate this of whether userspace touches the mode
> > > > property on the corresponding CRTC. I.e. if that's touched (even if it's
> > > > the same mode), and a link is bad in one of the connectors in the state
> > > > then we do a full modeset to recover.
> > > > 
> > > > Another option would be to make the link status writeable. Trying to
> > > > change it from bad->good would force the modeset. That would be 100% 
> > > > clear
> > > > to userspace, not special hacks needed with checking for allow_modeset,
> > > > no magic property that auto-changes its value. And 100% backwards compat
> > > > because existing userspace should never touch properties it doesn't
> > > > understand (except when restoring a mode, and then it should allow a 
> > > > full
> > > > modeset). And if someone does try a good->bad transition, we just 
> > > > silently
> > > > keep it at good.
> > > > 
> > > > Definitely need to document this properly in the property docs, no 
> > > > matter
> > > > what we decide.
> > > 
> > > Hmm. I think I kinda like this idea of userspace clear the state back
> > > to good explicitly, if it happens with the same prop. So it's like
> > > Maarten's retrain_link prop idea, but without having to add the second
> > > prop to the mix.
> > > 
> > > It would also save me from pointing out (for the nth time) that the
> > > link status should really be cleared to good during the commit state
> > > swap and not at some random point during the commit ;)
> > >
> > 
> > Okay, so change 1 is to make the userspace clear the state back to Good for 
> > the property..
> > Then Change 2 is to set connector_changed flag in crtc_state to true if 
> > this property changed
> > from BAD to GOOD. I am not quite how and where to change this to state 
> > connector_changed to true.
> > Without this the full modeset will not happen and the whole design of 
> > retrianing is lost.
> 
> To make this work we need a few more bits:
> 
> - link-status needs to become a full-blown atomic property. This means we
>   need to move link_status into drm_connector_state, mark the property as
>   type ATOMIC and wire up the get/set stuff.
> 
> - once that's done it's also pretty easy to set crtc->connectors_changed
>   from the atomic helpers. You can just compare old and new link_status in
>   drm_connector_state, similar to how we compare old/new CRTC in
>   drm_connector_state->crtc already.
> 
> - Another fallout is that legacy clients will no longer see the
>   link-status property. And they won't be able to set it through the
>   SETCRTC ioctl, which would kinda defaut the point. I think the best
>   solution would be to check for link_status == BAD in
>   drm_atomic_helper_set_config, and reset it to good automatically for
>   legacy clients.

Then how do they know that the kernel demands the modeset? Both a legacy
and atomic property?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed

[Chris Wilson]

On Mon, Nov 21, 2016 at 11:00:52AM -0800, Manasi Navare wrote:
> On Mon, Nov 21, 2016 at 04:48:07PM +0100, Daniel Vetter wrote:
> > On Mon, Nov 21, 2016 at 11:10:45AM +0100, Daniel Vetter wrote:
> > > On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote:
> > > > On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> > > > > - Another fallout is that legacy clients will no longer see the
> > > > >   link-status property. And they won't be able to set it through the
> > > > >   SETCRTC ioctl, which would kinda defaut the point. I think the best
> > > > >   solution would be to check for link_status == BAD in
> > > > >   drm_atomic_helper_set_config, and reset it to good automatically for
> > > > >   legacy clients.
> > > > 
> > > > Then how do they know that the kernel demands the modeset? Both a legacy
> > > > and atomic property?
> > > 
> > > I guess we could avoid the filtering of the property for legacy clients.
> > > Definitely not 2 properties, that's silly. Or we teach userspace to go
> > > look for atomic properties.
> > 
> > Well, now that I flushed the gunk out of my brain with some work-out it's
> > a lot easier: ATOMIC on properties is only to hide them from legacy
> > userspace, it doesn't control how it's implement. Which means we can
> > implement it as described above, and non-atomic userspace can still read
> > it. Setting would also work, but since we want to do that as part of
> > SETCRTC anyway, and since legacy SETCRTC doesn't specifiy whether a
> > modeset will happen or not, automagic in there seems reasonable.
> 
> Thanks Daniel for providing the solution alternatives here.
> So after we make it atomic, we would solve the problem of updating the 
> connector_changed
> in atomic_helper_check_modeset function. So in this, who resets the property 
> to GOOD?
> Would this happen in drm_atomic_helper_set_config in both atomic and non 
> atomic cases?
> 
> And in case of non atomic userspace, will it still be able to read 
> link-status as BAD in userspace
> to decide whether it needs to call setcrtc?
> 
> Chris, will any implementation in your patch for link _status change if this 
> is made atomic?

So long at the property remains visible via the GETCONNECTOR ioctl, no.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Chris Wilson]

On Tue, Nov 22, 2016 at 10:53:51AM +, Brian Starkey wrote:
> Hi Gustavo,
> 
> A little late to the party here, but I was blocked by our internal
> contributions process...
> 
> I didn't see these end up in my checkout yet though, so I guess they
> aren't picked up yet.
> 
> On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote:
> >From: Gustavo Padovan 
> >
> >Add support for the OUT_FENCE_PTR property to enable setting out fences for
> >atomic commits.
> >
> >Signed-off-by: Gustavo Padovan 
> >---
> >lib/igt_kms.c | 20 +++-
> >lib/igt_kms.h |  3 +++
> >2 files changed, 22 insertions(+), 1 deletion(-)
> >
> >diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >index 4748c0a..f25e1eb 100644
> >--- a/lib/igt_kms.c
> >+++ b/lib/igt_kms.c
> >@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
> > "DEGAMMA_LUT",
> > "GAMMA_LUT",
> > "MODE_ID",
> >-"ACTIVE"
> >+"ACTIVE",
> >+"OUT_FENCE_PTR"
> >};
> >
> >const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> >@@ -2103,6 +2104,9 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t 
> >*pipe_obj, drmModeAtomicRe
> > igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, 
> > !!output);
> > }
> >
> >+if (pipe_obj->out_fence_ptr)
> >+igt_atomic_populate_crtc_req(req, pipe_obj, 
> >IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
> >+
> > /*
> >  *  TODO: Add all crtc level properties here
> >  */
> >@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, 
> >size_t length)
> >}
> >
> >/**
> >+ * igt_pipe_set_out_fence_ptr:
> >+ * @pipe: pipe pointer to which background color to be set
> >+ * @fence_ptr: out fence pointer
> 
> I don't think fence_ptr can be int *. It needs to be a pointer to a
> 64-bit type.
> 
> >+ *
> >+ * Sets the out fence pointer that will be passed to the kernel in
> >+ * the atomic ioctl. When the kernel returns the out fence pointer
> >+ * will contain the fd number of the out fence created by KMS.
> >+ */
> >+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
> >+{
> >+pipe->out_fence_ptr = (uint64_t) fence_ptr;
> >+}
> >+
> >+/**
> > * igt_crtc_set_background:
> > * @pipe: pipe pointer to which background color to be set
> > * @background: background color value in BGR 16bpc
> >diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> >index 344f931..02d7bd1 100644
> >--- a/lib/igt_kms.h
> >+++ b/lib/igt_kms.h
> >@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
> >   IGT_CRTC_GAMMA_LUT,
> >   IGT_CRTC_MODE_ID,
> >   IGT_CRTC_ACTIVE,
> >+   IGT_CRTC_OUT_FENCE_PTR,
> >   IGT_NUM_CRTC_PROPS
> >};
> >
> >@@ -298,6 +299,7 @@ struct igt_pipe {
> >
> > uint64_t mode_blob;
> > bool mode_changed;
> >+uint64_t out_fence_ptr;
> 
> IMO this should be:
> 
>   int64_t *out_fence_ptr;

In userspace, fences are *fd*, a plain int. It is only the uabi that we
pass pointers as u64 to the kernel, and indeed that should be limited to
the uabi wrapper.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Intel-gfx] [PATCH 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

[Chris Wilson]

On Tue, Nov 22, 2016 at 11:54:57AM +, Brian Starkey wrote:
> On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote:
> >On Tue, Nov 22, 2016 at 10:53:51AM +, Brian Starkey wrote:
> >>Hi Gustavo,
> >>
> >>A little late to the party here, but I was blocked by our internal
> >>contributions process...
> >>
> >>I didn't see these end up in my checkout yet though, so I guess they
> >>aren't picked up yet.
> >>
> >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote:
> >>>From: Gustavo Padovan 
> >>>
> >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for
> >>>atomic commits.
> >>>
> >>>Signed-off-by: Gustavo Padovan 
> >>>---
> >>>lib/igt_kms.c | 20 +++-
> >>>lib/igt_kms.h |  3 +++
> >>>2 files changed, 22 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >>>index 4748c0a..f25e1eb 100644
> >>>--- a/lib/igt_kms.c
> >>>+++ b/lib/igt_kms.c
> >>>@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
> >>>   "DEGAMMA_LUT",
> >>>   "GAMMA_LUT",
> >>>   "MODE_ID",
> >>>-  "ACTIVE"
> >>>+  "ACTIVE",
> >>>+  "OUT_FENCE_PTR"
> >>>};
> >>>
> >>>const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
> >>>@@ -2103,6 +2104,9 @@ static void 
> >>>igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
> >>>   igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, 
> >>> !!output);
> >>>   }
> >>>
> >>>+  if (pipe_obj->out_fence_ptr)
> >>>+  igt_atomic_populate_crtc_req(req, pipe_obj, 
> >>>IGT_CRTC_OUT_FENCE_PTR, pipe_obj->out_fence_ptr);
> >>>+
> >>>   /*
> >>>*  TODO: Add all crtc level properties here
> >>>*/
> >>>@@ -2683,6 +2687,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, 
> >>>size_t length)
> >>>}
> >>>
> >>>/**
> >>>+ * igt_pipe_set_out_fence_ptr:
> >>>+ * @pipe: pipe pointer to which background color to be set
> >>>+ * @fence_ptr: out fence pointer
> >>
> >>I don't think fence_ptr can be int *. It needs to be a pointer to a
> >>64-bit type.
> >>
> >>>+ *
> >>>+ * Sets the out fence pointer that will be passed to the kernel in
> >>>+ * the atomic ioctl. When the kernel returns the out fence pointer
> >>>+ * will contain the fd number of the out fence created by KMS.
> >>>+ */
> >>>+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int *fence_ptr)
> >>>+{
> >>>+  pipe->out_fence_ptr = (uint64_t) fence_ptr;
> >>>+}
> >>>+
> >>>+/**
> >>> * igt_crtc_set_background:
> >>> * @pipe: pipe pointer to which background color to be set
> >>> * @background: background color value in BGR 16bpc
> >>>diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> >>>index 344f931..02d7bd1 100644
> >>>--- a/lib/igt_kms.h
> >>>+++ b/lib/igt_kms.h
> >>>@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
> >>>   IGT_CRTC_GAMMA_LUT,
> >>>   IGT_CRTC_MODE_ID,
> >>>   IGT_CRTC_ACTIVE,
> >>>+   IGT_CRTC_OUT_FENCE_PTR,
> >>>   IGT_NUM_CRTC_PROPS
> >>>};
> >>>
> >>>@@ -298,6 +299,7 @@ struct igt_pipe {
> >>>
> >>>   uint64_t mode_blob;
> >>>   bool mode_changed;
> >>>+  uint64_t out_fence_ptr;
> >>
> >>IMO this should be:
> >>
> >>int64_t *out_fence_ptr;
> >
> >In userspace, fences are *fd*, a plain int. It is only the uabi that we
> >pass pointers as u64 to the kernel, and indeed that should be limited to
> >the uabi wrapper.
> >-Chris
> 
> Where's the uabi wrapper in this case?
> 
> Wherever it is, afaik someone needs to have 64-bit type for the kernel
> to stash its fd in - on the kernel side out_fence_ptr is
> (s64 __user *), so if there's not a 64-bit variable on the other end
> of it then someone's going to have a bad day.

We do not have pointers in the uabi because they are different sizes on
different platforms. The uabi must be a u64 representation of a user
address to store the result - that is what we pass to the crtc set
property ioctl. That it has been futher managled not to pass around fd
is an interesting twist, but ideally that sillyness should not make
itself into our API.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs

[Chris Wilson]

The fb_helper->connector_count is modified when a new connector is
constructed following a hotplug event (e.g. DP-MST). This causes trouble
for drm_setup_crtcs() and friends that assume that fb_helper is
constant:

[ 1250.872997] BUG: KASAN: slab-out-of-bounds in drm_setup_crtcs+0x320/0xf80 at 
addr 88074cdd2608
[ 1250.873020] Write of size 40 by task kworker/u8:3/480
[ 1250.873039] CPU: 2 PID: 480 Comm: kworker/u8:3 Tainted: G U  
4.9.0-rc6+ #285
[ 1250.873043] Hardware name:  /NUC6i3SYB, BIOS 
SYSKLi35.86A.0024.2015.1027.2142 10/27/2015
[ 1250.873050] Workqueue: events_unbound async_run_entry_fn
[ 1250.873056]  88070f9d78f0 814b72aa 88074e40c5c0 
88074cdd2608
[ 1250.873067]  88070f9d7918 8124ff3c 88070f9d79b0 
88074cdd2600
[ 1250.873079]  88074e40c5c0 88070f9d79a0 812501e4 
0005
[ 1250.873090] Call Trace:
[ 1250.873099]  [] dump_stack+0x67/0x9d
[ 1250.873106]  [] kasan_object_err+0x1c/0x70
[ 1250.873113]  [] kasan_report_error+0x204/0x4f0
[ 1250.873120]  [] ? drm_dev_printk+0x140/0x140
[ 1250.873127]  [] kasan_report+0x53/0x60
[ 1250.873134]  [] ? drm_setup_crtcs+0x320/0xf80
[ 1250.873142]  [] check_memory_region+0x13e/0x1a0
[ 1250.873147]  [] memset+0x23/0x40
[ 1250.873154]  [] drm_setup_crtcs+0x320/0xf80
[ 1250.873161]  [] ? wake_up_q+0x45/0x80
[ 1250.873169]  [] ? mutex_lock_nested+0x5a0/0x5a0
[ 1250.873176]  [] drm_fb_helper_initial_config+0x206/0x7a0
[ 1250.873183]  [] ? drm_fb_helper_set_par+0x90/0x90
[ 1250.873303]  [] ? intel_fbdev_fini+0x140/0x140 [i915]
[ 1250.873387]  [] intel_fbdev_initial_config+0x22/0x40 [i915]
[ 1250.873391]  [] async_run_entry_fn+0x7f/0x270
[ 1250.873394]  [] process_one_work+0x3d0/0x960
[ 1250.873398]  [] ? process_one_work+0x33d/0x960
[ 1250.873401]  [] ? max_active_store+0xf0/0xf0
[ 1250.873406]  [] ? do_raw_spin_lock+0x10d/0x1a0
[ 1250.873413]  [] worker_thread+0x8d/0x840
[ 1250.873419]  [] ? create_worker+0x2e0/0x2e0
[ 1250.873426]  [] kthread+0x194/0x1c0
[ 1250.873432]  [] ? kthread_park+0x60/0x60
[ 1250.873438]  [] ? trace_hardirqs_on+0xd/0x10
[ 1250.873446]  [] ? kthread_park+0x60/0x60
[ 1250.873453]  [] ? kthread_park+0x60/0x60
[ 1250.873457]  [] ret_from_fork+0x27/0x40
[ 1250.873460] Object at 88074cdd2608, in cache kmalloc-32 size: 32

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98826
Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/drm_fb_helper.c | 73 ++---
 1 file changed, 40 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 14547817566d..d13c85682891 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -97,6 +97,10 @@ static LIST_HEAD(kernel_fb_helper_list);
  * mmap page writes.
  */

+#define drm_fb_helper_for_each_connector(fbh, i__) \
+   for (({lockdep_assert_held(&(fbh)->dev->mode_config.mutex); 1;}), \
+i__ = 0; i__ < (fbh)->connector_count; i__++)
+
 /**
  * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
  *emulation helper
@@ -130,7 +134,7 @@ int drm_fb_helper_single_add_all_connectors(struct 
drm_fb_helper *fb_helper)
mutex_unlock(>mode_config.mutex);
return 0;
 fail:
-   for (i = 0; i < fb_helper->connector_count; i++) {
+   drm_fb_helper_for_each_connector(fb_helper, i) {
struct drm_fb_helper_connector *fb_helper_connector =
fb_helper->connector_info[i];

@@ -565,7 +569,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int 
dpms_mode)
continue;

/* Walk the connectors & encoders on this fb turning them 
on/off */
-   for (j = 0; j < fb_helper->connector_count; j++) {
+   drm_fb_helper_for_each_connector(fb_helper, j) {
connector = fb_helper->connector_info[j]->connector;
connector->funcs->dpms(connector, dpms_mode);
drm_object_property_set_value(>base,
@@ -1469,7 +1473,6 @@ static int drm_fb_helper_single_fb_probe(struct 
drm_fb_helper *fb_helper,
int ret = 0;
int crtc_count = 0;
int i;
-   struct fb_info *info;
struct drm_fb_helper_surface_size sizes;
int gamma_size = 0;

@@ -1485,7 +1488,7 @@ static int drm_fb_helper_single_fb_probe(struct 
drm_fb_helper *fb_helper,
sizes.surface_depth = sizes.surface_bpp = preferred_bpp;

/* first up get a count of crtcs now in use and new min/maxes 
width/heights */
-   for (i = 0; i < fb_helper->connector_count; i++) {
+   drm_fb_helper_for_each_connector(fb_helper, i) {
struct drm_fb_helper_connector *fb_helper_conn = 
fb_helper->connector_info[i];
struct drm_cmdline_mode *cmdline_mode;

@@ -

[PATCH 2/3] drm: Pull together probe + setup for drm_fb_helper

[Chris Wilson]

drm_fb_helper_probe_connector_modes() is always called before
drm_setup_crtcs(), so just move the call into drm_setup_crtcs for a
small bit of code compaction.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/drm_fb_helper.c | 37 +++--
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d13c85682891..a19afc7eccde 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2098,20 +2098,19 @@ static int drm_pick_crtcs(struct drm_fb_helper 
*fb_helper,
return best_score;
 }

-static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
+static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
+   u32 width, u32 height)
 {
struct drm_device *dev = fb_helper->dev;
struct drm_fb_helper_crtc **crtcs;
struct drm_display_mode **modes;
struct drm_fb_offset *offsets;
bool *enabled;
-   int width, height;
int i;

DRM_DEBUG_KMS("\n");
-
-   width = dev->mode_config.max_width;
-   height = dev->mode_config.max_height;
+   if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
+   DRM_DEBUG_KMS("No connectors reported connected with modes\n");

/* prevent concurrent modification of connector_count by hotplug */
lockdep_assert_held(_helper->dev->mode_config.mutex);
@@ -2235,23 +2234,15 @@ int drm_fb_helper_initial_config(struct drm_fb_helper 
*fb_helper, int bpp_sel)
 {
struct drm_device *dev = fb_helper->dev;
struct fb_info *info;
-   int count = 0;
int ret;

if (!drm_fbdev_emulation)
return 0;

mutex_lock(>mode_config.mutex);
-   count = drm_fb_helper_probe_connector_modes(fb_helper,
-   dev->mode_config.max_width,
-   
dev->mode_config.max_height);
-   /*
-* we shouldn't end up with no modes here.
-*/
-   if (count == 0)
-   dev_info(fb_helper->dev->dev, "No connectors reported connected 
with modes\n");
-
-   drm_setup_crtcs(fb_helper);
+   drm_setup_crtcs(fb_helper,
+   dev->mode_config.max_width,
+   dev->mode_config.max_height);
ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
mutex_unlock(>mode_config.mutex);
if (ret)
@@ -2299,28 +2290,22 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
 int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 {
struct drm_device *dev = fb_helper->dev;
-   u32 max_width, max_height;

if (!drm_fbdev_emulation)
return 0;

-   mutex_lock(_helper->dev->mode_config.mutex);
+   mutex_lock(>mode_config.mutex);
if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
fb_helper->delayed_hotplug = true;
-   mutex_unlock(_helper->dev->mode_config.mutex);
+   mutex_unlock(>mode_config.mutex);
return 0;
}
DRM_DEBUG_KMS("\n");

-   max_width = fb_helper->fb->width;
-   max_height = fb_helper->fb->height;
+   drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);

-   drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height);
-   mutex_unlock(_helper->dev->mode_config.mutex);
+   mutex_unlock(>mode_config.mutex);

-   drm_modeset_lock_all(dev);
-   drm_setup_crtcs(fb_helper);
-   drm_modeset_unlock_all(dev);
drm_fb_helper_set_par(fb_helper->fbdev);

return 0;
-- 
2.10.2

[PATCH 3/3] drm: Protect fb_helper list manipulation with a mutex

[Chris Wilson]

Though we only walk the kernel_fb_helper_list inside a panic (or single
thread debugging), we still need to protect the list manipulation on
creating/removing a framebuffer device in order to prevent list
corruption.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/drm_fb_helper.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a19afc7eccde..2ac2f462d37b 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -49,6 +49,7 @@ MODULE_PARM_DESC(fbdev_emulation,
 "Enable legacy fbdev emulation [default=true]");

 static LIST_HEAD(kernel_fb_helper_list);
+static DEFINE_MUTEX(kernel_fb_helper_lock);

 /**
  * DOC: fbdev helpers
@@ -855,12 +856,14 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
if (!drm_fbdev_emulation)
return;

+   mutex_lock(_fb_helper_lock);
if (!list_empty(_helper->kernel_fb_list)) {
list_del(_helper->kernel_fb_list);
if (list_empty(_fb_helper_list)) {
unregister_sysrq_key('v', 
_drm_fb_helper_restore_op);
}
}
+   mutex_unlock(_fb_helper_lock);

drm_fb_helper_crtc_free(fb_helper);

@@ -2257,10 +2260,12 @@ int drm_fb_helper_initial_config(struct drm_fb_helper 
*fb_helper, int bpp_sel)
dev_info(dev->dev, "fb%d: %s frame buffer device\n",
 info->node, info->fix.id);

+   mutex_lock(_fb_helper_lock);
if (list_empty(_fb_helper_list))
register_sysrq_key('v', _drm_fb_helper_restore_op);

list_add(_helper->kernel_fb_list, _fb_helper_list);
+   mutex_unlock(_fb_helper_lock);

return 0;
 }
-- 
2.10.2

[PATCH 1/5] drm: Define drm_mm_for_each_node_in_range()

[Chris Wilson]

Some clients would like to iterate over every node within a certain
range. Make a nice little macro for them to hide the mixing of the
rbtree search and linear walk.

v2: Blurb

Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Reviewed-by: Joonas Lahtinen 
---
 drivers/gpu/drm/drm_mm.c | 11 ++-
 include/drm/drm_mm.h | 22 +++---
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 632473beb40c..f8eebbde376e 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -174,19 +174,12 @@ INTERVAL_TREE_DEFINE(struct drm_mm_node, rb,
 START, LAST, static inline, drm_mm_interval_tree)

 struct drm_mm_node *
-drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last)
+__drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last)
 {
return drm_mm_interval_tree_iter_first(>interval_tree,
   start, last);
 }
-EXPORT_SYMBOL(drm_mm_interval_first);
-
-struct drm_mm_node *
-drm_mm_interval_next(struct drm_mm_node *node, u64 start, u64 last)
-{
-   return drm_mm_interval_tree_iter_next(node, start, last);
-}
-EXPORT_SYMBOL(drm_mm_interval_next);
+EXPORT_SYMBOL(__drm_mm_interval_first);

 static void drm_mm_interval_tree_add_node(struct drm_mm_node *hole_node,
  struct drm_mm_node *node)
diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
index 41ddafe92b2f..6add455c651b 100644
--- a/include/drm/drm_mm.h
+++ b/include/drm/drm_mm.h
@@ -308,10 +308,26 @@ void drm_mm_takedown(struct drm_mm *mm);
 bool drm_mm_clean(struct drm_mm *mm);

 struct drm_mm_node *
-drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last);
+__drm_mm_interval_first(struct drm_mm *mm, u64 start, u64 last);

-struct drm_mm_node *
-drm_mm_interval_next(struct drm_mm_node *node, u64 start, u64 last);
+/**
+ * drm_mm_for_each_node_in_range - iterator to walk over a range of
+ * allocated nodes
+ * @node: drm_mm_node structure to assign to in each iteration step
+ * @mm: drm_mm allocator to walk
+ * @start: starting offset, the first node will overlap this
+ * @end: ending offset, the last node will start before this (but may overlap)
+ *
+ * This iterator walks over all nodes in the range allocator that lie
+ * between @start and @end. It is implemented similarly to list_for_each(),
+ * but using the internal interval tree to accelerate the search for the
+ * starting node, and so not safe against removal of elements. It assumes
+ * that @end is within (or is the upper limit of) the drm_mm allocator.
+ */
+#define drm_mm_for_each_node_in_range(node, mm, start, end)\
+   for (node = __drm_mm_interval_first((mm), (start), (end)-1);\
+node && node->start < (end);   \
+node = list_next_entry(node, node_list))   \

 void drm_mm_init_scan(struct drm_mm *mm,
  u64 size,
-- 
2.10.2

[PATCH 2/5] drm: Check against color expansion in drm_mm_reserve_node()

[Chris Wilson]

Use the color_adjust callback when reserving a node to check if
inserting a node into this hole requires any additional space, and so if
that space then conflicts with an existing allocation.

Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
Cc: dri-devel at lists.freedesktop.org
Reviewed-by: Joonas Lahtinen 
---
 drivers/gpu/drm/drm_mm.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index f8eebbde376e..025dcd8cadcb 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -306,6 +306,7 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct 
drm_mm_node *node)
u64 end = node->start + node->size;
struct drm_mm_node *hole;
u64 hole_start, hole_end;
+   u64 adj_start, adj_end;

if (WARN_ON(node->size == 0))
return -EINVAL;
@@ -327,9 +328,13 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct 
drm_mm_node *node)
if (!hole->hole_follows)
return -ENOSPC;

-   hole_start = __drm_mm_hole_node_start(hole);
-   hole_end = __drm_mm_hole_node_end(hole);
-   if (hole_start > node->start || hole_end < end)
+   adj_start = hole_start = __drm_mm_hole_node_start(hole);
+   adj_end = hole_end = __drm_mm_hole_node_end(hole);
+
+   if (mm->color_adjust)
+   mm->color_adjust(hole, node->color, _start, _end);
+
+   if (adj_start > node->start || adj_end < end)
return -ENOSPC;

node->mm = mm;
-- 
2.10.2

[PATCH] drm: Fixup kernel doc for driver->gem_create_object

[Chris Wilson]

Silences

./include/drm/drm_drv.h:295: warning: Incorrect use of kernel-doc format

Signed-off-by: Chris Wilson 
---
 include/drm/drm_drv.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index aad8bbacd1f0..52bf44e2b5cc 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -291,6 +291,8 @@ struct drm_driver {
void (*gem_close_object) (struct drm_gem_object *, struct drm_file *);

/**
+* @gem_create_object: constructor for gem objects
+*
 * Hook for allocating the GEM object struct, for use by core
 * helpers.
 */
-- 
2.10.2

[PATCH 1/3] drm/i915: Use helper for setting plane->state->fb

[Chris Wilson]

We are told not to set plane_state->fb directly, but use
drm_atomic_set_fb_for_plane() instead. Using the helper, means one less
piece of code that needs fixing should the interface change...

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/intel_display.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 77c4ff9efbe3..8630de472f9a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2732,11 +2732,7 @@ update_state_fb(struct drm_plane *plane)
if (plane->fb == plane->state->fb)
return;

-   if (plane->state->fb)
-   drm_framebuffer_unreference(plane->state->fb);
-   plane->state->fb = plane->fb;
-   if (plane->state->fb)
-   drm_framebuffer_reference(plane->state->fb);
+   drm_atomic_set_fb_for_plane(plane->state, plane->fb);
 }

 static void
-- 
2.10.2

[PATCH 3/3] drm: Track framebuffer references at the point of assignment

[Chris Wilson]

We can simplify the code greatly if both legacy and atomic paths updated
the references as they assigned the framebuffer to the planes. Long
before framebuffer reference counting was added, the code had to keep
the old_fb around until after the operation was completed - but now we
can simply leave it to the reference handling to prevent invalid use.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c |  2 +-
 drivers/gpu/drm/armada/armada_crtc.c|  9 +
 drivers/gpu/drm/bochs/bochs_kms.c   |  2 +-
 drivers/gpu/drm/drm_atomic.c| 44 +---
 drivers/gpu/drm/drm_atomic_helper.c | 35 ++--
 drivers/gpu/drm/drm_crtc.c  | 18 +
 drivers/gpu/drm/drm_crtc_helper.c   | 18 +
 drivers/gpu/drm/drm_fb_helper.c | 23 +--
 drivers/gpu/drm/drm_plane.c | 62 ++---
 drivers/gpu/drm/i915/intel_display.c| 17 
 drivers/gpu/drm/mgag200/mgag200_mode.c  |  2 +-
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c   |  2 +-
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c   |  2 +-
 drivers/gpu/drm/nouveau/nouveau_display.c   |  2 +-
 drivers/gpu/drm/nouveau/nv50_display.c  |  7 
 drivers/gpu/drm/qxl/qxl_display.c   |  4 +-
 drivers/gpu/drm/radeon/radeon_display.c |  3 +-
 drivers/gpu/drm/shmobile/shmob_drm_crtc.c   |  2 +-
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c|  4 +-
 drivers/gpu/drm/udl/udl_modeset.c   |  2 +-
 drivers/gpu/drm/vc4/vc4_crtc.c  |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c |  4 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c| 10 ++---
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c|  4 +-
 include/drm/drm_atomic.h|  3 --
 include/drm/drm_plane.h | 27 ++---
 26 files changed, 100 insertions(+), 210 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 741144fcc7bc..2aa4e707be1b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -225,7 +225,7 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
DRM_DEBUG_DRIVER("crtc:%d[%p], pflip_stat:AMDGPU_FLIP_PENDING, work: 
%p,\n",
 amdgpu_crtc->crtc_id, amdgpu_crtc, 
work);
/* update crtc fb */
-   crtc->primary->fb = fb;
+   drm_plane_set_fb(crtc->primary, fb);
spin_unlock_irqrestore(>dev->event_lock, flags);
amdgpu_flip_work_func(>flip_work.work);
return 0;
diff --git a/drivers/gpu/drm/armada/armada_crtc.c 
b/drivers/gpu/drm/armada/armada_crtc.c
index 95cb3966b2ca..1b8cc4dbe5a5 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -1065,13 +1065,7 @@ static int armada_drm_crtc_page_flip(struct drm_crtc 
*crtc,
return ret;
}

-   /*
-* Don't take a reference on the new framebuffer;
-* drm_mode_page_flip_ioctl() has already grabbed a reference and
-* will _not_ drop that reference on successful return from this
-* function.  Simply mark this new framebuffer as the current one.
-*/
-   dcrtc->crtc.primary->fb = fb;
+   drm_plane_set_fb(dcrtc->crtc.primary, fb);

/*
 * Finally, if the display is blanked, we won't receive an
@@ -1080,6 +1074,7 @@ static int armada_drm_crtc_page_flip(struct drm_crtc 
*crtc,
if (dpms_blanked(dcrtc->dpms))
armada_drm_plane_work_run(dcrtc, dcrtc->crtc.primary);

+   drm_framebuffer_unreference(fb);
return 0;
 }

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index 0b4e5d117043..afe3bd2cd79e 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -103,7 +103,7 @@ static int bochs_crtc_page_flip(struct drm_crtc *crtc,
struct drm_framebuffer *old_fb = crtc->primary->fb;
unsigned long irqflags;

-   crtc->primary->fb = fb;
+   drm_plane_set_fb(crtc->primary, fb);
bochs_crtc_mode_set_base(crtc, 0, 0, old_fb);
if (event) {
spin_lock_irqsave(>dev->event_lock, irqflags);
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 19d7bcb88217..63dd5d5becdf 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1253,7 +1253,7 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state 
*plane_state,
DRM_DEBUG_ATOMIC("Set [NOFB] for plane state %p\n",
 plane_state);

-   drm_framebuffer_assign(_state->fb, fb);
+   drm_framebuffer_assign(__mkwrite_drm_framebuffer(_state->fb), fb);
 }
 EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);

@@ -1774,45 +1774,6 @@ static int atomic_set_prop(st

[PATCH 2/3] drm: Introduce drm_framebuffer_assign()

[Chris Wilson]

In a couple of places currently, and with the intent to add more, we
update a pointer to a framebuffer to hold a new fb reference (evicting
the old).

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/drm_atomic.c  |  8 ++--
 include/drm/drm_framebuffer.h | 18 ++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 89737e42fa83..19d7bcb88217 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1246,18 +1246,14 @@ void
 drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
struct drm_framebuffer *fb)
 {
-   if (plane_state->fb)
-   drm_framebuffer_unreference(plane_state->fb);
-   if (fb)
-   drm_framebuffer_reference(fb);
-   plane_state->fb = fb;
-
if (fb)
DRM_DEBUG_ATOMIC("Set [FB:%d] for plane state %p\n",
 fb->base.id, plane_state);
else
DRM_DEBUG_ATOMIC("Set [NOFB] for plane state %p\n",
 plane_state);
+
+   drm_framebuffer_assign(_state->fb, fb);
 }
 EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);

diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index b3141a0e609b..1ddfa2928802 100644
--- a/include/drm/drm_framebuffer.h
+++ b/include/drm/drm_framebuffer.h
@@ -251,6 +251,24 @@ static inline uint32_t 
drm_framebuffer_read_refcount(struct drm_framebuffer *fb)
 }

 /**
+ * drm_framebuffer_assign - store a reference to the fb
+ * @p: location to store framebuffer
+ * @fb: new framebuffer (maybe NULL)
+ *
+ * This functions sets the location to store a reference to the framebuffer,
+ * unreferencing the framebuffer that was previously stored in that location.
+ */
+static inline void drm_framebuffer_assign(struct drm_framebuffer **p,
+ struct drm_framebuffer *fb)
+{
+   if (fb)
+   drm_framebuffer_reference(fb);
+   if (*p)
+   drm_framebuffer_unreference(*p);
+   *p = fb;
+}
+
+/*
  * drm_for_each_fb - iterate over all framebuffers
  * @fb: the loop cursor
  * @dev: the DRM device
-- 
2.10.2