[Intel-gfx] [PATCH 2/2] drm/i915: Add 180 degree primary plane rotation support
Jindal, Sonika
sonika.jindal at intel.com
Wed Aug 20 07:36:10 CEST 2014
On 8/20/2014 4:21 AM, Matt Roper wrote:
> On Tue, Aug 19, 2014 at 11:56:43AM +0530, sonika.jindal at intel.com wrote:
>> From: Sonika Jindal <sonika.jindal at intel.com>
>>
>> Primary planes support 180 degree rotation. Expose the feature
>> through rotation drm property.
>>
>> v2: Calculating linear/tiled offsets based on pipe source width and
>> height. Added 180 degree rotation support in ironlake_update_plane.
>>
>> v3: Checking if CRTC is active before issueing update_plane. Added
>> wait for vblank to make sure we dont overtake page flips. Disabling
>> FBC since it does not work with rotated planes.
>>
>> v4: Updated rotation checks for pending flips, fbc disable. Creating
>> rotation property only for Gen4 onwards. Property resetting as part
>> of lastclose.
>>
>> v5: Resetting property in i915_driver_lastclose properly for planes
>> and crtcs. Fixed linear offset calculation that was off by 1 w.r.t
>> width in i9xx_update_plane and ironlake_update_plane. Removed tab
>> based indentation and unnecessary braces in intel_crtc_set_property
>> and intel_update_fbc. FBC and flip related checks should be done only
>> for valid crtcs.
>>
>> v6: Minor nits in FBC disable checks for comments in intel_crtc_set_property
>> and positioning the disable code in intel_update_fbc.
>>
>> v7: In case rotation property on inactive crtc is updated, we return
>> successfully printing debug log as crtc is inactive and only property change
>> is preserved.
>>
>> v8: update_plane is changed to update_primary_plane, crtc->fb is changed to
>> crtc->primary->fb and return value of update_primary_plane is ignored.
>>
>> v9: added rotation property to primary plane instead of crtc. Removing reset
>> of rotation property from lastclose. rotation_property is moved to
>> drm_mode_config, so drm layer will take care of resetting. Adding updation of
>> fbc when rotation is set to 0. Allowing rotation only if value is
>> different than old one.
>>
>> v10: Calling intel_primary_plane_setplane instead of update_primary_plane in
>> set_property(Daniel).
>>
>> Testcase: igt/kms_rotation_crc
>>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com>
>> Signed-off-by: Sonika Jindal <sonika.jindal at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 1 +
>> drivers/gpu/drm/i915/intel_display.c | 116 ++++++++++++++++++++++++++++++++--
>> drivers/gpu/drm/i915/intel_pm.c | 6 ++
>> 3 files changed, 119 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 7a6cc69..f8aaf0b 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4209,6 +4209,7 @@ enum punit_power_well {
>> #define DISPPLANE_NO_LINE_DOUBLE 0
>> #define DISPPLANE_STEREO_POLARITY_FIRST 0
>> #define DISPPLANE_STEREO_POLARITY_SECOND (1<<18)
>> +#define DISPPLANE_ROTATE_180 (1<<15)
>> #define DISPPLANE_TRICKLE_FEED_DISABLE (1<<14) /* Ironlake */
>> #define DISPPLANE_TILED (1<<10)
>> #define _DSPAADDR 0x70184
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 1b0e403..cc2999b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2332,6 +2332,9 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>> unsigned long linear_offset;
>> u32 dspcntr;
>> u32 reg = DSPCNTR(plane);
>> + int pixel_size;
>> +
>> + pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>>
>> if (!intel_crtc->primary_enabled) {
>> I915_WRITE(reg, 0);
>> @@ -2359,6 +2362,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>> (intel_crtc->config.pipe_src_w - 1));
>> I915_WRITE(DSPPOS(plane), 0);
>> }
>> + dspcntr &= ~DISPPLANE_ROTATE_180;
>>
>> switch (fb->pixel_format) {
>> case DRM_FORMAT_C8:
>> @@ -2398,8 +2402,6 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>> if (IS_G4X(dev))
>> dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
>>
>> - I915_WRITE(reg, dspcntr);
>> -
>> linear_offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
>>
>> if (INTEL_INFO(dev)->gen >= 4) {
>> @@ -2412,6 +2414,21 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>> intel_crtc->dspaddr_offset = linear_offset;
>> }
>>
>> + if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
>> + dspcntr |= DISPPLANE_ROTATE_180;
>> +
>> + x += (intel_crtc->config.pipe_src_w - 1);
>> + y += (intel_crtc->config.pipe_src_h - 1);
>> +
>> + /* Finding the last pixel of the last line of the display
>> + data and adding to linear_offset*/
>> + linear_offset += (intel_crtc->config.pipe_src_h - 1) *
>> + fb->pitches[0] +
>> + (intel_crtc->config.pipe_src_w - 1) * pixel_size;
>> + }
>> +
>> + I915_WRITE(reg, dspcntr);
>> +
>> DRM_DEBUG_KMS("Writing base %08lX %08lX %d %d %d\n",
>> i915_gem_obj_ggtt_offset(obj), linear_offset, x, y,
>> fb->pitches[0]);
>> @@ -2438,6 +2455,9 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>> unsigned long linear_offset;
>> u32 dspcntr;
>> u32 reg = DSPCNTR(plane);
>> + int pixel_size;
>> +
>> + pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>>
>> if (!intel_crtc->primary_enabled) {
>> I915_WRITE(reg, 0);
>> @@ -2453,6 +2473,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>> if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>> dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
>>
>> + dspcntr &= ~DISPPLANE_ROTATE_180;
>> +
>> switch (fb->pixel_format) {
>> case DRM_FORMAT_C8:
>> dspcntr |= DISPPLANE_8BPP;
>> @@ -2486,14 +2508,26 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>> if (!IS_HASWELL(dev) && !IS_BROADWELL(dev))
>> dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
>>
>> - I915_WRITE(reg, dspcntr);
>> -
>> linear_offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
>> intel_crtc->dspaddr_offset =
>> intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
>> fb->bits_per_pixel / 8,
>> fb->pitches[0]);
>> linear_offset -= intel_crtc->dspaddr_offset;
>> + if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
>> + dspcntr |= DISPPLANE_ROTATE_180;
>> +
>> + if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
>> + x += (intel_crtc->config.pipe_src_w - 1);
>> + y += (intel_crtc->config.pipe_src_h - 1);
>> + linear_offset +=
>> + (intel_crtc->config.pipe_src_h - 1) *
>> + fb->pitches[0] + (intel_crtc->config.pipe_src_w - 1) *
>> + pixel_size;
>> + }
>> + }
>> +
>> + I915_WRITE(reg, dspcntr);
>>
>> DRM_DEBUG_KMS("Writing base %08lX %08lX %d %d %d\n",
>> i915_gem_obj_ggtt_offset(obj), linear_offset, x, y,
>> @@ -11647,10 +11681,70 @@ static void intel_plane_destroy(struct drm_plane *plane)
>> kfree(intel_plane);
>> }
>>
>> +static int intel_primary_plane_set_property(struct drm_plane *plane,
>> + struct drm_property *prop,
>> + uint64_t val)
>> +{
>> + struct drm_device *dev = plane->dev;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + struct intel_plane *intel_plane = to_intel_plane(plane);
>> + struct drm_crtc *crtc = plane->crtc;
>> + struct intel_crtc *intel_crtc;
>> + uint64_t old_val;
>> +
>> + if (prop == dev->mode_config.rotation_property) {
>> + /* exactly one rotation angle please */
>> + if (hweight32(val & 0xf) != 1)
>> + return -EINVAL;
>> +
>> + old_val = intel_plane->rotation;
>> + intel_plane->rotation = val;
>> +
>> + if (old_val == intel_plane->rotation)
>> + return 0;
>> +
>> + if (crtc == NULL)
>> + return -EINVAL;
>
> For sprite planes it looks like we allow this property to be updated
> while the plane is off (no FB assigned and thus no crtc pointer) without
> error. Is it intentional that the primary plane behaves differently
> here? Note that you're returning -EINVAL here, but you still leave the
> intel_plane->rotation value you set above, which seems a bit odd.
>
I think it is ok to leave the rotation updated because in the next
update it will be reflected. The check is to avoid calling of setplane
if the crtc is inactive.
>> +
>> + intel_crtc = to_intel_crtc(plane->crtc);
>> +
>> + if (intel_crtc && intel_crtc->active &&
>> + intel_crtc->primary_enabled) {
>> + intel_crtc_wait_for_pending_flips(crtc);
>> +
>> + /* FBC does not work on some platforms for rotated
>> + planes, so disable it when rotation is not 0 and update
>> + it when rotation is set back to 0 */
>> + if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) {
>> + if (dev_priv->fbc.plane == intel_crtc->plane &&
>> + intel_plane->rotation != BIT(DRM_ROTATE_0))
>> + intel_disable_fbc(dev);
>> + else if (intel_plane->rotation == BIT(DRM_ROTATE_0)) {
>> + mutex_lock(&dev->struct_mutex);
>> + intel_update_fbc(dev);
>> + mutex_unlock(&dev->struct_mutex);
>> + }
>> + }
>> +
>> + intel_primary_plane_setplane(plane, crtc, crtc->primary->fb,
>> + intel_plane->crtc_x, intel_plane->crtc_y,
>> + intel_plane->crtc_w, intel_plane->crtc_h,
>> + intel_plane->src_x, intel_plane->src_y,
>> + intel_plane->src_w, intel_plane->src_h);
>> +
>> + } else {
>> + DRM_DEBUG_KMS("[CRTC:%d] inactive. Only rotation"
>> + "property is updated\n", crtc->base.id);
>> + }
>> + }
>> + return 0;
>> +}
>
> I haven't followed this thread very carefully, so sorry if this has
> already been discussed, but could you just use the same set_property()
> handler for both primary and sprite planes to avoid duplicate effort?
> It seems like you'd only need to make a few changes:
>
> * Move the wait for asynchronous flips and FBC stuff here into an "if
> (plane type is primary)" block
> * Call intel_plane_restore() here rather than
> intel_primary_plane_setplane()
> * Update intel_plane_restore() such that it calls
> drm_plane->funcs->update_plane() rather than intel_update_plane()
> directly.
>
> I'm sure in the future we'll add other properties that we'll want to
> handle on both primary and sprite (and cursor) planes, so having a
> single set_property handler might allow us to cut down on needing to
> duplicate future code in multiple places.
>
Hmm, yes having single set_property function might be right approach.
Since, it all started with crtc property and not a separate primary
plane property they are maintained separately.
> On the cosmetic side, I also kind of feel like it might make sense to
> rename intel_plane_restore() to intel_plane_reprogram() since that seems
> like a more accurate description of what we're using the function for
> now.
>
>
> Matt
>
>> +
>> static const struct drm_plane_funcs intel_primary_plane_funcs = {
>> .update_plane = intel_primary_plane_setplane,
>> .disable_plane = intel_primary_plane_disable,
>> .destroy = intel_plane_destroy,
>> + .set_property = intel_primary_plane_set_property
>> };
>>
>> static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>> @@ -11668,6 +11762,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>> primary->max_downscale = 1;
>> primary->pipe = pipe;
>> primary->plane = pipe;
>> + primary->rotation = BIT(DRM_ROTATE_0);
>> if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
>> primary->plane = !pipe;
>>
>> @@ -11683,6 +11778,19 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>> &intel_primary_plane_funcs,
>> 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,
>> + primary->rotation);
>> + }
>> +
>> return &primary->base;
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 12f4e14..12f5b99 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -581,6 +581,12 @@ void intel_update_fbc(struct drm_device *dev)
>> DRM_DEBUG_KMS("framebuffer not tiled or fenced, disabling compression\n");
>> goto out_disable;
>> }
>> + if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
>> + to_intel_plane(crtc->primary)->rotation != BIT(DRM_ROTATE_0)) {
>> + if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE))
>> + DRM_DEBUG_KMS("Rotation unsupported, disabling\n");
>> + goto out_disable;
>> + }
>>
>> /* If the kernel debugger is active, always disable compression */
>> if (in_dbg_master())
>> --
>> 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