[Intel-gfx] [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation
Matt Roper
matthew.d.roper at intel.com
Wed Apr 1 11:22:08 PDT 2015
On Mon, Mar 30, 2015 at 02:04:57PM +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)
>
> Signed-off-by: Sonika Jindal <sonika.jindal at intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 2 +
> drivers/gpu/drm/i915/intel_display.c | 103 +++++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/intel_drv.h | 6 ++
> drivers/gpu/drm/i915/intel_sprite.c | 52 ++++++++++++-----
> 4 files changed, 129 insertions(+), 34 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_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f0bbc22..86ee0f0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2318,6 +2318,28 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
> return -EINVAL;
> }
>
> + switch (fb->pixel_format) {
> + case DRM_FORMAT_XRGB8888:
> + case DRM_FORMAT_ARGB8888:
> + case DRM_FORMAT_XBGR8888:
> + case DRM_FORMAT_ABGR8888:
> + case DRM_FORMAT_XRGB2101010:
> + case DRM_FORMAT_ARGB2101010:
> + case DRM_FORMAT_XBGR2101010:
> + case DRM_FORMAT_ABGR2101010:
> + case DRM_FORMAT_YUYV:
> + case DRM_FORMAT_YVYU:
> + case DRM_FORMAT_UYVY:
> + case DRM_FORMAT_VYUY:
> + case DRM_FORMAT_NV12:
> + break;
> +
> + default:
> + DRM_DEBUG_KMS("Unsupported pixel format:%d for 90/270 rotation!\n",
> + fb->pixel_format);
> + return -EINVAL;
> + }
Shouldn't we be matching against the list of formats the plane supports
(which may vary by platform, or by specific plane) rather than this
generic list? We already specified what formats the plane can handle at
plane init time, so it seems like what you'd really want is just a call
to
drm_plane_check_pixel_format(plane_state->plane, fb->pixel_format)
then follow that up with explicit checks to exclude any formats that we
can handle in 0/180, but not in 90/270.
I'd also move this check to intel_plane_atomic_check(), since the
'check' code path is where I'd usually go looking for these types of
checks; the function you've got it in at the moment gets called from the
'prepare' step which works as well, but seems a bit less obvious.
> +
> return 0;
> }
>
> @@ -2919,8 +2941,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 +3007,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);
Minor nit; can we move this down inside the 'else' branch to make it
apparent how this serves a parallel role to 'tile_height' from the 'if'
branch.
> - surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
> + surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
> +
> + if (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) {
Might as well use the intel_rotation_90_or_270() function since we have
it. Same comment for the sprite path farther down.
> + /* 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 +12462,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..65eb147 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 (rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) {
> + /* 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
More information about the Intel-gfx
mailing list