[Intel-gfx] [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation
Jindal, Sonika
sonika.jindal at intel.com
Mon Apr 6 05:27:03 PDT 2015
On 4/2/2015 9:29 PM, Matt Roper wrote:
> On Thu, Apr 02, 2015 at 10:24:02AM +0530, Jindal, Sonika wrote:
>>
>>
>> On 4/1/2015 11:52 PM, Matt Roper wrote:
>>> 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 am not sure how it will help. drm_plane_check_pixel_format should
>> be used to check the pixel format of the fb which we should be doing
>> in some -check functions (I don't think we do that right now?)
>> against what is supported by the plane.
>> But to check for the formats which are allowed for 90/270, we would
>> need this kind of explicit check.
>
> Right, I guess there are two aspects here. First, we need to properly
> test for acceptable pixel formats for the plane in general; at the
> moment the DRM core setplane() tests this, but if we use the atomic
> ioctl it never gets checked (which is a bug). So as you say, we need a
> test in a _check() function to verify this. We probably also need to
> add an i-g-t test for it too.
>
OK, I'l add that check in intel_atomic_plane_check()
> Once we know that the pixel format is valid in general, it makes sense
> to have a simpler test to reject some subset of those formats iff we
> notice we're doing 90/270 rotation. Maybe it's not really a big deal,
> but it seems like that's a little easier to understand and verify than
> having two completely separate lists, especially when future platforms
> may support different formats, or even different planes of the same
> platform have varying pixel format capabilities.
>
I will send out a patch with the changes and add negative test case in
kms_rotation_crc.
Regards,
Sonika
>
> Matt
>
>>> 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.
>>>
>> Yes, I agree, but this is on top of Tvrtko's patch for secondary
>> buffer mapping where based upon tiling and pixel format we are
>> allowing the rotated gtt.
>>
>> Tvrtko,
>> Can these be moved to the intel_plane_atomic_check()
>>
>>>> +
>>>> 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.
>>>
>> Yes sure.
>>>
>>>> - 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.
>>>
>> Sure.
>>
>> Regards,
>> Sonika
>>>> + /* 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
>>>
>
More information about the Intel-gfx
mailing list