Re: [PATCH 5/7] drm/i915/skl: Support secondary (rotated) frame buffer mapping

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

On 03/20/2015 12:01 PM, Joonas Lahtinen wrote:
> On to, 2015-03-19 at 15:07 +0000, Tvrtko Ursulin wrote:
>> Hi,
>>
>> On 03/19/2015 01:02 PM, Joonas Lahtinen wrote:
>>>>    static inline
>>>>    int i915_get_ggtt_vma_pages(struct i915_vma *vma)
>>>
>>> Same rant about function signatures as on earlier patch, put all on the
>>> same line like most of new the code has it.
>>
>> Ok.
>>
>>>>    struct i915_ggtt_view {
>>>>    	enum i915_ggtt_view_type type;
>>>>
>>>>    	struct sg_table *pages;
>>>> +
>>>> +	union {
>>>> +		struct intel_rotation_info rotation_info;
>>>> +	};
>>>
>>> In preparation for the memcmp way of comparing views, I would move this
>>> be before the variable struct parts (namely sg_table *pages), and also
>>> wrap it once more so the result would be like this:
>>>
>>> [snip]
>>> enum i915_ggtt_view_type type;
>>>
>>> union {
>>> 	struct {
>>> 		struct intel_rotation_info info;
>>> 	} rotated;
>>> 	struct {
>>> 		...
>>> 	} partial;
>>> };
>>>
>>> // private bits go here, to be wrapped in their struct with view
>>> // type comparing patches
>>>
>>> struct sg_table *pages;
>>> [snip]
>>>
>>> That way it's clear which view owns what.
>>
>> Hm, rotation info is not considered in comparing views, it is just a
>> bucket of data passed around between layers. So I suppose private data
>> under your design. Since there is no private union yet, maybe do this later?
>
> Why not? Isn't a 270 degree rotated view substantially different from a
> 90 degree rotated view (even when the difference technically is just
> some bit flip somewhere else).
>
> At least I would be pretty upset if I was returned the address for 90
> degree rotated view when I wanted 270 rotated. If multiple rotated views
> are not possible, then it is again an implicit thing.
>
> There are quite a lot of hardware constraints like this that appear in
> the code implicitly, which IMHO makes the code hard to follow at times.
> So I'd try to make it more explicit that the views are not the same,
> there just can be one rotated view at a time (if that is the case).

90 and 270 views are indeed the same page layout - same address for 
scanout. And there can only be one such VMA for an object at a time.

But how this mapping needs to look like is determined by more than the 
object itself - framebuffer geometry defines it. The private data in the 
view is used to transfer that meta-data so the GTT core can build the 
appropriate view.

That was my argument in fact for not putting the page shuffling bit in 
i915_gem_gtt.c since it is really display engine ownership.

Regards,

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