Re: [Intel-gfx] [PATCH 13/26] drm/i915: use the new iterator in i915_gem_busy_ioctl
On 21/09/2021 10:41, Christian König wrote:
Am 20.09.21 um 12:33 schrieb Tvrtko Ursulin:
On 20/09/2021 11:13, Christian König wrote:
Am 20.09.21 um 10:45 schrieb Tvrtko Ursulin:
On 17/09/2021 13:35, Christian König wrote:
This makes the function much simpler since the complex
retry logic is now handled else where.
Signed-off-by: Christian König
---
drivers/gpu/drm/i915/gem/i915_gem_busy.c | 32
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
index 6234e17259c1..b1cb7ba688da 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
@@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void
*data,
{
struct drm_i915_gem_busy *args = data;
struct drm_i915_gem_object *obj;
- struct dma_resv_list *list;
- unsigned int seq;
+ struct dma_resv_iter cursor;
+ struct dma_fence *fence;
int err;
err = -ENOENT;
@@ -109,27 +109,17 @@ i915_gem_busy_ioctl(struct drm_device *dev,
void *data,
* to report the overall busyness. This is what the
wait-ioctl does.
*
*/
-retry:
- seq = raw_read_seqcount(>base.resv->seq);
-
- /* Translate the exclusive fence to the READ *and* WRITE
engine */
- args->busy =
busy_check_writer(dma_resv_excl_fence(obj->base.resv));
-
- /* Translate shared fences to READ set of engines */
- list = dma_resv_shared_list(obj->base.resv);
- if (list) {
- unsigned int shared_count = list->shared_count, i;
-
- for (i = 0; i < shared_count; ++i) {
- struct dma_fence *fence =
- rcu_dereference(list->shared[i]);
-
+ args->busy = false;
+ dma_resv_iter_begin(, obj->base.resv, true);
+ dma_resv_for_each_fence_unlocked(, fence) {
You did not agree with my suggestion to reset args->busy on restart
and so preserve current behaviour?
No, I want to keep the restart behavior internally to the dma_resv
object and as far as I can see it should not make a difference here.
To be clear, on paper difference between old and new implementation is
if the restart happens while processing the shared fences.
Old implementation unconditionally goes to "args->busy =
>>> busy_check_writer(dma_resv_excl_fence(obj->base.resv));" and so
overwrites the set of flags returned to userspace.
New implementation can merge new read flags to the old set of flags
and so return a composition of past and current fences.
Maybe it does not matter hugely in this case, depends if userspace
typically just restarts until flags are clear. But I am not sure.
On the higher level - what do you mean with wanting to keep the
restart behaviour internal? Not providing iterators users means of
detecting it? I think it has to be provided.
Ok I will adjust that for now to get the patch set upstream. But in
general when somebody outside of the dma_resv code base depends on the
restart behavior then that's a bug inside the design of that code.
Thanks, no change in behaviour makes for an easy r-b. :)
The callers should only care about what unsignaled fences are inside the
dma_resv container and it shouldn't matter if those fences are presented
once or multiple times because of a reset..
When this makes a difference we have a bug in the handling and should
probably consider taking the dma_resv.lock instead.
I agree, which is why I was mentioning earlier how it would be good to
completely sort locked from unlocked iterators and avoid situations
where unlocked one is called from a path where object is locked.
Unfortunately for the display code path I cannot easily help with the
audit of call paths. And I think there are at least two patches in your
series which need KMS expertise.
Regards,
Tvrtko
Re: [Intel-gfx] [PATCH 13/26] drm/i915: use the new iterator in i915_gem_busy_ioctl
Am 20.09.21 um 12:33 schrieb Tvrtko Ursulin:
On 20/09/2021 11:13, Christian König wrote:
Am 20.09.21 um 10:45 schrieb Tvrtko Ursulin:
On 17/09/2021 13:35, Christian König wrote:
This makes the function much simpler since the complex
retry logic is now handled else where.
Signed-off-by: Christian König
---
drivers/gpu/drm/i915/gem/i915_gem_busy.c | 32
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
index 6234e17259c1..b1cb7ba688da 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
@@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void
*data,
{
struct drm_i915_gem_busy *args = data;
struct drm_i915_gem_object *obj;
- struct dma_resv_list *list;
- unsigned int seq;
+ struct dma_resv_iter cursor;
+ struct dma_fence *fence;
int err;
err = -ENOENT;
@@ -109,27 +109,17 @@ i915_gem_busy_ioctl(struct drm_device *dev,
void *data,
* to report the overall busyness. This is what the
wait-ioctl does.
*
*/
-retry:
- seq = raw_read_seqcount(>base.resv->seq);
-
- /* Translate the exclusive fence to the READ *and* WRITE
engine */
- args->busy =
busy_check_writer(dma_resv_excl_fence(obj->base.resv));
-
- /* Translate shared fences to READ set of engines */
- list = dma_resv_shared_list(obj->base.resv);
- if (list) {
- unsigned int shared_count = list->shared_count, i;
-
- for (i = 0; i < shared_count; ++i) {
- struct dma_fence *fence =
- rcu_dereference(list->shared[i]);
-
+ args->busy = false;
+ dma_resv_iter_begin(, obj->base.resv, true);
+ dma_resv_for_each_fence_unlocked(, fence) {
You did not agree with my suggestion to reset args->busy on restart
and so preserve current behaviour?
No, I want to keep the restart behavior internally to the dma_resv
object and as far as I can see it should not make a difference here.
To be clear, on paper difference between old and new implementation is
if the restart happens while processing the shared fences.
Old implementation unconditionally goes to "args->busy =
>>> busy_check_writer(dma_resv_excl_fence(obj->base.resv));" and so
overwrites the set of flags returned to userspace.
New implementation can merge new read flags to the old set of flags
and so return a composition of past and current fences.
Maybe it does not matter hugely in this case, depends if userspace
typically just restarts until flags are clear. But I am not sure.
On the higher level - what do you mean with wanting to keep the
restart behaviour internal? Not providing iterators users means of
detecting it? I think it has to be provided.
Ok I will adjust that for now to get the patch set upstream. But in
general when somebody outside of the dma_resv code base depends on the
restart behavior then that's a bug inside the design of that code.
The callers should only care about what unsignaled fences are inside the
dma_resv container and it shouldn't matter if those fences are presented
once or multiple times because of a reset..
When this makes a difference we have a bug in the handling and should
probably consider taking the dma_resv.lock instead.
Regards,
Christian.
Regards,
Tvrtko
Regards,
Christian.
Regards,
Tvrtko
+ if (dma_resv_iter_is_exclusive())
+ /* Translate the exclusive fence to the READ *and*
WRITE engine */
+ args->busy = busy_check_writer(fence);
+ else
+ /* Translate shared fences to READ set of engines */
args->busy |= busy_check_reader(fence);
- }
}
-
- if (args->busy && read_seqcount_retry(>base.resv->seq, seq))
- goto retry;
+ dma_resv_iter_end();
err = 0;
out:
Re: [Intel-gfx] [PATCH 13/26] drm/i915: use the new iterator in i915_gem_busy_ioctl
On 20/09/2021 11:13, Christian König wrote:
Am 20.09.21 um 10:45 schrieb Tvrtko Ursulin:
On 17/09/2021 13:35, Christian König wrote:
This makes the function much simpler since the complex
retry logic is now handled else where.
Signed-off-by: Christian König
---
drivers/gpu/drm/i915/gem/i915_gem_busy.c | 32
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
index 6234e17259c1..b1cb7ba688da 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
@@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void
*data,
{
struct drm_i915_gem_busy *args = data;
struct drm_i915_gem_object *obj;
- struct dma_resv_list *list;
- unsigned int seq;
+ struct dma_resv_iter cursor;
+ struct dma_fence *fence;
int err;
err = -ENOENT;
@@ -109,27 +109,17 @@ i915_gem_busy_ioctl(struct drm_device *dev,
void *data,
* to report the overall busyness. This is what the wait-ioctl
does.
*
*/
-retry:
- seq = raw_read_seqcount(>base.resv->seq);
-
- /* Translate the exclusive fence to the READ *and* WRITE engine */
- args->busy =
busy_check_writer(dma_resv_excl_fence(obj->base.resv));
-
- /* Translate shared fences to READ set of engines */
- list = dma_resv_shared_list(obj->base.resv);
- if (list) {
- unsigned int shared_count = list->shared_count, i;
-
- for (i = 0; i < shared_count; ++i) {
- struct dma_fence *fence =
- rcu_dereference(list->shared[i]);
-
+ args->busy = false;
+ dma_resv_iter_begin(, obj->base.resv, true);
+ dma_resv_for_each_fence_unlocked(, fence) {
You did not agree with my suggestion to reset args->busy on restart
and so preserve current behaviour?
No, I want to keep the restart behavior internally to the dma_resv
object and as far as I can see it should not make a difference here.
To be clear, on paper difference between old and new implementation is
if the restart happens while processing the shared fences.
Old implementation unconditionally goes to "args->busy =
>>> busy_check_writer(dma_resv_excl_fence(obj->base.resv));" and so
overwrites the set of flags returned to userspace.
New implementation can merge new read flags to the old set of flags and
so return a composition of past and current fences.
Maybe it does not matter hugely in this case, depends if userspace
typically just restarts until flags are clear. But I am not sure.
On the higher level - what do you mean with wanting to keep the restart
behaviour internal? Not providing iterators users means of detecting it?
I think it has to be provided.
Regards,
Tvrtko
Regards,
Christian.
Regards,
Tvrtko
+ if (dma_resv_iter_is_exclusive())
+ /* Translate the exclusive fence to the READ *and* WRITE
engine */
+ args->busy = busy_check_writer(fence);
+ else
+ /* Translate shared fences to READ set of engines */
args->busy |= busy_check_reader(fence);
- }
}
-
- if (args->busy && read_seqcount_retry(>base.resv->seq, seq))
- goto retry;
+ dma_resv_iter_end();
err = 0;
out:
Re: [Intel-gfx] [PATCH 13/26] drm/i915: use the new iterator in i915_gem_busy_ioctl
Am 20.09.21 um 10:45 schrieb Tvrtko Ursulin:
On 17/09/2021 13:35, Christian König wrote:
This makes the function much simpler since the complex
retry logic is now handled else where.
Signed-off-by: Christian König
---
drivers/gpu/drm/i915/gem/i915_gem_busy.c | 32
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
index 6234e17259c1..b1cb7ba688da 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
@@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void
*data,
{
struct drm_i915_gem_busy *args = data;
struct drm_i915_gem_object *obj;
- struct dma_resv_list *list;
- unsigned int seq;
+ struct dma_resv_iter cursor;
+ struct dma_fence *fence;
int err;
err = -ENOENT;
@@ -109,27 +109,17 @@ i915_gem_busy_ioctl(struct drm_device *dev,
void *data,
* to report the overall busyness. This is what the wait-ioctl
does.
*
*/
-retry:
- seq = raw_read_seqcount(>base.resv->seq);
-
- /* Translate the exclusive fence to the READ *and* WRITE engine */
- args->busy =
busy_check_writer(dma_resv_excl_fence(obj->base.resv));
-
- /* Translate shared fences to READ set of engines */
- list = dma_resv_shared_list(obj->base.resv);
- if (list) {
- unsigned int shared_count = list->shared_count, i;
-
- for (i = 0; i < shared_count; ++i) {
- struct dma_fence *fence =
- rcu_dereference(list->shared[i]);
-
+ args->busy = false;
+ dma_resv_iter_begin(, obj->base.resv, true);
+ dma_resv_for_each_fence_unlocked(, fence) {
You did not agree with my suggestion to reset args->busy on restart
and so preserve current behaviour?
No, I want to keep the restart behavior internally to the dma_resv
object and as far as I can see it should not make a difference here.
Regards,
Christian.
Regards,
Tvrtko
+ if (dma_resv_iter_is_exclusive())
+ /* Translate the exclusive fence to the READ *and* WRITE
engine */
+ args->busy = busy_check_writer(fence);
+ else
+ /* Translate shared fences to READ set of engines */
args->busy |= busy_check_reader(fence);
- }
}
-
- if (args->busy && read_seqcount_retry(>base.resv->seq, seq))
- goto retry;
+ dma_resv_iter_end();
err = 0;
out:
Re: [Intel-gfx] [PATCH 13/26] drm/i915: use the new iterator in i915_gem_busy_ioctl
On 17/09/2021 13:35, Christian König wrote:
This makes the function much simpler since the complex
retry logic is now handled else where.
Signed-off-by: Christian König
---
drivers/gpu/drm/i915/gem/i915_gem_busy.c | 32
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
index 6234e17259c1..b1cb7ba688da 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
@@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
{
struct drm_i915_gem_busy *args = data;
struct drm_i915_gem_object *obj;
- struct dma_resv_list *list;
- unsigned int seq;
+ struct dma_resv_iter cursor;
+ struct dma_fence *fence;
int err;
err = -ENOENT;
@@ -109,27 +109,17 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
* to report the overall busyness. This is what the wait-ioctl does.
*
*/
-retry:
- seq = raw_read_seqcount(>base.resv->seq);
-
- /* Translate the exclusive fence to the READ *and* WRITE engine */
- args->busy = busy_check_writer(dma_resv_excl_fence(obj->base.resv));
-
- /* Translate shared fences to READ set of engines */
- list = dma_resv_shared_list(obj->base.resv);
- if (list) {
- unsigned int shared_count = list->shared_count, i;
-
- for (i = 0; i < shared_count; ++i) {
- struct dma_fence *fence =
- rcu_dereference(list->shared[i]);
-
+ args->busy = false;
+ dma_resv_iter_begin(, obj->base.resv, true);
+ dma_resv_for_each_fence_unlocked(, fence) {
You did not agree with my suggestion to reset args->busy on restart and
so preserve current behaviour?
Regards,
Tvrtko
+ if (dma_resv_iter_is_exclusive())
+ /* Translate the exclusive fence to the READ *and*
WRITE engine */
+ args->busy = busy_check_writer(fence);
+ else
+ /* Translate shared fences to READ set of engines */
args->busy |= busy_check_reader(fence);
- }
}
-
- if (args->busy && read_seqcount_retry(>base.resv->seq, seq))
- goto retry;
+ dma_resv_iter_end();
err = 0;
out:
Re: [Intel-gfx] [PATCH 13/26] drm/i915: use the new iterator in i915_gem_busy_ioctl
On 13/09/2021 14:16, Christian König wrote:
This makes the function much simpler since the complex
retry logic is now handled else where.
Signed-off-by: Christian König
---
drivers/gpu/drm/i915/gem/i915_gem_busy.c | 30 +++-
1 file changed, 9 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
index 6234e17259c1..c6c6d747b33e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
@@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
{
struct drm_i915_gem_busy *args = data;
struct drm_i915_gem_object *obj;
- struct dma_resv_list *list;
- unsigned int seq;
+ struct dma_resv_cursor cursor;
+ struct dma_fence *fence;
int err;
err = -ENOENT;
@@ -109,28 +109,16 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
* to report the overall busyness. This is what the wait-ioctl does.
*
*/
-retry:
- seq = raw_read_seqcount(>base.resv->seq);
-
- /* Translate the exclusive fence to the READ *and* WRITE engine */
- args->busy = busy_check_writer(dma_resv_excl_fence(obj->base.resv));
-
- /* Translate shared fences to READ set of engines */
- list = dma_resv_shared_list(obj->base.resv);
- if (list) {
- unsigned int shared_count = list->shared_count, i;
-
- for (i = 0; i < shared_count; ++i) {
- struct dma_fence *fence =
- rcu_dereference(list->shared[i]);
-
+ args->busy = false;
+ dma_resv_for_each_fence_unlocked(obj->base.resv, , true, fence) {
To preserve strictly identical behaviour, I think add before the below
if-else:
if (cursor.is_first)
args->busy = 0;
As mentioned elsewhere in the thread I am not convinced it is hugely
important, all that can happen without it is that stale read activity
gets reported, and this ioctl is not really that sensitive to that.
Regards,
Tvrtko
+ if (cursor.is_exclusive)
+ /* Translate the exclusive fence to the READ *and*
WRITE engine */
+ args->busy = busy_check_writer(fence);
+ else
+ /* Translate shared fences to READ set of engines */
args->busy |= busy_check_reader(fence);
- }
}
- if (args->busy && read_seqcount_retry(>base.resv->seq, seq))
- goto retry;
-
err = 0;
out:
rcu_read_unlock();
