* [drm-intel:for-linux-next 479/497] drivers/gpu/drm/i915/i915_gem_gtt.c:1086:26: sparse: Using plain integer as NULL pointer
@ 2015-08-06 20:17 kbuild test robot
2015-08-06 22:00 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: kbuild test robot @ 2015-08-06 20:17 UTC (permalink / raw)
To: Michel Thierry
Cc: Akash Goel, Daniel Vetter, Ben Widawsky, intel-gfx, kbuild-all
tree: git://anongit.freedesktop.org/drm-intel for-linux-next
head: 176b15ab89a89f5bd5f73f96afcc5c4a6105fa5e
commit: fb6c09f3ae620c5e0db5b4c2058a9c25b52197f9 [479/497] drm/i915/gen8: Make pdp allocation more dynamic
reproduce:
# apt-get install sparse
git checkout fb6c09f3ae620c5e0db5b4c2058a9c25b52197f9
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/i915/i915_gem_gtt.c:1086:26: sparse: Using plain integer as NULL pointer
vim +1086 drivers/gpu/drm/i915/i915_gem_gtt.c
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);
1087
1088 if (ret)
1089 goto free_scratch;
1090
1091 return 0;
1092
1093 free_scratch:
1094 gen8_free_scratch(&ppgtt->base);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [drm-intel:for-linux-next 479/497] drivers/gpu/drm/i915/i915_gem_gtt.c:1086:26: sparse: Using plain integer as NULL pointer
2015-08-06 20:17 [drm-intel:for-linux-next 479/497] drivers/gpu/drm/i915/i915_gem_gtt.c:1086:26: sparse: Using plain integer as NULL pointer kbuild test robot
@ 2015-08-06 22:00 ` Daniel Vetter
2015-08-07 9:21 ` Michel Thierry
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2015-08-06 22:00 UTC (permalink / raw)
To: kbuild test robot; +Cc: Akash Goel, Ben Widawsky, intel-gfx, kbuild-all
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.
How exactly was this tested again?
Oh and the hunk right below with the CONFIG_X86_32 needs to got too -
if we're 48b safe then we should be 32b safe too ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [drm-intel:for-linux-next 479/497] drivers/gpu/drm/i915/i915_gem_gtt.c:1086:26: sparse: Using plain integer as NULL pointer
2015-08-06 22:00 ` Daniel Vetter
@ 2015-08-07 9:21 ` Michel Thierry
2015-08-07 10:21 ` Chris Wilson
0 siblings, 1 reply; 5+ messages in thread
From: Michel Thierry @ 2015-08-07 9:21 UTC (permalink / raw)
To: Daniel Vetter, kbuild test robot
Cc: Akash Goel, Ben Widawsky, intel-gfx, kbuild-all
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.
> How exactly was this tested again?
>
> Oh and the hunk right below with the CONFIG_X86_32 needs to got too -
> if we're 48b safe then we should be 32b safe too ;-)
Yes, after the offset change it'll be completely safe. I left it in this
patch as that was only moving code around.
I'll include a proper "revert-me" patch, saying why it is safe now.
-Michel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [drm-intel:for-linux-next 479/497] drivers/gpu/drm/i915/i915_gem_gtt.c:1086:26: sparse: Using plain integer as NULL pointer
2015-08-07 9:21 ` Michel Thierry
@ 2015-08-07 10:21 ` Chris Wilson
2015-08-07 11:40 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2015-08-07 10:21 UTC (permalink / raw)
To: Michel Thierry
Cc: Ben Widawsky, Daniel Vetter, intel-gfx, Akash Goel, kbuild-all,
kbuild test robot
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.
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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [drm-intel:for-linux-next 479/497] drivers/gpu/drm/i915/i915_gem_gtt.c:1086:26: sparse: Using plain integer as NULL pointer
2015-08-07 10:21 ` Chris Wilson
@ 2015-08-07 11:40 ` Daniel Vetter
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2015-08-07 11:40 UTC (permalink / raw)
To: Chris Wilson, Michel Thierry, Daniel Vetter, kbuild test robot,
Akash Goel, Ben Widawsky, intel-gfx, kbuild-all
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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-08-07 11:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-06 20:17 [drm-intel:for-linux-next 479/497] drivers/gpu/drm/i915/i915_gem_gtt.c:1086:26: sparse: Using plain integer as NULL pointer kbuild test robot
2015-08-06 22:00 ` Daniel Vetter
2015-08-07 9:21 ` Michel Thierry
2015-08-07 10:21 ` Chris Wilson
2015-08-07 11:40 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox