[Intel-gfx] [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation
Jindal, Sonika
sonika.jindal at intel.com
Mon Apr 13 03:23:22 PDT 2015
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.
>>
>> -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
>>>
>
More information about the Intel-gfx
mailing list