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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Dec 01, 2014 at 12:44:12PM +0000, John Harrison wrote:
> 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@Intel.com wrote:
> >>>>From: John Harrison <John.C.Harrison@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@Intel.com>
> >>>>Reviewed-by: Thomas Daniel <Thomas.Daniel@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.

No concern with those patches from my side except that they're after the
caching change. I can cherry-pick them but it looks a bit like that might
backfire if I also don't pick the request state caching.

> 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?

Yeah, probably easier to discuss in a mtg. Jon Ewins should be there too I
think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx