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

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

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?

>>   extern const struct i915_ggtt_view i915_ggtt_view_normal;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index fe11e99..c2c3a76 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2194,7 +2194,7 @@ static bool need_vtd_wa(struct drm_device *dev)
>>   	return false;
>>   }
>>
>> -static unsigned int
>> +unsigned int
>>   intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
>>   		  uint64_t fb_format_modifier)
>>   {
>> @@ -2254,9 +2254,34 @@ int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
>>   			    struct drm_framebuffer *fb,
>>   			    const struct drm_plane_state *plane_state)
>>   {
>> +	struct intel_rotation_info *info = &view->rotation_info;
>> +	static const struct i915_ggtt_view rotated_view =
>> +				{ .type = I915_GGTT_VIEW_ROTATED };
>> +
>>   	*view = i915_ggtt_view_normal;
>>
>> -	return 0;
>> +	if (!plane_state)
>> +		return 0;
>> +
>> +	if (!(plane_state->rotation &
>> +	    (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))))
>> +		return 0;
>
> Should the rotation checking helper introduced in previous patch be used
> here too?

It's the other way round, following patch adds the helper and replaces 
this with a call to it.

Regards,

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