Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Apr 13, 2015 at 03:53:22PM +0530, Jindal, Sonika wrote:
> 
> 
> On 4/13/2015 3:40 PM, Ville Syrjälä wrote:
> > On Mon, Apr 13, 2015 at 09:36:02AM +0530, Jindal, Sonika wrote:
> >>
> >>
> >> On 4/10/2015 8:14 PM, Ville Syrjälä wrote:
> >>> On Fri, Apr 10, 2015 at 04:17:17PM +0200, Daniel Vetter wrote:
> >>>> On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote:
> >>>>> v2: Moving creation of property in a function, checking for 90/270
> >>>>> rotation simultaneously (Chris)
> >>>>> Letting primary plane to be positioned
> >>>>> v3: Adding if/else for 90/270 and rest params programming, adding check for
> >>>>> pixel_format, some cleanup (review comments)
> >>>>> v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
> >>>>> and size programming (Ville)
> >>>>> v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
> >>>>> v6: Rebased on -nightly (Tvrtko's series merged)
> >>>>> v7: Moving pixel_format check to intel_atomic_plane_check (Matt)
> >>>>>
> >>>>> Signed-off-by: Sonika Jindal <sonika.jindal@xxxxxxxxx>
> >>>>> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> >>>>
> >>>> Patches are missing the Testcase: tag, please add that. Also, are all the
> >>>> igt committed or not yet? Pulled these two in meanwhile.
> >>>
> >>> Things are going somewhat broken because you didn't apply my plane
> >>> state stuff. Hmm. Actually it sort of looks that it might work by luck
> >>> as long as the primary plane doesn't get clipped since this is bashing
> >>> the user state directly into the hardware registers and not the derived
> >>> state (ie. clipped coordinates).
> >>>
> >> I was hoping your changes get merged, but not sure why they are held up.
> >>
> >>> Also I see my review comment about the 90 vs. 270 rotation mixup was
> >>> ignored at least.
> >>>
> >> I never really got the to understand the need of reversing 90 and 270 :)
> >> The last discussion was not concluded, AFAIR.
> >> Things look correct to me and work fine with the testcase also.
> >> Is there something completely different which I am unable to get?
> >
> > BSpec gives me the impression the hw rotation is cw, whereas the drm one
> > is ccw.
> >
> Yes, HW rotation is cw as per bspec. In drm, where do we consider it as 
> anti-cw? I assume it is cw because all the macros work as expected, ie cw.

The ccw rule was inherited from XRandR. I'm not sure what macros you
mean, but drm_rect_rotate() will certainly give you wrong results if
you assume cw.

