Re: [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 14/10/15 16:33, Chris Wilson wrote:
> On Wed, Oct 14, 2015 at 04:08:25PM +0100, Tvrtko Ursulin wrote:
>>
>> Hi,
>>
>> On 14/10/15 15:51, Daniel Vetter wrote:
>>> The rotated view depends upon the rotation paramters, but thus far we
>>> didn't bother checking for those. This seems to have been an issue
>>> ever since this was introduce in
>>>
>>> commit fe14d5f4e5468c5b80a24f1a64abcbe116143670
>>> Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Date:   Wed Dec 10 17:27:58 2014 +0000
>>>
>>>      drm/i915: Infrastructure for supporting different GGTT views per object
>>>
>>> But userspace is allowed to reuse framebuffer backing storage with
>>> different framebuffers with different pixel formats/stride/whatever.
>>> And e.g. SNA indeed does this. Hence we must check for all the
>>> paramters to match, not just that it's rotated.
>>>
>>> v2: intel_plane_obj_offset also needs to construct the full view, to
>>> avoid fallout since they don't fully match.
>>>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem_gtt.h  |  2 +-
>>>   drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
>>>   2 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
>>> index 2e1f6493c9e7..8a36f4fcc676 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>>> @@ -551,7 +551,7 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
>>>
>>>   	if (a->type != b->type)
>>>   		return false;
>>> -	if (a->type == I915_GGTT_VIEW_PARTIAL)
>>> +	if (a->type != I915_GGTT_VIEW_NORMAL)
>>>   		return !memcmp(&a->params, &b->params, sizeof(a->params));
>>>   	return true;
>>>   }
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 57459fedf216..2a5987ce576c 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -2894,16 +2894,16 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>>>   				     struct drm_i915_gem_object *obj,
>>>   				     unsigned int plane)
>>>   {
>>> -	const struct i915_ggtt_view *view = &i915_ggtt_view_normal;
>>> +	struct i915_ggtt_view view;
>>>   	struct i915_vma *vma;
>>>   	unsigned char *offset;
>>>
>>> -	if (intel_rotation_90_or_270(intel_plane->base.state->rotation))
>>> -		view = &i915_ggtt_view_rotated;
>>> +	intel_fill_fb_ggtt_view(&view, intel_plane->base.fb,
>>> +				intel_plane->base.state);
>>>
>>> -	vma = i915_gem_obj_to_ggtt_view(obj, view);
>>> +	vma = i915_gem_obj_to_ggtt_view(obj, &view);
>>>   	if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n",
>>> -		view->type))
>>> +		view.type))
>>>   		return -1;
>>>
>>>   	offset = (unsigned char *)vma->node.start;
>>>
>>
>> As we discussed on IRC I had wrong assumptions when developing this.
>> Luckily SNA is not using hardware 90/270 yet so we are safe there.
>> And Android probably doesn't reuse the fb obj or it would have been
>> reported. But I'll check.
>>
>> So thanks for the cleanup! For all three:
>>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Just a shame this means so much more computation in
>> intel_plane_obj_offset, really highlights that vma should be stored
>> in the state, if it is possible.
>
> On your todo list is reviewing the patches that eliminate
> intel_plane_obj_offset.
> :-p

Have I missed something? I thought I reviewed what you sent so far.

Regards,

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