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

[Tomas Elf <tomas.elf@intel.com>]

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);
>>>
>>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx