drm/i915/guc: Set init value for cached work queue head

Submitted by yu.dai@intel.com on Feb. 10, 2016, 12:05 a.m.

Details

Message ID 1455062707-30895-1-git-send-email-yu.dai@intel.com
State New
Headers show
Series "drm/i915/guc: Set init value for cached work queue head" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

yu.dai@intel.com Feb. 10, 2016, 12:05 a.m.
From: Alex Dai <yu.dai@intel.com>

The cached work queue header pointer is set to last byte of work
queue buffer. It will make sure the whole work queue buffer is
available after coming back from reset or init.

Do not hold kmap_atomic mapping before going to sleep when work
queue is full.

Signed-off-by: Alex Dai <yu.dai@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index d7543ef..41f4a96 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -486,11 +486,11 @@  int i915_guc_wq_check_space(struct i915_guc_client *gc)
 	if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size)
 		return 0;
 
-	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
-	desc = base + gc->proc_desc_offset;
-
 	while (timeout_counter-- > 0) {
+		base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
+		desc = base + gc->proc_desc_offset;
 		gc->wq_head = desc->head;
+		kunmap_atomic(base);
 
 		if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size) {
 			ret = 0;
@@ -501,8 +501,6 @@  int i915_guc_wq_check_space(struct i915_guc_client *gc)
 			usleep_range(1000, 2000);
 	};
 
-	kunmap_atomic(base);
-
 	return ret;
 }
 
@@ -730,6 +728,8 @@  static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 	client->client_obj = obj;
 	client->wq_offset = GUC_DB_SIZE;
 	client->wq_size = GUC_WQ_SIZE;
+	client->wq_head = GUC_WQ_SIZE - 1;
+	client->wq_tail = 0;
 
 	client->doorbell_offset = select_doorbell_cacheline(guc);
 

Comments

Hi,

On 10/02/16 00:05, yu.dai@intel.com wrote:
> From: Alex Dai <yu.dai@intel.com>
>
> The cached work queue header pointer is set to last byte of work
> queue buffer. It will make sure the whole work queue buffer is
> available after coming back from reset or init.
>
> Do not hold kmap_atomic mapping before going to sleep when work
> queue is full.

Could you please split this into two patches? They are two completely 
separate issues and it is customary to do so.

For the kmap_atomic issue you can also reference 
https://bugs.freedesktop.org/show_bug.cgi?id=93847 in the commit message.

> Signed-off-by: Alex Dai <yu.dai@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index d7543ef..41f4a96 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -486,11 +486,11 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
>   	if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size)
>   		return 0;
>
> -	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> -	desc = base + gc->proc_desc_offset;
> -
>   	while (timeout_counter-- > 0) {
> +		base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> +		desc = base + gc->proc_desc_offset;
>   		gc->wq_head = desc->head;
> +		kunmap_atomic(base);
>
>   		if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size) {
>   			ret = 0;
> @@ -501,8 +501,6 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
>   			usleep_range(1000, 2000);
>   	};
>
> -	kunmap_atomic(base);
> -
>   	return ret;
>   }

This part is OK to extinguish this fire. But in general you could also 
consider caching the kmap in the client since it looks to me that object 
is persistently pinned for its lifetime. So kmap_atomic just complicates 
things.

> @@ -730,6 +728,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>   	client->client_obj = obj;
>   	client->wq_offset = GUC_DB_SIZE;
>   	client->wq_size = GUC_WQ_SIZE;
> +	client->wq_head = GUC_WQ_SIZE - 1;
> +	client->wq_tail = 0;
>
>   	client->doorbell_offset = select_doorbell_cacheline(guc);
>
>

This one I can't really figure out without I suppose knowing more about 
the code design. How come it was OK when it was zero (apart after reset)?

The value is otherwise only updated from the GuC shared page and a 
driver does not appear to modify it. Perhaps just a better commit 
message to explain things?

Perhaps

Regards,

Tvrtko
On 02/10/2016 09:30 AM, Tvrtko Ursulin wrote:
> Hi,
>
> On 10/02/16 00:05, yu.dai@intel.com wrote:
> > From: Alex Dai <yu.dai@intel.com>
> >
> > The cached work queue header pointer is set to last byte of work
> > queue buffer. It will make sure the whole work queue buffer is
> > available after coming back from reset or init.
> >
> > Do not hold kmap_atomic mapping before going to sleep when work
> > queue is full.
>
> Could you please split this into two patches? They are two completely
> separate issues and it is customary to do so.
>
> For the kmap_atomic issue you can also reference
> https://bugs.freedesktop.org/show_bug.cgi?id=93847 in the commit message.

