[Intel-gfx] [PATCH v3 21/28] drm/i915: Remove the now redundant 'obj->ring'

[John Harrison]

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 at Intel.com wrote:
>>>> From: John Harrison <John.C.Harrison at Intel.com>
>>>>
>>>> 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 at Intel.com>
>>>> Reviewed-by: Thomas Daniel <Thomas.Daniel at intel.com>
>>> 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.