[Intel-gfx] [PATCH 09/13] drm/i915: add reset_state for hw_contexts

[Mika Kuoppala]

Ian Romanick <idr at freedesktop.org> writes:

> On 02/26/2013 03:05 AM, Mika Kuoppala wrote:
>> For arb-robustness, every context needs to have it's own
>> reset state tracking. Default context will be handled in a identical
>> way as the no-context case in further down in the patch set.
>> For no-context case, the reset state will be stored in
>> the file_priv part.
>>
>> v2: handle default context inside get_reset_state
>
> This isn't the interface we want.  I already sent you the patches for 
> Mesa, and you seem to have completely ignored them.  Moreover, this 
> interface provides no mechanism to query for its existence (other than 
> relying on the kernel version), and no method to deprecate it.
>
> Based on e-mail discussions, I think danvet agrees with me here.
>
> Putting guilty / innocent counting in kernel puts policy decisions in 
> the kernel that belong with the user space API that implements them. 
> Putting these choices in the kernel significantly decreases how "future 
> proof" the interface is.  Since any kernel/user interface has to be kept 
> forever, this is just asking for maintenance trouble down the road.
>
> Also, it's difficult (for me) to tell from the rest of the series 
> whether or not a context that was not affected by a reset (had no 
> batches in flight at all) can still observe that a reset occurred.
>
>  From the GL point of view, once a context has observed a reset, it's 
> garbage.  Knowing that it saw 1 reset or 43,000,000 resets is the same. 
>   Since it's a count, GL will have to query the values before any 
> rendering happens or it won't know whether the value 6 means there was a 
> reset or not.
>
> The right interface is a set of flags indicating the state the context 
> was in when it observed a reset.  As this patch series stands, Mesa will 
> not use this interface.

There is a little bit of misunderstanding in here I think.
I haven't ignored your input. I abandoned the statistics based
ioctl interface based on your input.

This commit in question adds only internal interface to query
the context reset state.

My intention was to redo the ioctl interface on top of this internal
bookkeepping. The ioctl interface will be made according to this
commit:

http://cgit.freedesktop.org/~idr/mesa/commit/?h=robustness2&id=ef97f4092c703afd49b3516b15dfbfcd27d4bc97

Thanks,
-Mika

>> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h         |    4 ++++
>>   drivers/gpu/drm/i915/i915_gem_context.c |   34 +++++++++++++++++++++++++++++++
>>   2 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 1c67fb2..2cc5817 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1684,6 +1684,10 @@ int i915_switch_context(struct intel_ring_buffer *ring,
>>   			struct drm_file *file, int to_id,
>>   			struct i915_hw_context **ctx);
>>   void i915_gem_context_free(struct kref *ctx_ref);
>> +int i915_gem_context_get_reset_state(struct intel_ring_buffer *ring,
>> +				     struct drm_file *file,
>> +				     u32 id,
>> +				     struct ctx_reset_state **rs);
>>   int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>>   				  struct drm_file *file);
>>   int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>> index cba54fb..1b14a06 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -304,6 +304,40 @@ static int context_idr_cleanup(int id, void *p, void *data)
>>   	return 0;
>>   }
>>
>> +int i915_gem_context_get_reset_state(struct intel_ring_buffer *ring,
>> +				     struct drm_file *file,
>> +				     u32 id,
>> +				     struct ctx_reset_state **rs)
>> +{
>> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>> +	struct drm_i915_file_private *file_priv = file->driver_priv;
>> +	struct i915_hw_context *to;
>> +
>> +	if (dev_priv->hw_contexts_disabled)
>> +		return -ENOENT;
>> +
>> +	if (ring->id != RCS)
>> +		return -EINVAL;
>> +
>> +	if (rs == NULL)
>> +		return -EINVAL;
>> +
>> +	if (file == NULL)
>> +		return -EINVAL;
>> +
>> +	if (id == DEFAULT_CONTEXT_ID) {
>> +		*rs = &file_priv->reset_state;
>> +	} else {
>> +		to = i915_gem_context_get(file->driver_priv, id);
>> +		if (to == NULL)
>> +			return -ENOENT;
>> +
>> +		*rs = &to->reset_state;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
>>   {
>>   	struct drm_i915_file_private *file_priv = file->driver_priv;
>>