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

[Joonas Lahtinen]

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).

> 
> >>   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.
> 

Yes, my bad, I got:)

Regards, Joonas

> Regards,
> 
> Tvrtko