From: "Jindal, Sonika" <sonika.jindal@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>,
"Daniel Vetter" <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation
Date: Mon, 13 Apr 2015 09:36:02 +0530 [thread overview]
Message-ID: <552B40AA.3040504@intel.com> (raw)
In-Reply-To: <20150410144419.GA28877@intel.com>
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@intel.com>
>>> Reviewed-by: Matt Roper <matthew.d.roper@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?
-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@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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2015-04-13 4:06 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-10 9:07 [PATCH 1/2] drm/i915/skl: Allow universal planes to position Sonika Jindal
2015-04-10 9:07 ` [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation Sonika Jindal
2015-04-10 14:17 ` Daniel Vetter
2015-04-10 14:44 ` Ville Syrjälä
2015-04-13 4:06 ` Jindal, Sonika [this message]
2015-04-13 10:10 ` Ville Syrjälä
2015-04-13 10:23 ` Jindal, Sonika
2015-04-13 10:49 ` Ville Syrjälä
2015-04-13 23:39 ` Matt Roper
2015-04-14 12:19 ` Jindal, Sonika
2015-04-14 17:27 ` Daniel Vetter
2015-04-15 10:05 ` [PATCH 1/2] drm/i915: Swapping 90 and 270 to be compliant with Xrandr Sonika Jindal
2015-04-15 10:05 ` [PATCH 2/2] Documentation/drm: Update rotation property with 90/270 and description Sonika Jindal
2015-04-15 10:27 ` Daniel Vetter
2015-04-15 10:29 ` Jindal, Sonika
2015-04-15 10:42 ` Daniel Vetter
2015-04-15 10:35 ` [PATCH] " Sonika Jindal
2015-05-12 12:50 ` [Intel-gfx] " Ville Syrjälä
2015-05-13 4:27 ` Jindal, Sonika
2015-05-20 6:39 ` Jindal, Sonika
2015-05-20 7:49 ` Jindal, Sonika
2015-05-20 14:03 ` Ville Syrjälä
2015-05-28 11:05 ` [PATCH] Documentation/drm: Update rotation property Sonika Jindal
2015-05-28 11:19 ` [Intel-gfx] " Daniel Vetter
2015-05-12 12:35 ` [PATCH 1/2] drm/i915: Swapping 90 and 270 to be compliant with Xrandr Ville Syrjälä
2015-05-20 8:10 ` [PATCH] " Sonika Jindal
2015-05-20 9:15 ` Daniel Vetter
2015-04-13 11:10 ` [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation Damien Lespiau
2015-04-13 4:02 ` Jindal, Sonika
2015-04-14 3:56 ` shuang.he
2015-10-06 13:32 ` [PATCH 1/2] drm/i915/skl: Allow universal planes to position Tvrtko Ursulin
2015-10-06 14:29 ` Matt Roper
2015-10-06 14:42 ` Ville Syrjälä
2015-10-06 15:16 ` Matt Roper
2015-10-06 16:28 ` Ville Syrjälä
2015-10-07 14:19 ` Daniel Vetter
2015-10-08 8:58 ` Tvrtko Ursulin
2015-10-16 12:23 ` Tvrtko Ursulin
-- strict thread matches above, loose matches on Subject: below --
2015-03-30 8:34 Sonika Jindal
2015-03-30 8:34 ` [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation Sonika Jindal
2015-03-30 12:15 ` shuang.he
2015-04-01 18:22 ` Matt Roper
2015-04-02 4:54 ` Jindal, Sonika
2015-04-02 8:55 ` Tvrtko Ursulin
2015-04-02 15:59 ` Matt Roper
2015-04-06 12:27 ` Jindal, Sonika
2015-04-07 8:13 ` Daniel Vetter
2015-04-07 8:22 ` Jindal, Sonika
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=552B40AA.3040504@intel.com \
--to=sonika.jindal@intel.com \
--cc=daniel@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox