Re: [PATCH v3] drm/i915: Add null state batch to active list

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

"Volkin, Bradley D" <bradley.d.volkin@intel.com> writes:

> On Wed, May 21, 2014 at 07:02:56AM -0700, Mika Kuoppala wrote:
>> +	if (ring->id == RCS && !to->is_initialized && from == NULL) {
>> +		ret = i915_gem_render_state_init(ring);
>> +		if (ret)
>> +			DRM_ERROR("init render state: %d\n", ret);
>> +	}
>
> Apologies if this has already been discussed, but why do we have the
> 'from == NULL' check? Shouldn't we initialize all uninitialized RCS
> contexts? Otherwise I thought we'll inherit whatever state 'from' left
> behind.
>
> The hw state should be valid in either case (and so I expect would fix
> the rc6 issue either way), it's just the difference between initializing
> every context to a specific valid state or initializing every context to
> _some_ valid state. The commit message on the first render state patch
> seemed to indicate the former while the implementation looks like the
> latter. Just want to understand which we intended.

It seems that the intentions changed and I forgot to update the
commit message. For now this is just to push some state and hoping
that we can get in/out from rc6 without symptomps. 

The idea that we would restore/initialize to a specific (golden) state for
each new context makes me think that we would get rid of some transient 
bugs we are seeing. As how I understand things are now, app might
inherit some parts from previous ctx and then have lacking
initialization by itself and then see a hang..sometimes.

I guess the main opponent here is the performance implications.
And I lack the experience from user/application side to estimate the impact.

I hope that other devs with more experience on this topic will join
the discussion.

Thanks,
-Mika