Re: [PATCH] drm/i915: Split alloc from init for lrc

Nick Hoath
2015-08-19T14:41:01Z

On 19/08/2015 13:37, Chris Wilson wrote:
> On Wed, Aug 19, 2015 at 01:24:28PM +0100, Nick Hoath wrote:
>> Extend init/init_hw split to context init.
>> - Move context initialisation in to i915_gem_init_hw
>> - Move one off initialisation for render ring to
>> i915_gem_validate_context
>> - Move default context initialisation to logical_ring_init
>>
>> Rename intel_lr_context_deferred_create to
>> intel_lr_context_deferred_alloc, to reflect reduced functionality &
>> alloc/init split.
>>
>> This patch is intended to split out the allocation of resources & initialisation
>> to allow easier reuse of code for resume/gpu reset.
>>
>> v2: Removed function ptr wrapping of do_switch_context (Daniel Vetter)
>> Left ->init_context int intel_lr_context_deferred_alloc (Daniel Vetter)
>> Remove unnecessary init flag & ring type test. (Daniel Vetter)
>> Improve commit message (Daniel Vetter)
>> v3: On init/reinit, set the hw next sequence number to the sw next sequence
>> number. This is set to 1 at driver load time. This prevents the seqno
>> being reset on reinit (Chris Wilson)
>>
>> Issue: VIZ-4798
>> Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 1 -
>> drivers/gpu/drm/i915/i915_gem.c | 18 ++--
>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +-
>> drivers/gpu/drm/i915/intel_lrc.c | 147 ++++++++++++++---------------
>> drivers/gpu/drm/i915/intel_lrc.h | 4 +-
>> 5 files changed, 86 insertions(+), 87 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index f7fd519..844ccf0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -880,7 +880,6 @@ struct intel_context {
>> } legacy_hw_ctx;
>>
>> /* Execlists */
>> - bool rcs_initialized;
>> struct {
>> struct drm_i915_gem_object *state;
>> struct intel_ringbuffer *ringbuf;
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 73293b4..eb7c1f2 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4603,14 +4603,8 @@ int i915_gem_init_rings(struct drm_device *dev)
>> goto cleanup_vebox_ring;
>> }
>>
>> - ret = i915_gem_set_seqno(dev, ((u32)~0 - 0x1000));
>> - if (ret)
>> - goto cleanup_bsd2_ring;
>> -
>> return 0;
>>
>> -cleanup_bsd2_ring:
>> - intel_cleanup_ring_buffer(&dev_priv->ring[VCS2]);
>> cleanup_vebox_ring:
>> intel_cleanup_ring_buffer(&dev_priv->ring[VECS]);
>> cleanup_blt_ring:
>> @@ -4629,6 +4623,7 @@ i915_gem_init_hw(struct drm_device *dev)
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> struct intel_engine_cs *ring;
>> int ret, i, j;
>> + struct drm_i915_gem_request *req;
>>
>> if (INTEL_INFO(dev)->gen < 6 && !intel_enable_gtt())
>> return -EIO;
>> @@ -4680,9 +4675,12 @@ i915_gem_init_hw(struct drm_device *dev)
>> goto out;
>> }
>>
>> + ret = i915_gem_set_seqno(dev, dev_priv->next_seqno);
>> + if (ret)
>> + goto out;
>
> The only reason to do this would be to ensure that the contents of the
> registers are valid (assuming we take over from ourselves). The right
> value to use then is last_seqno.
i915_gem_set_seqno uses the following code:
ret = i915_gem_init_seqno(dev, seqno - 1);
......
dev_priv->next_seqno = seqno;
dev_priv->last_seqno = seqno - 1;

So using last_seqno would rewind the seqno by one...
>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index 923a3c4..95f1a0d 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -994,6 +994,7 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
>> {
>> struct intel_context *ctx = NULL;
>> struct i915_ctx_hang_stats *hs;
>> + int ret;
>>
>> if (ring->id != RCS && ctx_id != DEFAULT_CONTEXT_HANDLE)
>> return ERR_PTR(-EINVAL);
>> @@ -1009,7 +1010,7 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
>> }
>>
>> if (i915.enable_execlists && !ctx->engine[ring->id].state) {
>> - int ret = intel_lr_context_deferred_create(ctx, ring);
>> + ret = intel_lr_context_deferred_alloc(ctx, ring);
>> if (ret) {
>> DRM_DEBUG("Could not create LRC %u: %d\n", ctx_id, ret);
>> return ERR_PTR(ret);
>
> Still modifying this for no reason, and you still haven't realised this
> call is redundant (hint there is already a hook in alloc_request).
>
> From last year:
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?id=37fbd370152211688bc5bce3d28d13233cfe7d8b
>
> More recent (i.e a couple of months ago):
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=ba4950a8f489d54ec4898f94dad44f9ec13301d2
> -Chris