Re: [PATCH 1/2] drm/i915/guc: Simplify code by keeping kmap of guc_client object

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

On 02/15/2016 06:39 AM, Dave Gordon wrote:
> On 12/02/16 13:03, Tvrtko Ursulin wrote:
> >
> > On 11/02/16 23:09, yu.dai@intel.com wrote:
> >> From: Alex Dai <yu.dai@intel.com>
> >>
> >> GuC client object is always pinned during its life cycle. We cache
> >> the kmap of its first page, which includes guc_process_desc and
> >> doorbell. By doing so, we can simplify the code where we read from
> >> this page to get where GuC is progressing on work queue; and the
> >> code where driver program doorbell to send work queue item to GuC.
>
> [snip]
>
> >>
> >> -    /* Finally, update the cached copy of the GuC's WQ head */
> >> -    gc->wq_head = desc->head;
> >
> > Did you mean to remove the above?
>
> I wondered that too at first, but the answer is "yes" -- see below.
>
> >>
> >> +    client->client_base = kmap(i915_gem_object_get_dirty_page(obj, 0));
> >
> > Was this another bug, that the page/object wasn't dirtied before?
>
> It wouldn't have made any difference; the object is pinned in the GTT
> forever, so it can't be swapped out or reclaimed.
>
> >> -    uint32_t wq_head;
> >
> > Hm ok I don't get why kmap caching means removing this as well?
>
> 'wq_head' was an optimisation so that we could check whether there was
> known to be space in the workqueue without kmapping and reading the
> process descriptor. Now that the client (which includes the process
> descriptor) is permanently mapped, there's no advantage to caching the
> head; we might just as well read the current value from 'desc->head'
> each time.
>
> > Btw I don't see patch 2/2 ?
> >
>
My bad, there is no 2/2. Thanks Dave for answering the questions. I have 
no more comments. :-)

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