Yes, will do.
> > Signed-off-by: Alex Dai <yu.dai@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_guc_submission.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index d7543ef..41f4a96 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -486,11 +486,11 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
> >   	if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size)
> >   		return 0;
> >
> > -	base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> > -	desc = base + gc->proc_desc_offset;
> > -
> >   	while (timeout_counter-- > 0) {
> > +		base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> > +		desc = base + gc->proc_desc_offset;
> >   		gc->wq_head = desc->head;
> > +		kunmap_atomic(base);
> >
> >   		if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size) {
> >   			ret = 0;
> > @@ -501,8 +501,6 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
> >   			usleep_range(1000, 2000);
> >   	};
> >
> > -	kunmap_atomic(base);
> > -
> >   	return ret;
> >   }
>
> This part is OK to extinguish this fire. But in general you could also
> consider caching the kmap in the client since it looks to me that object
> is persistently pinned for its lifetime. So kmap_atomic just complicates
> things.

Yes this object must be pinned for its lifetime as it is used by GuC 
internally too. I will think about a way to cache it.

> > @@ -730,6 +728,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
> >   	client->client_obj = obj;
> >   	client->wq_offset = GUC_DB_SIZE;
> >   	client->wq_size = GUC_WQ_SIZE;
> > +	client->wq_head = GUC_WQ_SIZE - 1;
> > +	client->wq_tail = 0;
> >
> >   	client->doorbell_offset = select_doorbell_cacheline(guc);
> >
> >
>
> This one I can't really figure out without I suppose knowing more about
> the code design. How come it was OK when it was zero (apart after reset)?
>
> The value is otherwise only updated from the GuC shared page and a
> driver does not appear to modify it. Perhaps just a better commit
> message to explain things?

The way this kernel CIRC_xx works is it leaves one byte free and treat 
head == tail case as empty. So, there won't be a problem if this head 
happens to be 0. If it comes with some random number between [1, 
sizeof(WQ item)], there will be a SW dead looping in driver.

And, I will split this patch into two ones.

Thanks,
Alex
On 10/02/16 20:31, Yu Dai wrote:
>
>
> On 02/10/2016 09:30 AM, Tvrtko Ursulin wrote:
>> Hi,
>>
>> On 10/02/16 00:05, yu.dai@intel.com wrote:
>> > From: Alex Dai <yu.dai@intel.com>
>> >
>> > The cached work queue header pointer is set to last byte of work
>> > queue buffer. It will make sure the whole work queue buffer is
>> > available after coming back from reset or init.

[snip]

>> > @@ -730,6 +728,8 @@ static struct i915_guc_client
>> *guc_client_alloc(struct drm_device *dev,
>> >       client->client_obj = obj;
>> >       client->wq_offset = GUC_DB_SIZE;
>> >       client->wq_size = GUC_WQ_SIZE;
>> > +    client->wq_head = GUC_WQ_SIZE - 1;
>> > +    client->wq_tail = 0;
>> >
>> >       client->doorbell_offset = select_doorbell_cacheline(guc);
>>
>> This one I can't really figure out without I suppose knowing more about
>> the code design. How come it was OK when it was zero (apart after reset)?
>>
>> The value is otherwise only updated from the GuC shared page and a
>> driver does not appear to modify it. Perhaps just a better commit
>> message to explain things?
>
> The way this kernel CIRC_xx works is it leaves one byte free and treat
> head == tail case as empty. So, there won't be a problem if this head
> happens to be 0. If it comes with some random number between [1,
> sizeof(WQ item)], there will be a SW dead looping in driver.

Actually, I think it works like this:

if both wq_tail and wq_head (which is a CACHE of the GuC's WQ tail) are 
initialised to 0, the driver thinks the WQ is empty, which is correct at 
startup but not necessarily after a GPU reset.

By initialising them as above, the driver will at first think the WQ is 
FULL, and will then refresh the actual value from the GuC's process 
descriptor. This should ensure that it doesn't assume more WQ space than 
the GuC thinks there is.

It's very much the same sort of issue as (re)initialising the CSB 
pointers after a reset, because the CS h/w (or in this case the GuC 
firmware) has internally set them to a new value and the driver needs to 
get in sync.

Anyway all of this is superseded by Alex's later patch that leaves the 
client permanently kmapped, 'cos that gets rid of wq_head entirely :)

.Dave.