[PATCH 06/91] drm/i915: Consolidate checks for memcpy-from-wc support

[Tvrtko Ursulin]

On 05/01/2017 13:47, Chris Wilson wrote:
> On Thu, Jan 05, 2017 at 01:34:20PM +0000, Tvrtko Ursulin wrote:
>>
>> On 05/01/2017 10:34, Chris Wilson wrote:
>>> In order to silence sparse:
>>>
>>> ../drivers/gpu/drm/i915/i915_gpu_error.c:200:39: warning: Using plain integer as NULL pointer
>>>
>>> add a helper to check whether we have sse4.1 and that the desired
>>> alignment is valid for acceleration.
>>>
>>> Reported-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_cmd_parser.c     | 2 +-
>>> drivers/gpu/drm/i915/i915_drv.h            | 3 +++
>>> drivers/gpu/drm/i915/i915_gpu_error.c      | 2 +-
>>> drivers/gpu/drm/i915/i915_guc_submission.c | 2 +-
>>> 4 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
>>> index 6c310a8a97a7..73aefb0300ba 100644
>>> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
>>> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
>>> @@ -1079,7 +1079,7 @@ static u32 *copy_batch(struct drm_i915_gem_object *dst_obj,
>>>
>>> 	src = ERR_PTR(-ENODEV);
>>> 	if (src_needs_clflush &&
>>> -	    i915_memcpy_from_wc((void *)(uintptr_t)batch_start_offset, NULL, 0)) {
>>> +	    i915_has_memcpy_from_wc((uintptr_t)batch_start_offset)) {
>>
>> I am not convinced having i915_has_memcpy_from_wc take a parameter
>> is useful with a) this being the only call site, and b) it is not
>> checking the alignment on two other parameters anyway.
>
> It is... They are implicitly or'ed in. ;)

What where how? :) src and dst obviously for some reason are correctly 
aligned, but then this code has to know that already and only question 
batch_start_offset. So my point was, i915_has_memcpy_from_wc should take 
no parameters since all the other call sites do not use them, and this 
call site can do its own smarts.

>
>>> 		src = i915_gem_object_pin_map(src_obj, I915_MAP_WC);
>>> 		if (!IS_ERR(src)) {
>>> 			i915_memcpy_from_wc(dst,
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 7b436621d038..946d6ecd52d5 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -4032,6 +4032,9 @@ __i915_request_irq_complete(struct drm_i915_gem_request *req)
>>> void i915_memcpy_init_early(struct drm_i915_private *dev_priv);
>>> bool i915_memcpy_from_wc(void *dst, const void *src, unsigned long len);
>>>
>>> +#define i915_has_memcpy_from_wc(align) \
>>> +	i915_memcpy_from_wc((void *)(align), NULL, 0)
>>
>> I wouldn't call this parameter align in either case.
>
> It is the alignment of the pointers (+stride) that dictate whether or
> not we can use the aligned movnt instructions.
>
> #define i915_can_memcpy_from_wc(dst, src, stride)
> 	i915_memcpy_from_wc((void *)((uintptr_t)dst | (uintptr)src | stride), NULL, 0)
>
> Better?

In addition to "has" with no parameters, yes better. :)

Regards,

Tvrtko