[Intel-gfx] [drm-intel:for-linux-next 479/497] drivers/gpu/drm/i915/i915_gem_gtt.c:1086:26: sparse: Using plain integer as NULL pointer

[Daniel Vetter]

On Fri, Aug 7, 2015 at 12:21 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Fri, Aug 07, 2015 at 10:21:34AM +0100, Michel Thierry wrote:
>> On 8/6/2015 11:00 PM, Daniel Vetter wrote:
>> >On Thu, Aug 6, 2015 at 10:17 PM, kbuild test robot
>> ><fengguang.wu at intel.com> wrote:
>> >>   1070          if (IS_ENABLED(CONFIG_X86_32))
>> >>   1071                  /* While we have a proliferation of size_t variables
>> >>   1072                   * we cannot represent the full ppgtt size on 32bit,
>> >>   1073                   * so limit it to the same size as the GGTT (currently
>> >>   1074                   * 2GiB).
>> >>   1075                   */
>> >>   1076                  ppgtt->base.total = to_i915(ppgtt->base.dev)->gtt.base.total;
>> >>   1077          ppgtt->base.cleanup = gen8_ppgtt_cleanup;
>> >>   1078          ppgtt->base.allocate_va_range = gen8_alloc_va_range;
>> >>   1079          ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
>> >>   1080          ppgtt->base.clear_range = gen8_ppgtt_clear_range;
>> >>   1081          ppgtt->base.unbind_vma = ppgtt_unbind_vma;
>> >>   1082          ppgtt->base.bind_vma = ppgtt_bind_vma;
>> >>   1083
>> >>   1084          ppgtt->switch_mm = gen8_mm_switch;
>> >>   1085
>> >>>1086          ret = __pdp_init(false, &ppgtt->pdp);
>> >
>> >So the first argument of pdp_init ist struct drm_device *dev and yes
>> >the first thing it does is deref it.
>> >
>>
>> *dev is used only for I915_PDPES_PER_PDP/USES_FULL_48BIT_PPGTT,
>> which in this path is always false. I didn't expect kbuild to
>> complain. I'll change it with the other modifications I'm about to
>> send.
>
> Wrong. dev is never deferenced even though it is passed around.

Yeah I spotted that too right after sending out my mail.

> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 41c5e7c9c8ab..37283d5a8a4a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2549,12 +2549,12 @@ struct drm_i915_cmd_table {
>
>  #define HAS_HW_CONTEXTS(dev)   (INTEL_INFO(dev)->gen >= 6)
>  #define HAS_LOGICAL_RING_CONTEXTS(dev) (INTEL_INFO(dev)->gen >= 8)
> -#define USES_PPGTT(dev)                (i915.enable_ppgtt)
> -#define USES_FULL_PPGTT(dev)   (i915.enable_ppgtt >= 2)
> +#define USES_PPGTT()           (i915.enable_ppgtt)
> +#define USES_FULL_PPGTT()      (i915.enable_ppgtt >= 2)
>  #ifdef CONFIG_X86_64
> -# define USES_FULL_48BIT_PPGTT(dev)    (i915.enable_ppgtt == 3)
> +# define USES_FULL_48BIT_PPGTT()       (i915.enable_ppgtt == 3)
>  #else
> -# define USES_FULL_48BIT_PPGTT(dev)    false
> +# define USES_FULL_48BIT_PPGTT()       false
>  #endif

I'd vote to abolish all these macros and just add an enum for ppgtt modes:

enum i915_ppgtt_mode {
PPGTT_DISABLED = 0,
ALIASING_PPGTT = 1,
FULL_PPGTT = 2,
FULL_48B_PPGTT = 3 };

Then just open-code the checks. Of course separate patch from anything
else really.
-Daniel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch