Re: [PATCH 20/53] drm/i915: Update ppgtt_init_ring() & context_enable() to take requests

Tomas Elf
2015-03-13T13:35:39Z

On 13/03/2015 12:46, John Harrison wrote:
> On 05/03/2015 17:57, Tomas Elf wrote:
>> On 19/02/2015 17:17, John.C.Harrison@Intel.com wrote:
>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>
>>> The final step in removing the OLR from i915_gem_init_hw() is to pass
>>> the newly
>>> allocated request structure in to each step rather than passing a ring
>>> structure. This patch updates both i915_ppgtt_init_ring() and
>>> i915_gem_context_enable() to take request pointers.
>>>
>>> For: VIZ-5115
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_drv.h | 2 +-
>>> drivers/gpu/drm/i915/i915_gem.c | 4 ++--
>>> drivers/gpu/drm/i915/i915_gem_context.c | 7 ++++---
>>> drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +++---
>>> drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +-
>>> 5 files changed, 11 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index ea0da6b..618a841 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2990,7 +2990,7 @@ int __must_check i915_gem_context_init(struct
>>> drm_device *dev);
>>> void i915_gem_context_fini(struct drm_device *dev);
>>> void i915_gem_context_reset(struct drm_device *dev);
>>> int i915_gem_context_open(struct drm_device *dev, struct drm_file
>>> *file);
>>> -int i915_gem_context_enable(struct intel_engine_cs *ring);
>>> +int i915_gem_context_enable(struct drm_i915_gem_request *req);
>>> void i915_gem_context_close(struct drm_device *dev, struct drm_file
>>> *file);
>>> int i915_switch_context(struct intel_engine_cs *ring,
>>> struct intel_context *to);
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>>> b/drivers/gpu/drm/i915/i915_gem.c
>>> index efed49a..379bf44 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -4877,7 +4877,7 @@ i915_gem_init_hw(struct drm_device *dev)
>>> i915_gem_l3_remap(ring, i);
>>> }
>>>
>>> - ret = i915_ppgtt_init_ring(ring);
>>> + ret = i915_ppgtt_init_ring(req);
>>> if (ret && ret != -EIO) {
>>> DRM_ERROR("PPGTT enable ring #%d failed %d\n", i, ret);
>>> i915_gem_request_unreference(req);
>>> @@ -4885,7 +4885,7 @@ i915_gem_init_hw(struct drm_device *dev)
>>> return ret;
>>> }
>>>
>>> - ret = i915_gem_context_enable(ring);
>>> + ret = i915_gem_context_enable(req);
>>> if (ret && ret != -EIO) {
>>> DRM_ERROR("Context enable ring #%d failed %d\n", i, ret);
>>> i915_gem_request_unreference(req);
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
>>> b/drivers/gpu/drm/i915/i915_gem_context.c
>>> index dd83d61..04d2a20 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>>> @@ -403,17 +403,18 @@ void i915_gem_context_fini(struct drm_device *dev)
>>> i915_gem_context_unreference(dctx);
>>> }
>>>
>>> -int i915_gem_context_enable(struct intel_engine_cs *ring)
>>> +int i915_gem_context_enable(struct drm_i915_gem_request *req)
>>> {
>>> + struct intel_engine_cs *ring = req->ring;
>>> int ret;
>>>
>>> if (i915.enable_execlists) {
>>> if (ring->init_context == NULL)
>>> return 0;
>>>
>>> - ret = ring->init_context(ring, ring->default_context);
>>> + ret = ring->init_context(req->ring, ring->default_context);
>>> } else
>>> - ret = i915_switch_context(ring, ring->default_context);
>>> + ret = i915_switch_context(req->ring, ring->default_context);
>>
>> You don't have to make any more changes to this function aside from
>> setting up the ring variable at the top. ring = req->ring and if you
>> don't change these lines the will by default use ring like they always
>> did.
>>
>>>
>>> if (ret) {
>>> DRM_ERROR("ring init context: %d\n", ret);
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> index 428d2f6..cd00080 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> @@ -1227,15 +1227,15 @@ int i915_ppgtt_init_hw(struct drm_device *dev)
>>> return 0;
>>> }
>>>
>>> -int i915_ppgtt_init_ring(struct intel_engine_cs *ring)
>>> +int i915_ppgtt_init_ring(struct drm_i915_gem_request *req)
>>> {
>>> - struct drm_i915_private *dev_priv = ring->dev->dev_private;
>>> + struct drm_i915_private *dev_priv = req->ring->dev->dev_private;
>>> struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
>>>
>>> if (!ppgtt)
>>> return 0;
>>>
>>> - return ppgtt->switch_mm(ppgtt, ring);
>>> + return ppgtt->switch_mm(ppgtt, req->ring);
>>> }
>>>
>>
>> If you want to uphold a pattern that you've already established you
>> could just make a single change in the function above by setting up
>> ring = req->ring and then make no more changes to the function body.
>> In this case it's one new line vs. two changes of existing code so
>> it's doesn't make that much of a difference but it is nice to stick to
>> patterns. Also, you wouldn't have to make a 4-level indirection
>> (req->ring->dev->dev_private), only a 3-level one.
> It all depends how often the ring variable would get used. In this case,
> there are only two references. One of which, the switch_mm(), will
> disappear later in the series anyway. So for the sake of a single usage,
> it isn't worth adding in the extra line. The dev_priv is only really
> there because if you don't use an explicit variable, you have to do a
> messy type cast in the middle of the dereference chain.

If switch_mm won't require the ring parameter later in the patch series
(which I should've noticed) then it admittedly becomes kind of
unnecessary to have a local ring variable. Never mind!

Thanks,
Tomas

>
>>
>>> struct i915_hw_ppgtt *
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
>>> b/drivers/gpu/drm/i915/i915_gem_gtt.h
>>> index 5a6cef9..e7e202f 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>>> @@ -300,7 +300,7 @@ void i915_global_gtt_cleanup(struct drm_device
>>> *dev);
>>>
>>> int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt
>>> *ppgtt);
>>> int i915_ppgtt_init_hw(struct drm_device *dev);
>>> -int i915_ppgtt_init_ring(struct intel_engine_cs *ring);
>>> +int i915_ppgtt_init_ring(struct drm_i915_gem_request *req);
>>> void i915_ppgtt_release(struct kref *kref);
>>> struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_device *dev,
>>> struct drm_i915_file_private *fpriv);
>>>
>>
>