[Intel-gfx] [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation
Matt Roper
matthew.d.roper at intel.com
Mon Apr 13 16:39:10 PDT 2015
On Mon, Apr 13, 2015 at 01:49:22PM +0300, Ville Syrjälä wrote:
> 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 at intel.com>
> > >>>>> Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> > >>>>
> > >>>> 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.
It seems like this information needs to be added to the property section
of the DRM DocBook; continuing to follow XRandR probably makes sense if
that's what's agreed on, but there's no indication of that design
philosophy in the actual DRM documentation today, so we're in danger of
having different driver authors use conflicting interpretations.
90/270 rotation is already supported by existing drivers (omap for
several years now and atmel-hlcdc just recently). I think it's too late
to try to redefine the meaning of rotation property values that are
already in active use, so we probably need to check whether omap is
using a cw or ccw scheme and follow (and document!) that.
Matt
>
> >
> > >>
> > >> -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 at lists.freedesktop.org
> > >>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >>>>
> > >>>> --
> > >>>> Daniel Vetter
> > >>>> Software Engineer, Intel Corporation
> > >>>> http://blog.ffwll.ch
> > >>>> _______________________________________________
> > >>>> Intel-gfx mailing list
> > >>>> Intel-gfx at lists.freedesktop.org
> > >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >>>
> > >
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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