> 
> >>
> >> -Sonika
> >>
> >>>> -Daniel
> >>>>
> >>>>> ---
> >>>>>    drivers/gpu/drm/i915/i915_reg.h           |    2 +
> >>>>>    drivers/gpu/drm/i915/intel_atomic_plane.c |   24 ++++++++
> >>>>>    drivers/gpu/drm/i915/intel_display.c      |   88 ++++++++++++++++++++---------
> >>>>>    drivers/gpu/drm/i915/intel_drv.h          |    6 ++
> >>>>>    drivers/gpu/drm/i915/intel_sprite.c       |   52 ++++++++++++-----
> >>>>>    5 files changed, 131 insertions(+), 41 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>>>> index b522eb6..564bbd5 100644
> >>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>>>> @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells {
> >>>>>    #define   PLANE_CTL_ALPHA_HW_PREMULTIPLY	(  3 << 4)
> >>>>>    #define   PLANE_CTL_ROTATE_MASK			0x3
> >>>>>    #define   PLANE_CTL_ROTATE_0			0x0
> >>>>> +#define   PLANE_CTL_ROTATE_90			0x1
> >>>>>    #define   PLANE_CTL_ROTATE_180			0x2
> >>>>> +#define   PLANE_CTL_ROTATE_270			0x3
> >>>>>    #define _PLANE_STRIDE_1_A			0x70188
> >>>>>    #define _PLANE_STRIDE_2_A			0x70288
> >>>>>    #define _PLANE_STRIDE_3_A			0x70388
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>>> index 976b891..a27ee8c 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>>> @@ -162,6 +162,30 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >>>>>    			(1 << drm_plane_index(plane));
> >>>>>    	}
> >>>>>
> >>>>> +	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
> >>>>> +		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> >>>>> +			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> >>>>> +			DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> >>>>> +			return -EINVAL;
> >>>>> +		}
> >>>>> +
> >>>>> +		/*
> >>>>> +		 * 90/270 is not allowed with RGB64 16:16:16:16,
> >>>>> +		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
> >>>>> +		 * TBD: Add RGB64 case once its added in supported format list.
> >>>>> +		 */
> >>>>> +		switch (state->fb->pixel_format) {
> >>>>> +		case DRM_FORMAT_C8:
> >>>>> +		case DRM_FORMAT_RGB565:
> >>>>> +			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
> >>>>> +					drm_get_format_name(state->fb->pixel_format));
> >>>>> +			return -EINVAL;
> >>>>> +
> >>>>> +		default:
> >>>>> +			break;
> >>>>> +		}
> >>>>> +	}
> >>>>> +
> >>>>>    	return intel_plane->check_plane(plane, intel_state);
> >>>>>    }
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>> index f0bbc22..4de544c 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>> @@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
> >>>>>    	info->pitch = fb->pitches[0];
> >>>>>    	info->fb_modifier = fb->modifier[0];
> >>>>>
> >>>>> -	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
> >>>>> -	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
> >>>>> -		DRM_DEBUG_KMS(
> >>>>> -			      "Y or Yf tiling is needed for 90/270 rotation!\n");
> >>>>> -		return -EINVAL;
> >>>>> -	}
> >>>>> -
> >>>>>    	return 0;
> >>>>>    }
> >>>>>
> >>>>> @@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >>>>>    	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>>>>    	struct drm_i915_gem_object *obj;
> >>>>>    	int pipe = intel_crtc->pipe;
> >>>>> -	u32 plane_ctl, stride_div;
> >>>>> +	u32 plane_ctl, stride_div, stride;
> >>>>> +	u32 tile_height, plane_offset, plane_size;
> >>>>> +	unsigned int rotation;
> >>>>> +	int x_offset, y_offset;
> >>>>>    	unsigned long surf_addr;
> >>>>> +	struct drm_plane *plane;
> >>>>>
> >>>>>    	if (!intel_crtc->primary_enabled) {
> >>>>>    		I915_WRITE(PLANE_CTL(pipe, 0), 0);
> >>>>> @@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >>>>>    	}
> >>>>>
> >>>>>    	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> >>>>> -	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
> >>>>> +
> >>>>> +	plane = crtc->primary;
> >>>>> +	rotation = plane->state->rotation;
> >>>>> +	switch (rotation) {
> >>>>> +	case BIT(DRM_ROTATE_90):
> >>>>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
> >>>>> +		break;
> >>>>> +
> >>>>> +	case BIT(DRM_ROTATE_180):
> >>>>>    		plane_ctl |= PLANE_CTL_ROTATE_180;
> >>>>> +		break;
> >>>>> +
> >>>>> +	case BIT(DRM_ROTATE_270):
> >>>>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
> >>>>> +		break;
> >>>>> +	}
> >>>>>
> >>>>>    	obj = intel_fb_obj(fb);
> >>>>>    	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> >>>>>    					       fb->pixel_format);
> >>>>> -	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
> >>>>> +	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
> >>>>> +
> >>>>> +	if (intel_rotation_90_or_270(rotation)) {
> >>>>> +		/* stride = Surface height in tiles */
> >>>>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> >>>>> +						fb->modifier[0]);
> >>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> >>>>> +		x_offset = stride * tile_height - y - (plane->state->src_h >> 16);
> >>>>> +		y_offset = x;
> >>>>> +		plane_size = ((plane->state->src_w >> 16) - 1) << 16 |
> >>>>> +					((plane->state->src_h >> 16) - 1);
> >>>>> +	} else {
> >>>>> +		stride = fb->pitches[0] / stride_div;
> >>>>> +		x_offset = x;
> >>>>> +		y_offset = y;
> >>>>> +		plane_size = ((plane->state->src_h >> 16) - 1) << 16 |
> >>>>> +			((plane->state->src_w >> 16) - 1);
> >>>>> +	}
> >>>>> +	plane_offset = y_offset << 16 | x_offset;
> >>>>>
> >>>>>    	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> >>>>>    	I915_WRITE(PLANE_POS(pipe, 0), 0);
> >>>>> -	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
> >>>>> -	I915_WRITE(PLANE_SIZE(pipe, 0),
> >>>>> -		   (intel_crtc->config->pipe_src_h - 1) << 16 |
> >>>>> -		   (intel_crtc->config->pipe_src_w - 1));
> >>>>> -	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
> >>>>> +	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
> >>>>> +	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
> >>>>> +	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> >>>>>    	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
> >>>>>
> >>>>>    	POSTING_READ(PLANE_SURF(pipe, 0));
> >>>>> @@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >>>>>    				 intel_primary_formats, num_formats,
> >>>>>    				 DRM_PLANE_TYPE_PRIMARY);
> >>>>>
> >>>>> -	if (INTEL_INFO(dev)->gen >= 4) {
> >>>>> -		if (!dev->mode_config.rotation_property)
> >>>>> -			dev->mode_config.rotation_property =
> >>>>> -				drm_mode_create_rotation_property(dev,
> >>>>> -							BIT(DRM_ROTATE_0) |
> >>>>> -							BIT(DRM_ROTATE_180));
> >>>>> -		if (dev->mode_config.rotation_property)
> >>>>> -			drm_object_attach_property(&primary->base.base,
> >>>>> -				dev->mode_config.rotation_property,
> >>>>> -				state->base.rotation);
> >>>>> -	}
> >>>>> +	if (INTEL_INFO(dev)->gen >= 4)
> >>>>> +		intel_create_rotation_property(dev, primary);
> >>>>>
> >>>>>    	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> >>>>>
> >>>>>    	return &primary->base;
> >>>>>    }
> >>>>>
> >>>>> +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
> >>>>> +{
> >>>>> +	if (!dev->mode_config.rotation_property) {
> >>>>> +		unsigned long flags = BIT(DRM_ROTATE_0) |
> >>>>> +			BIT(DRM_ROTATE_180);
> >>>>> +
> >>>>> +		if (INTEL_INFO(dev)->gen >= 9)
> >>>>> +			flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
> >>>>> +
> >>>>> +		dev->mode_config.rotation_property =
> >>>>> +			drm_mode_create_rotation_property(dev, flags);
> >>>>> +	}
> >>>>> +	if (dev->mode_config.rotation_property)
> >>>>> +		drm_object_attach_property(&plane->base.base,
> >>>>> +				dev->mode_config.rotation_property,
> >>>>> +				plane->base.state->rotation);
> >>>>> +}
> >>>>> +
> >>>>>    static int
> >>>>>    intel_check_cursor_plane(struct drm_plane *plane,
> >>>>>    			 struct intel_plane_state *state)
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>>>> index 811a1db..d32025a 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>>>> @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation)
> >>>>>    	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
> >>>>>    }
> >>>>>
> >>>>> +unsigned int
> >>>>> +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
> >>>>> +		  uint64_t fb_modifier);
> >>>>> +void intel_create_rotation_property(struct drm_device *dev,
> >>>>> +					struct intel_plane *plane);
> >>>>> +
> >>>>>    bool intel_wm_need_update(struct drm_plane *plane,
> >>>>>    			  struct drm_plane_state *state);
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >>>>> index f41e872..83adc9b 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>>>> @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >>>>>    	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >>>>>    	const int pipe = intel_plane->pipe;
> >>>>>    	const int plane = intel_plane->plane + 1;
> >>>>> -	u32 plane_ctl, stride_div;
> >>>>> +	u32 plane_ctl, stride_div, stride;
> >>>>>    	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> >>>>>    	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
> >>>>>    	unsigned long surf_addr;
> >>>>> +	u32 tile_height, plane_offset, plane_size;
> >>>>> +	unsigned int rotation;
> >>>>> +	int x_offset, y_offset;
> >>>>>
> >>>>>    	plane_ctl = PLANE_CTL_ENABLE |
> >>>>>    		PLANE_CTL_PIPE_CSC_ENABLE;
> >>>>> @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >>>>>    		MISSING_CASE(fb->modifier[0]);
> >>>>>    	}
> >>>>>
> >>>>> -	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
> >>>>> +	rotation = drm_plane->state->rotation;
> >>>>> +	switch (rotation) {
> >>>>> +	case BIT(DRM_ROTATE_90):
> >>>>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
> >>>>> +		break;
> >>>>> +
> >>>>> +	case BIT(DRM_ROTATE_180):
> >>>>>    		plane_ctl |= PLANE_CTL_ROTATE_180;
> >>>>> +		break;
> >>>>> +
> >>>>> +	case BIT(DRM_ROTATE_270):
> >>>>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
> >>>>> +		break;
> >>>>> +	}
> >>>>>
> >>>>>    	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
> >>>>>    				       pixel_size, true,
> >>>>> @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >>>>>
> >>>>>    	surf_addr = intel_plane_obj_offset(intel_plane, obj);
> >>>>>
> >>>>> -	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
> >>>>> -	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
> >>>>> +	if (intel_rotation_90_or_270(rotation)) {
> >>>>> +		/* stride: Surface height in tiles */
> >>>>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> >>>>> +							fb->modifier[0]);
> >>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> >>>>> +		plane_size = (src_w << 16) | src_h;
> >>>>> +		x_offset = stride * tile_height - y - (src_h + 1);
> >>>>> +		y_offset = x;
> >>>>> +	} else {
> >>>>> +		stride = fb->pitches[0] / stride_div;
> >>>>> +		plane_size = (src_h << 16) | src_w;
> >>>>> +		x_offset = x;
> >>>>> +		y_offset = y;
> >>>>> +	}
> >>>>> +	plane_offset = y_offset << 16 | x_offset;
> >>>>> +
> >>>>> +	I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
> >>>>> +	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
> >>>>>    	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
> >>>>> -	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
> >>>>> +	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
> >>>>>    	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
> >>>>>    	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
> >>>>>    	POSTING_READ(PLANE_SURF(pipe, plane));
> >>>>> @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >>>>>    		goto out;
> >>>>>    	}
> >>>>>
> >>>>> -	if (!dev->mode_config.rotation_property)
> >>>>> -		dev->mode_config.rotation_property =
> >>>>> -			drm_mode_create_rotation_property(dev,
> >>>>> -							  BIT(DRM_ROTATE_0) |
> >>>>> -							  BIT(DRM_ROTATE_180));
> >>>>> -
> >>>>> -	if (dev->mode_config.rotation_property)
> >>>>> -		drm_object_attach_property(&intel_plane->base.base,
> >>>>> -					   dev->mode_config.rotation_property,
> >>>>> -					   state->base.rotation);
> >>>>> +	intel_create_rotation_property(dev, intel_plane);
> >>>>>
> >>>>>    	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> >>>>>
> >>>>> --
> >>>>> 1.7.10.4
> >>>>>
> >>>>> _______________________________________________
> >>>>> Intel-gfx mailing list
> >>>>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>>
> >>>> --
> >>>> Daniel Vetter
> >>>> Software Engineer, Intel Corporation
> >>>> http://blog.ffwll.ch
> >>>> _______________________________________________
> >>>> Intel-gfx mailing list
> >>>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux