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

Message ID 20150807102103.GD6924@nuc-i3427.alporthouse.com
State New
Headers show

Commit Message

Chris Wilson Aug. 7, 2015, 10:21 a.m. UTC
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@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.

Comments

Daniel Vetter Aug. 7, 2015, 11:40 a.m. UTC | #1
On Fri, Aug 7, 2015 at 12:21 PM, Chris Wilson <chris@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@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

Patch
diff mbox

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

-Chris