[Intel-gfx] [PATCH 03/12] drm/i915: Unify intel_logical_ring_emit and intel_ring_emit

[Chris Wilson]

On Tue, Nov 24, 2015 at 03:33:12PM +0100, Daniel Vetter wrote:
> On Fri, Nov 20, 2015 at 12:43:43PM +0000, Chris Wilson wrote:
> > @@ -547,7 +547,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> >  
> >  			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
> >  			for_each_ring(signaller, req->i915, i) {
> > -				if (signaller == ring)
> > +				if (signaller == req->ring)
> >  					continue;
> 
> Tsk, unrelated hunks above. And maybe a cocci to s/req->ring/req->engine/.
> At least mention in the commit message that your on a crusade against
> superflous ring/dev/whatever pointers and arguments while in the area.

It was hard to tell where one patch began and another ended.

> > @@ -1275,9 +1269,9 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
> >  	exec_start = params->batch_obj_vm_offset +
> >  		     params->args_batch_start_offset;
> >  
> > -	ret = ring->dispatch_execbuffer(params->request,
> > -					exec_start, exec_len,
> > -					params->dispatch_flags);
> > +	ret = params->ring->dispatch_execbuffer(params->request,
> > +						exec_start, exec_len,
> > +						params->dispatch_flags);
> 
> I guess we should just shovel all the stuff in params into request
> eventually ...

Have you been reading ahead? The only argument for these exact
parameters to be recorded would be for hangcheck. But we do get to
cleanup execbuf dispatch eventually as well.

> > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> > index 444542696a2c..5e9c7c15a84b 100644
> > --- a/drivers/gpu/drm/i915/intel_overlay.c
> > +++ b/drivers/gpu/drm/i915/intel_overlay.c
> > @@ -433,9 +433,9 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
> >  			return ret;
> >  		}
> >  
> > -		intel_ring_emit(ring, MI_WAIT_FOR_EVENT | MI_WAIT_FOR_OVERLAY_FLIP);
> > -		intel_ring_emit(ring, MI_NOOP);
> > -		intel_ring_advance(ring);
> > +		intel_ring_emit(req->ringbuf, MI_WAIT_FOR_EVENT | MI_WAIT_FOR_OVERLAY_FLIP);
> > +		intel_ring_emit(req->ringbuf, MI_NOOP);
> > +		intel_ring_advance(req->ringbuf);
> >  
> >  		ret = intel_overlay_do_wait_request(overlay, req,
> >  						    intel_overlay_release_old_vid_tail);
> 
> Any reason for not doing your usual transform to intel_ringbuffer for the
> local ring here in the overlay code? Or does that happen when
> intel_ring_begin falls?

Later patches looked a bit neater with not adding a temporary
intel_engine_cs *ring; intel_ringbuf *ringbuf. That was my thinking at
least.

> At the end of this crusade we should add some pretty kerneldoc for our
> ringbuffer interface.

request_create
ring_begin{ ring_emit }ring_advance
request_commit (add_request)

But one may ask how do we go from request to ring. Wouldn't it be neat
to have that explicit in the interface:

req = request_create()
..
ring = ring_begin(req);
ring_emit(ring)...
ring_advance(ring)
...
request_commit(req);

Deja vu.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre