Re: [PATCH] drm/i915/guc: Set init value for cached work queue head

[Yu Dai <yu.dai@intel.com>]

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