Re: drm/i915: Initialize all contexts

Re: drm/i915: Initialize all contexts

[Dan Carpenter]

Hello Ben Widawsky,

This is a semi-automatic email about new static checker warnings.

The patch 6702cf16e0ba: "drm/i915: Initialize all contexts" from Mar 
16, 2015, leads to the following Smatch complaint:

drivers/gpu/drm/i915/i915_gem_context.c:730 do_switch()
	 error: we previously assumed 'to->ppgtt' could be null (see line 713)

drivers/gpu/drm/i915/i915_gem_context.c
   712			 * occur when this occurs. */
   713		} else if (to->ppgtt &&
                           ^^^^^^^^^
Patch introduces a new NULL check.

   714				test_and_clear_bit(ring->id, &to->ppgtt->pd_dirty_rings))
   715			hw_flags |= MI_FORCE_RESTORE;
   716	
   717		/* We should never emit switch_mm more than once */
   718		WARN_ON(needs_pd_load_pre(ring, to) &&
   719				needs_pd_load_post(ring, to, hw_flags));
   720	
   721		ret = mi_set_context(ring, to, hw_flags);
   722		if (ret)
   723			goto unpin_out;
   724	
   725		/* GEN8 does *not* require an explicit reload if the PDPs have been
   726		 * setup, and we do not wish to move them.
   727		 */
   728		if (needs_pd_load_post(ring, to, hw_flags)) {
   729			trace_switch_mm(ring, to);
   730			ret = to->ppgtt->switch_mm(to->ppgtt, ring);
                              ^^^^^^^^^^^
Existing unchecked dereference.

   731			/* The hardware context switch is emitted, but we haven't
   732			 * actually changed the state - so it's probably safe to bail

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

Re: drm/i915: Initialize all contexts

[Daniel Vetter]

On Mon, Mar 23, 2015 at 06:54:52PM +0300, Dan Carpenter wrote:
> Hello Ben Widawsky,
> 
> This is a semi-automatic email about new static checker warnings.

Adding Michel and Mika.
-Daniel

> 
> The patch 6702cf16e0ba: "drm/i915: Initialize all contexts" from Mar 
> 16, 2015, leads to the following Smatch complaint:
> 
> drivers/gpu/drm/i915/i915_gem_context.c:730 do_switch()
> 	 error: we previously assumed 'to->ppgtt' could be null (see line 713)
> 
> drivers/gpu/drm/i915/i915_gem_context.c
>    712			 * occur when this occurs. */
>    713		} else if (to->ppgtt &&
>                            ^^^^^^^^^
> Patch introduces a new NULL check.
> 
>    714				test_and_clear_bit(ring->id, &to->ppgtt->pd_dirty_rings))
>    715			hw_flags |= MI_FORCE_RESTORE;
>    716	
>    717		/* We should never emit switch_mm more than once */
>    718		WARN_ON(needs_pd_load_pre(ring, to) &&
>    719				needs_pd_load_post(ring, to, hw_flags));
>    720	
>    721		ret = mi_set_context(ring, to, hw_flags);
>    722		if (ret)
>    723			goto unpin_out;
>    724	
>    725		/* GEN8 does *not* require an explicit reload if the PDPs have been
>    726		 * setup, and we do not wish to move them.
>    727		 */
>    728		if (needs_pd_load_post(ring, to, hw_flags)) {
>    729			trace_switch_mm(ring, to);
>    730			ret = to->ppgtt->switch_mm(to->ppgtt, ring);
>                               ^^^^^^^^^^^
> Existing unchecked dereference.
> 
>    731			/* The hardware context switch is emitted, but we haven't
>    732			 * actually changed the state - so it's probably safe to bail
> 
> regards,
> dan carpenter
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: drm/i915: Initialize all contexts

[Mika Kuoppala]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Mon, Mar 23, 2015 at 06:54:52PM +0300, Dan Carpenter wrote:
>> Hello Ben Widawsky,
>> 
>> This is a semi-automatic email about new static checker warnings.
>
> Adding Michel and Mika.
> -Daniel
>
>> 
>> The patch 6702cf16e0ba: "drm/i915: Initialize all contexts" from Mar 
>> 16, 2015, leads to the following Smatch complaint:
>> 
>> drivers/gpu/drm/i915/i915_gem_context.c:730 do_switch()
>> 	 error: we previously assumed 'to->ppgtt' could be null (see line 713)
>> 
>> drivers/gpu/drm/i915/i915_gem_context.c
>>    712			 * occur when this occurs. */
>>    713		} else if (to->ppgtt &&
>>                            ^^^^^^^^^
>> Patch introduces a new NULL check.
>> 
>>    714				test_and_clear_bit(ring->id, &to->ppgtt->pd_dirty_rings))
>>    715			hw_flags |= MI_FORCE_RESTORE;
>>    716	
>>    717		/* We should never emit switch_mm more than once */
>>    718		WARN_ON(needs_pd_load_pre(ring, to) &&
>>    719				needs_pd_load_post(ring, to, hw_flags));
>>    720	
>>    721		ret = mi_set_context(ring, to, hw_flags);
>>    722		if (ret)
>>    723			goto unpin_out;
>>    724	
>>    725		/* GEN8 does *not* require an explicit reload if the PDPs have been
>>    726		 * setup, and we do not wish to move them.
>>    727		 */
>>    728		if (needs_pd_load_post(ring, to, hw_flags)) {
>>    729			trace_switch_mm(ring, to);
>>    730			ret = to->ppgtt->switch_mm(to->ppgtt, ring);
>>                               ^^^^^^^^^^^
>> Existing unchecked dereference.

That branch can't be taken if to->ppgtt == NULL. (see
needs_pd_load_post()) So we are safe.
Or is there something I miss out with the context?

Thanks,
--Mika


>> 
>>    731			/* The hardware context switch is emitted, but we haven't
>>    732			 * actually changed the state - so it's probably safe to bail
>> 
>> regards,
>> dan carpenter
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: drm/i915: Initialize all contexts

[Dan Carpenter]

Ugh...  Sorry for that.  Smatch is supposed to do cross function
analysis but I seem to have broken something.  I will investigate and
silence this false positive.

regards,
dan carpenter

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