On 28/11/2014 18:06, Daniel Vetter wrote:
On Fri, Nov 28, 2014 at 05:49:26PM +0000, John Harrison wrote:
On 26/11/2014 13:43, Daniel Vetter wrote:
On Mon, Nov 24, 2014 at 06:49:43PM +0000, John.C.Harrison@xxxxxxxxx wrote:
From: John Harrison <John.C.Harrison@xxxxxxxxx>
The ring member of the object structure was always updated with the
last_read_seqno member. Thus with the conversion to last_read_req, obj->ring is
now a direct copy of obj->last_read_req->ring. This makes it somewhat redundant
and potentially misleading (especially as there was no comment to explain its
purpose).
This checkin removes the redundant field. Many uses were simply testing for
non-null to see if the object is active on the GPU. Some of these have been
converted to check 'obj->active' instead. Others (where the last_read_req is
about to be used anyway) have been changed to check obj->last_read_req. The rest
simply pull the ring out from the request structure and proceed as before.
For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
Reviewed-by: Thomas Daniel <Thomas.Daniel@xxxxxxxxx>
Ok merged up to this for now. I'd like to settle things a bit first (and
also figure out what to do with the trace_irq stuff).
Thanks for patches&review,
Daniel
Now that the 3.19 pull request has gone, are you going to continue merging
these patches? Or is there something else you particularly want to wait for?
Well I'm still not convinced that those patches are what we want. For
android (and a few other things) we really want to support struct fence,
and that already provides all the necessary code to cache the completion
state. So I don't see why we have to do a detour just to get caching into
place if for the long-term plan we'll need to rip all that code out again
anyway. And the scheduler without fence/syncpt integration doesn't make a
lot of sense on Android I think.
Now if there's a seriuos performance issue with requests without this then
maybe this detour makes sense. But I don't see any benchmark data attached
to justify the patches from that pov.
-Daniel
Firstly, not all the missing patches are about performance caching. For
example, the kmalloc vs kzalloc patch is simply better code all round.
The tracing patch is also a general purpose driver improvement and makes
debugging/analysing the code easier.
Re the caching. My argument is that the request implementation, as it
stands, is a significant step backwards over the original seqno version.
In the original code, someone had evidently seen fit to make the
completion test a minimal inline function. In the half merged request
version, it is a function call with register reading and list traversal.
As the cached version is already written, tested and reviewed, I do not
see a reason to discard it in order to write a completely new version at
some unspecified point in the future that may or may not be quite a
large piece of work.
Also, the scheduler is already ported on top of the full request patch
set and is running in the GMin Android code base with native sync
included. Having to rewrite the underlying code yet again is going to be
a significant delay in something that needs to be shipped last month.
Delaying that short term work for a long term goal is really undesirable
at this point. The plan has always been to rework for long term
awesomeness (such as dmabuf sync integration) after something basic is
implemented and shipped.
Do you have time for a phone call or meeting to discuss this?
Thanks,
John.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
