| Message ID | 1405413629-4266-6-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal@intel.com wrote: > From: Sonika Jindal <sonika.jindal@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. > > Testcase: igt/kms_rotation_crc > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> Some stuff I've noticed below. -Daniel > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_display.c | 104 ++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_pm.c | 6 ++ > 3 files changed, 107 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 74283d5..f39e2e7 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4107,6 +4107,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 023c770..e881dcf 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2408,11 +2408,15 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, > unsigned long linear_offset; > u32 dspcntr; > u32 reg; > + int pixel_size; > + > + pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > > reg = DSPCNTR(plane); > dspcntr = I915_READ(reg); > /* Mask out pixel format bits in case we change it */ > dspcntr &= ~DISPPLANE_PIXFORMAT_MASK; > + dspcntr &= ~DISPPLANE_ROTATE_180; > switch (fb->pixel_format) { > case DRM_FORMAT_C8: > dspcntr |= DISPPLANE_8BPP; > @@ -2454,8 +2458,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) { > @@ -2467,6 +2469,17 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, > } else { > 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); > + linear_offset += (intel_crtc->config.pipe_src_h - 1) * > + fb->pitches[0] + > + (intel_crtc->config.pipe_src_w - 1) * pixel_size; We already have helper functions to compute the linear offset properly for tiling, I think we should put the rotation adjustments in there to avoid dpulicated code of this. And a comment about how this works would be nice. > + } > + > + 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, > @@ -2494,11 +2507,14 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > unsigned long linear_offset; > u32 dspcntr; > u32 reg; > + int pixel_size; > > + pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > reg = DSPCNTR(plane); > dspcntr = I915_READ(reg); > /* Mask out pixel format bits in case we change it */ > dspcntr &= ~DISPPLANE_PIXFORMAT_MASK; > + dspcntr &= ~DISPPLANE_ROTATE_180; > switch (fb->pixel_format) { > case DRM_FORMAT_C8: > dspcntr |= DISPPLANE_8BPP; > @@ -2536,14 +2552,26 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > else > 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, > @@ -11576,11 +11604,65 @@ static void intel_plane_destroy(struct drm_plane *plane) > drm_plane_cleanup(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; > + > + intel_crtc = to_intel_crtc(plane->crtc); > + > + if (intel_crtc && intel_crtc->active && You need to check plane->crtc before upcasting. > + intel_crtc->primary_enabled) { > + intel_crtc_wait_for_pending_flips(crtc); > + > + /* FBC does not work on some platforms for rotated planes */ > + 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); > + /* If rotation was set earlier and new rotation is 0, > + we might have disabled fbc earlier. So update it now */ > + else if (intel_plane->rotation == BIT(DRM_ROTATE_0) > + && old_val != BIT(DRM_ROTATE_0)) { > + mutex_lock(&dev->struct_mutex); > + intel_update_fbc(dev); > + mutex_unlock(&dev->struct_mutex); > + } > + } Indentation is screwed up here. Also if we convert some of the checks into early bails we could de-indent this by one level. Also Chris mentioned that on some platforms this won't work and it's more future-proof to just do a full modeset until we have the proper infrastructure. > + > + dev_priv->display.update_primary_plane(crtc, > + crtc->primary->fb, 0, 0); > + > + } else { > + DRM_DEBUG_KMS("[CRTC:%d] inactive. Only rotation" > + "property is updated\n", crtc->base.id); > + } > + } > + return 0; > +} > > 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, > @@ -11598,6 +11680,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; > > @@ -11613,6 +11696,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)); The property should be created in drm_mode_create_standard_plane_properties. > + 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 9585f15..0210b1e 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -578,6 +578,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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 7/15/2014 2:41 PM, Daniel Vetter wrote: > On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal@intel.com wrote: >> From: Sonika Jindal <sonika.jindal@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. >> >> Testcase: igt/kms_rotation_crc >> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > Some stuff I've noticed below. > -Daniel > >> --- >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> drivers/gpu/drm/i915/intel_display.c | 104 ++++++++++++++++++++++++++++++++-- >> drivers/gpu/drm/i915/intel_pm.c | 6 ++ >> 3 files changed, 107 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 74283d5..f39e2e7 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -4107,6 +4107,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 023c770..e881dcf 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -2408,11 +2408,15 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, >> unsigned long linear_offset; >> u32 dspcntr; >> u32 reg; >> + int pixel_size; >> + >> + pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); >> >> reg = DSPCNTR(plane); >> dspcntr = I915_READ(reg); >> /* Mask out pixel format bits in case we change it */ >> dspcntr &= ~DISPPLANE_PIXFORMAT_MASK; >> + dspcntr &= ~DISPPLANE_ROTATE_180; >> switch (fb->pixel_format) { >> case DRM_FORMAT_C8: >> dspcntr |= DISPPLANE_8BPP; >> @@ -2454,8 +2458,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) { >> @@ -2467,6 +2469,17 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, >> } else { >> 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); >> + linear_offset += (intel_crtc->config.pipe_src_h - 1) * >> + fb->pitches[0] + >> + (intel_crtc->config.pipe_src_w - 1) * pixel_size; > > We already have helper functions to compute the linear offset properly for > tiling, I think we should put the rotation adjustments in there to avoid > dpulicated code of this. And a comment about how this works would be nice. > Ok, I will add the comments. >> + } >> + >> + 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, >> @@ -2494,11 +2507,14 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, >> unsigned long linear_offset; >> u32 dspcntr; >> u32 reg; >> + int pixel_size; >> >> + pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); >> reg = DSPCNTR(plane); >> dspcntr = I915_READ(reg); >> /* Mask out pixel format bits in case we change it */ >> dspcntr &= ~DISPPLANE_PIXFORMAT_MASK; >> + dspcntr &= ~DISPPLANE_ROTATE_180; >> switch (fb->pixel_format) { >> case DRM_FORMAT_C8: >> dspcntr |= DISPPLANE_8BPP; >> @@ -2536,14 +2552,26 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, >> else >> 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, >> @@ -11576,11 +11604,65 @@ static void intel_plane_destroy(struct drm_plane *plane) >> drm_plane_cleanup(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; >> + >> + intel_crtc = to_intel_crtc(plane->crtc); >> + >> + if (intel_crtc && intel_crtc->active && > > You need to check plane->crtc before upcasting. > Ok, I will add. >> + intel_crtc->primary_enabled) { >> + intel_crtc_wait_for_pending_flips(crtc); >> + >> + /* FBC does not work on some platforms for rotated planes */ >> + 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); >> + /* If rotation was set earlier and new rotation is 0, >> + we might have disabled fbc earlier. So update it now */ >> + else if (intel_plane->rotation == BIT(DRM_ROTATE_0) >> + && old_val != BIT(DRM_ROTATE_0)) { >> + mutex_lock(&dev->struct_mutex); >> + intel_update_fbc(dev); >> + mutex_unlock(&dev->struct_mutex); >> + } >> + } > > Indentation is screwed up here. Also if we convert some of the checks into > early bails we could de-indent this by one level. > Ok, I will add > Also Chris mentioned that on some platforms this won't work and it's more > future-proof to just do a full modeset until we have the proper > infrastructure. > Can you please elaborate on this? What needs to be done? >> + >> + dev_priv->display.update_primary_plane(crtc, >> + crtc->primary->fb, 0, 0); >> + >> + } else { >> + DRM_DEBUG_KMS("[CRTC:%d] inactive. Only rotation" >> + "property is updated\n", crtc->base.id); >> + } >> + } >> + return 0; >> +} >> >> 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, >> @@ -11598,6 +11680,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; >> >> @@ -11613,6 +11696,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)); > > The property should be created in > drm_mode_create_standard_plane_properties. > Ok, I will add. >> + 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 9585f15..0210b1e 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -578,6 +578,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@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Tue, Jul 15, 2014 at 03:08:37PM +0530, Jindal, Sonika wrote: > On 7/15/2014 2:41 PM, Daniel Vetter wrote: > >On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal@intel.com wrote: > >>From: Sonika Jindal <sonika.jindal@intel.com> > Ok, I will add > >Also Chris mentioned that on some platforms this won't work and it's more > >future-proof to just do a full modeset until we have the proper > >infrastructure. > > > Can you please elaborate on this? What needs to be done? Apparently nothing, it turned out that on the platforms currently supported everything is fine with your patch. Damien promised to follow up with the details on internal mailing lists. -Daniel
From: Sonika Jindal <sonika.jindal@intel.com>
Incorporating comments from Daniel regarding movement of creation of
rotation_property to drm_mode_create_standard_plane_properties and other minor
things.
Sonika Jindal (3):
drm: Add rotation_property to mode_config and creating it
drm/i915: Add 180 degree primary plane rotation support
Ville Syrjälä (1):
drm/i915: Add rotation property for sprites
drivers/gpu/drm/drm_crtc.c | 3 +-
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/intel_display.c | 104 ++++++++++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_pm.c | 6 ++
drivers/gpu/drm/i915/intel_sprite.c | 35 +++++++++++-
include/drm/drm_crtc.h | 1 +
6 files changed, 153 insertions(+), 7 deletions(-)
On Tue, Jul 15, 2014 at 05:00:20PM +0530, sonika.jindal@intel.com wrote: > From: Sonika Jindal <sonika.jindal@intel.com> > > Incorporating comments from Daniel regarding movement of creation of > rotation_property to drm_mode_create_standard_plane_properties and other minor > things. > > Sonika Jindal (3): > drm: Add rotation_property to mode_config and creating it > drm/i915: Add 180 degree primary plane rotation support > > Ville Syrjälä (1): > drm/i915: Add rotation property for sprites The summary says 0/3 but the patches are numbered x/4. That's a bit confusing. Two fixes: - Specify the git range for format-patch/send-email with the start..end syntax like for git log. - Send only individual patches in-reply-to the respective original patch. This is my preferred approach. Without that I'll look for 4/4 and won't find it ... Thanks, Daniel > > drivers/gpu/drm/drm_crtc.c | 3 +- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_display.c | 104 ++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_pm.c | 6 ++ > drivers/gpu/drm/i915/intel_sprite.c | 35 +++++++++++- > include/drm/drm_crtc.h | 1 + > 6 files changed, 153 insertions(+), 7 deletions(-) > > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 7/15/2014 5:32 PM, Daniel Vetter wrote: > On Tue, Jul 15, 2014 at 05:00:20PM +0530, sonika.jindal@intel.com wrote: >> From: Sonika Jindal <sonika.jindal@intel.com> >> >> Incorporating comments from Daniel regarding movement of creation of >> rotation_property to drm_mode_create_standard_plane_properties and other minor >> things. >> >> Sonika Jindal (3): >> drm: Add rotation_property to mode_config and creating it >> drm/i915: Add 180 degree primary plane rotation support >> >> Ville Syrjälä (1): >> drm/i915: Add rotation property for sprites > > The summary says 0/3 but the patches are numbered x/4. That's a bit > confusing. Two fixes: > - Specify the git range for format-patch/send-email with the start..end > syntax like for git log. > - Send only individual patches in-reply-to the respective original patch. > This is my preferred approach. > Ok, I will send each patch in reply to the original patches. I thought replying the affected patches to the comment's mail should make it clearer. I'l will send the patches separately. -Sonika > Without that I'll look for 4/4 and won't find it ... > > Thanks, Daniel > >> >> drivers/gpu/drm/drm_crtc.c | 3 +- >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> drivers/gpu/drm/i915/intel_display.c | 104 ++++++++++++++++++++++++++++++++-- >> drivers/gpu/drm/i915/intel_pm.c | 6 ++ >> drivers/gpu/drm/i915/intel_sprite.c | 35 +++++++++++- >> include/drm/drm_crtc.h | 1 + >> 6 files changed, 153 insertions(+), 7 deletions(-) >> >> -- >> 1.7.10.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Tue, Jul 15, 2014 at 11:11:37AM +0200, Daniel Vetter wrote: > On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal@intel.com wrote: > > + /* FBC does not work on some platforms for rotated planes */ > > + 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); > > + /* If rotation was set earlier and new rotation is 0, > > + we might have disabled fbc earlier. So update it now */ > > + else if (intel_plane->rotation == BIT(DRM_ROTATE_0) > > + && old_val != BIT(DRM_ROTATE_0)) { > > + mutex_lock(&dev->struct_mutex); > > + intel_update_fbc(dev); > > + mutex_unlock(&dev->struct_mutex); > > + } > > + } > > Indentation is screwed up here. Also if we convert some of the checks into > early bails we could de-indent this by one level. > > Also Chris mentioned that on some platforms this won't work and it's more > future-proof to just do a full modeset until we have the proper > infrastructure. Apparently this review here was never addressed, as Chris just pointed out on irc. I've dropped the patch again. I think we need: - The same sequence as with the sprite set_property function, i.e. we need to call the update_plane function (not the raw low-level one, the high-level with all the checks). - The fbc check is wrong and will miss updates when the crtc is off. We need to move this into the general list of checks in intel_update_fbc. - Since this seems to be buggy I want added testcases to combine fbc correctness with screen rotation. Probably best to reuse the existing fbc testcase and add a bunch or rotated tests. Thanks, Daniel
On Thu, Aug 07, 2014 at 01:45:31PM +0200, Daniel Vetter wrote: > On Tue, Jul 15, 2014 at 11:11:37AM +0200, Daniel Vetter wrote: > > On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal@intel.com wrote: > > > + /* FBC does not work on some platforms for rotated planes */ > > > + 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); > > > + /* If rotation was set earlier and new rotation is 0, > > > + we might have disabled fbc earlier. So update it now */ > > > + else if (intel_plane->rotation == BIT(DRM_ROTATE_0) > > > + && old_val != BIT(DRM_ROTATE_0)) { > > > + mutex_lock(&dev->struct_mutex); > > > + intel_update_fbc(dev); > > > + mutex_unlock(&dev->struct_mutex); > > > + } > > > + } > > > > Indentation is screwed up here. Also if we convert some of the checks into > > early bails we could de-indent this by one level. > > > > Also Chris mentioned that on some platforms this won't work and it's more > > future-proof to just do a full modeset until we have the proper > > infrastructure. > > Apparently this review here was never addressed, as Chris just pointed out > on irc. I've dropped the patch again. > > I think we need: > - The same sequence as with the sprite set_property function, i.e. we need > to call the update_plane function (not the raw low-level one, the > high-level with all the checks). > - The fbc check is wrong and will miss updates when the crtc is off. We > need to move this into the general list of checks in intel_update_fbc. > - Since this seems to be buggy I want added testcases to combine fbc > correctness with screen rotation. Probably best to reuse the existing > fbc testcase and add a bunch or rotated tests. Ok, the check in update_fbc is there, I've been blind. Sorry about all the confusion. So just amounts of calling the higher level function and we can forgo the fbc testing. -Daniel
Hi Daniel, Are you taking this patch back in drm-intel? -Sonika On 8/7/2014 5:41 PM, Daniel Vetter wrote: > On Thu, Aug 07, 2014 at 01:45:31PM +0200, Daniel Vetter wrote: >> On Tue, Jul 15, 2014 at 11:11:37AM +0200, Daniel Vetter wrote: >>> On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal@intel.com wrote: >>>> + /* FBC does not work on some platforms for rotated planes */ >>>> + 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); >>>> + /* If rotation was set earlier and new rotation is 0, >>>> + we might have disabled fbc earlier. So update it now */ >>>> + else if (intel_plane->rotation == BIT(DRM_ROTATE_0) >>>> + && old_val != BIT(DRM_ROTATE_0)) { >>>> + mutex_lock(&dev->struct_mutex); >>>> + intel_update_fbc(dev); >>>> + mutex_unlock(&dev->struct_mutex); >>>> + } >>>> + } >>> >>> Indentation is screwed up here. Also if we convert some of the checks into >>> early bails we could de-indent this by one level. >>> >>> Also Chris mentioned that on some platforms this won't work and it's more >>> future-proof to just do a full modeset until we have the proper >>> infrastructure. >> >> Apparently this review here was never addressed, as Chris just pointed out >> on irc. I've dropped the patch again. >> >> I think we need: >> - The same sequence as with the sprite set_property function, i.e. we need >> to call the update_plane function (not the raw low-level one, the >> high-level with all the checks). >> - The fbc check is wrong and will miss updates when the crtc is off. We >> need to move this into the general list of checks in intel_update_fbc. >> - Since this seems to be buggy I want added testcases to combine fbc >> correctness with screen rotation. Probably best to reuse the existing >> fbc testcase and add a bunch or rotated tests. > > Ok, the check in update_fbc is there, I've been blind. Sorry about all the > confusion. So just amounts of calling the higher level function and we can > forgo the fbc testing. > -Daniel >
On Mon, Aug 11, 2014 at 04:37:00PM +0530, Jindal, Sonika wrote: > > Hi Daniel, > Are you taking this patch back in drm-intel? We should still call the primary_plane->update hook directly instead of open-coding it. -Daniel > > -Sonika > > On 8/7/2014 5:41 PM, Daniel Vetter wrote: > >On Thu, Aug 07, 2014 at 01:45:31PM +0200, Daniel Vetter wrote: > >>On Tue, Jul 15, 2014 at 11:11:37AM +0200, Daniel Vetter wrote: > >>>On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal@intel.com wrote: > >>>>+ /* FBC does not work on some platforms for rotated planes */ > >>>>+ 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); > >>>>+ /* If rotation was set earlier and new rotation is 0, > >>>>+ we might have disabled fbc earlier. So update it now */ > >>>>+ else if (intel_plane->rotation == BIT(DRM_ROTATE_0) > >>>>+ && old_val != BIT(DRM_ROTATE_0)) { > >>>>+ mutex_lock(&dev->struct_mutex); > >>>>+ intel_update_fbc(dev); > >>>>+ mutex_unlock(&dev->struct_mutex); > >>>>+ } > >>>>+ } > >>> > >>>Indentation is screwed up here. Also if we convert some of the checks into > >>>early bails we could de-indent this by one level. > >>> > >>>Also Chris mentioned that on some platforms this won't work and it's more > >>>future-proof to just do a full modeset until we have the proper > >>>infrastructure. > >> > >>Apparently this review here was never addressed, as Chris just pointed out > >>on irc. I've dropped the patch again. > >> > >>I think we need: > >>- The same sequence as with the sprite set_property function, i.e. we need > >> to call the update_plane function (not the raw low-level one, the > >> high-level with all the checks). > >>- The fbc check is wrong and will miss updates when the crtc is off. We > >> need to move this into the general list of checks in intel_update_fbc. > >>- Since this seems to be buggy I want added testcases to combine fbc > >> correctness with screen rotation. Probably best to reuse the existing > >> fbc testcase and add a bunch or rotated tests. > > > >Ok, the check in update_fbc is there, I've been blind. Sorry about all the > >confusion. So just amounts of calling the higher level function and we can > >forgo the fbc testing. > >-Daniel > >
On 8/11/2014 5:12 PM, Daniel Vetter wrote: > On Mon, Aug 11, 2014 at 04:37:00PM +0530, Jindal, Sonika wrote: >> >> Hi Daniel, >> Are you taking this patch back in drm-intel? > > We should still call the primary_plane->update hook directly instead of > open-coding it. > -Daniel > But we are already doing dev_priv->display.update_primary_plane. Can you please elaborate? Last time we had a discussion on this same point, and we thought for some platforms more work might be needed but for current platforms this looks ok. Following was the discussion: "> >Also Chris mentioned that on some platforms this won't work and it's > >more future-proof to just do a full modeset until we have the proper > >infrastructure. > > > Can you please elaborate on this? What needs to be done? Apparently nothing, it turned out that on the platforms currently supported everything is fine with your patch. Damien promised to follow up with the details on internal mailing lists. -Daniel " >> >> -Sonika >> >> On 8/7/2014 5:41 PM, Daniel Vetter wrote: >>> On Thu, Aug 07, 2014 at 01:45:31PM +0200, Daniel Vetter wrote: >>>> On Tue, Jul 15, 2014 at 11:11:37AM +0200, Daniel Vetter wrote: >>>>> On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal@intel.com wrote: >>>>>> + /* FBC does not work on some platforms for rotated planes */ >>>>>> + 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); >>>>>> + /* If rotation was set earlier and new rotation is 0, >>>>>> + we might have disabled fbc earlier. So update it now */ >>>>>> + else if (intel_plane->rotation == BIT(DRM_ROTATE_0) >>>>>> + && old_val != BIT(DRM_ROTATE_0)) { >>>>>> + mutex_lock(&dev->struct_mutex); >>>>>> + intel_update_fbc(dev); >>>>>> + mutex_unlock(&dev->struct_mutex); >>>>>> + } >>>>>> + } >>>>> >>>>> Indentation is screwed up here. Also if we convert some of the checks into >>>>> early bails we could de-indent this by one level. >>>>> >>>>> Also Chris mentioned that on some platforms this won't work and it's more >>>>> future-proof to just do a full modeset until we have the proper >>>>> infrastructure. >>>> >>>> Apparently this review here was never addressed, as Chris just pointed out >>>> on irc. I've dropped the patch again. >>>> >>>> I think we need: >>>> - The same sequence as with the sprite set_property function, i.e. we need >>>> to call the update_plane function (not the raw low-level one, the >>>> high-level with all the checks). >>>> - The fbc check is wrong and will miss updates when the crtc is off. We >>>> need to move this into the general list of checks in intel_update_fbc. >>>> - Since this seems to be buggy I want added testcases to combine fbc >>>> correctness with screen rotation. Probably best to reuse the existing >>>> fbc testcase and add a bunch or rotated tests. >>> >>> Ok, the check in update_fbc is there, I've been blind. Sorry about all the >>> confusion. So just amounts of calling the higher level function and we can >>> forgo the fbc testing. >>> -Daniel >>> >
On Mon, Aug 11, 2014 at 05:37:20PM +0530, Jindal, Sonika wrote: > > > On 8/11/2014 5:12 PM, Daniel Vetter wrote: > >On Mon, Aug 11, 2014 at 04:37:00PM +0530, Jindal, Sonika wrote: > >> > >>Hi Daniel, > >>Are you taking this patch back in drm-intel? > > > >We should still call the primary_plane->update hook directly instead of > >open-coding it. > >-Daniel > > > But we are already doing dev_priv->display.update_primary_plane. Can you > please elaborate? Last time we had a discussion on this same point, and we > thought for some platforms more work might be needed but for current > platforms this looks ok. Following was the discussion: That's a different cook used internally in the driver (mostly for historical reasons nowadays). I'm talking about the drm_plane->fops->update_plane hook, which is implemented for primary planes by intel_primary_plane_setplane. The idea is to have the exact same code flow as for sprite planes, since once we have atomic modesets that will be what we need. In the sprite set_prop function you pretty directly call the update_plane hook, and I think we should do the same here. Actually we might as well directly reuse the intel_plane_restore function, it seems to exactly do what we want. > "> >Also Chris mentioned that on some platforms this won't work and it's > > >more future-proof to just do a full modeset until we have the proper > > >infrastructure. > > > > > Can you please elaborate on this? What needs to be done? > > Apparently nothing, it turned out that on the platforms currently supported > everything is fine with your patch. Damien promised to follow up with the > details on internal mailing lists. Damien follow up internally. Since we're now allowed to talk about skl I'll paste this here: "On SKL, for 90/270, we'll also need to update the watermarks and remap the fb. "We already duplicate some of the logic: the FBC update, wait for pending flips, ... "So maybe we should try to find a way to take the same path for all updates. I'm not too sure how practical this is though." Damien also said that he doesn't want to still the series on this, but since we now have universal plane support for the primary plane I think it makes a lot of sense. -Daniel > -Daniel > " > > >> > >>-Sonika > >> > >>On 8/7/2014 5:41 PM, Daniel Vetter wrote: > >>>On Thu, Aug 07, 2014 at 01:45:31PM +0200, Daniel Vetter wrote: > >>>>On Tue, Jul 15, 2014 at 11:11:37AM +0200, Daniel Vetter wrote: > >>>>>On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal@intel.com wrote: > >>>>>>+ /* FBC does not work on some platforms for rotated planes */ > >>>>>>+ 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); > >>>>>>+ /* If rotation was set earlier and new rotation is 0, > >>>>>>+ we might have disabled fbc earlier. So update it now */ > >>>>>>+ else if (intel_plane->rotation == BIT(DRM_ROTATE_0) > >>>>>>+ && old_val != BIT(DRM_ROTATE_0)) { > >>>>>>+ mutex_lock(&dev->struct_mutex); > >>>>>>+ intel_update_fbc(dev); > >>>>>>+ mutex_unlock(&dev->struct_mutex); > >>>>>>+ } > >>>>>>+ } > >>>>> > >>>>>Indentation is screwed up here. Also if we convert some of the checks into > >>>>>early bails we could de-indent this by one level. > >>>>> > >>>>>Also Chris mentioned that on some platforms this won't work and it's more > >>>>>future-proof to just do a full modeset until we have the proper > >>>>>infrastructure. > >>>> > >>>>Apparently this review here was never addressed, as Chris just pointed out > >>>>on irc. I've dropped the patch again. > >>>> > >>>>I think we need: > >>>>- The same sequence as with the sprite set_property function, i.e. we need > >>>> to call the update_plane function (not the raw low-level one, the > >>>> high-level with all the checks). > >>>>- The fbc check is wrong and will miss updates when the crtc is off. We > >>>> need to move this into the general list of checks in intel_update_fbc. > >>>>- Since this seems to be buggy I want added testcases to combine fbc > >>>> correctness with screen rotation. Probably best to reuse the existing > >>>> fbc testcase and add a bunch or rotated tests. > >>> > >>>Ok, the check in update_fbc is there, I've been blind. Sorry about all the > >>>confusion. So just amounts of calling the higher level function and we can > >>>forgo the fbc testing. > >>>-Daniel > >>> > >
On 8/11/2014 5:53 PM, Daniel Vetter wrote: > On Mon, Aug 11, 2014 at 05:37:20PM +0530, Jindal, Sonika wrote: >> >> >> On 8/11/2014 5:12 PM, Daniel Vetter wrote: >>> On Mon, Aug 11, 2014 at 04:37:00PM +0530, Jindal, Sonika wrote: >>>> >>>> Hi Daniel, >>>> Are you taking this patch back in drm-intel? >>> >>> We should still call the primary_plane->update hook directly instead of >>> open-coding it. >>> -Daniel >>> >> But we are already doing dev_priv->display.update_primary_plane. Can you >> please elaborate? Last time we had a discussion on this same point, and we >> thought for some platforms more work might be needed but for current >> platforms this looks ok. Following was the discussion: > > That's a different cook used internally in the driver (mostly for > historical reasons nowadays). I'm talking about the > drm_plane->fops->update_plane hook, which is implemented for primary > planes by intel_primary_plane_setplane. > Ok, let me add that and post a new patch for this. -Sonika > The idea is to have the exact same code flow as for sprite planes, since > once we have atomic modesets that will be what we need. In the sprite > set_prop function you pretty directly call the update_plane hook, and I > think we should do the same here. Actually we might as well directly reuse > the intel_plane_restore function, it seems to exactly do what we want. > >> "> >Also Chris mentioned that on some platforms this won't work and it's >>>> more future-proof to just do a full modeset until we have the proper >>>> infrastructure. >>>> >>> Can you please elaborate on this? What needs to be done? >> >> Apparently nothing, it turned out that on the platforms currently supported >> everything is fine with your patch. Damien promised to follow up with the >> details on internal mailing lists. > > Damien follow up internally. Since we're now allowed to talk about skl > I'll paste this here: > > "On SKL, for 90/270, we'll also need to update the watermarks and remap > the fb. > > "We already duplicate some of the logic: the FBC update, wait for > pending flips, ... > > "So maybe we should try to find a way to take the same path for all > updates. I'm not too sure how practical this is though." > > Damien also said that he doesn't want to still the series on this, but > since we now have universal plane support for the primary plane I think it > makes a lot of sense. > -Daniel > > >> -Daniel >> " >> >>>> >>>> -Sonika >>>> >>>> On 8/7/2014 5:41 PM, Daniel Vetter wrote: >>>>> On Thu, Aug 07, 2014 at 01:45:31PM +0200, Daniel Vetter wrote: >>>>>> On Tue, Jul 15, 2014 at 11:11:37AM +0200, Daniel Vetter wrote: >>>>>>> On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal@intel.com wrote: >>>>>>>> + /* FBC does not work on some platforms for rotated planes */ >>>>>>>> + 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); >>>>>>>> + /* If rotation was set earlier and new rotation is 0, >>>>>>>> + we might have disabled fbc earlier. So update it now */ >>>>>>>> + else if (intel_plane->rotation == BIT(DRM_ROTATE_0) >>>>>>>> + && old_val != BIT(DRM_ROTATE_0)) { >>>>>>>> + mutex_lock(&dev->struct_mutex); >>>>>>>> + intel_update_fbc(dev); >>>>>>>> + mutex_unlock(&dev->struct_mutex); >>>>>>>> + } >>>>>>>> + } >>>>>>> >>>>>>> Indentation is screwed up here. Also if we convert some of the checks into >>>>>>> early bails we could de-indent this by one level. >>>>>>> >>>>>>> Also Chris mentioned that on some platforms this won't work and it's more >>>>>>> future-proof to just do a full modeset until we have the proper >>>>>>> infrastructure. >>>>>> >>>>>> Apparently this review here was never addressed, as Chris just pointed out >>>>>> on irc. I've dropped the patch again. >>>>>> >>>>>> I think we need: >>>>>> - The same sequence as with the sprite set_property function, i.e. we need >>>>>> to call the update_plane function (not the raw low-level one, the >>>>>> high-level with all the checks). >>>>>> - The fbc check is wrong and will miss updates when the crtc is off. We >>>>>> need to move this into the general list of checks in intel_update_fbc. >>>>>> - Since this seems to be buggy I want added testcases to combine fbc >>>>>> correctness with screen rotation. Probably best to reuse the existing >>>>>> fbc testcase and add a bunch or rotated tests. >>>>> >>>>> Ok, the check in update_fbc is there, I've been blind. Sorry about all the >>>>> confusion. So just amounts of calling the higher level function and we can >>>>> forgo the fbc testing. >>>>> -Daniel >>>>> >>> >
From: Sonika Jindal <sonika.jindal@intel.com>
As suggested by Daniel, now calling intel_primary_plane_setplane in
set_property.
Inside setplane, the intel_plane's member variables were not updated in the
structure. The first patch does that updation.
From kms_rotation_crc igt, drmModeSetCrtc was called to set up crtc for primary
plane. So, intel_plane's members like x, y, w, h will not get updated and when
setplane is called from set_property, the values will be invalid in intel_plane
structure. So, updated the kms_plane_crc igt also to call igt_display_commit2
with COMMIT_UNIVERSAL.
For legacy planes, I am still not sure how will this setplane approach work.
Open for suggestions..
Dropping the r-b tag from the second patch.
Sonika Jindal (2):
drm/i915: Updating intel_plane members for primary plane in setplane
drm/i915: Add 180 degree primary plane rotation support
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/intel_display.c | 140 +++++++++++++++++++++++++++++++++-
drivers/gpu/drm/i915/intel_pm.c | 6 ++
3 files changed, 143 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 74283d5..f39e2e7 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4107,6 +4107,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 023c770..e881dcf 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2408,11 +2408,15 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, unsigned long linear_offset; u32 dspcntr; u32 reg; + int pixel_size; + + pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); reg = DSPCNTR(plane); dspcntr = I915_READ(reg); /* Mask out pixel format bits in case we change it */ dspcntr &= ~DISPPLANE_PIXFORMAT_MASK; + dspcntr &= ~DISPPLANE_ROTATE_180; switch (fb->pixel_format) { case DRM_FORMAT_C8: dspcntr |= DISPPLANE_8BPP; @@ -2454,8 +2458,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) { @@ -2467,6 +2469,17 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, } else { 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); + 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, @@ -2494,11 +2507,14 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, unsigned long linear_offset; u32 dspcntr; u32 reg; + int pixel_size; + pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); reg = DSPCNTR(plane); dspcntr = I915_READ(reg); /* Mask out pixel format bits in case we change it */ dspcntr &= ~DISPPLANE_PIXFORMAT_MASK; + dspcntr &= ~DISPPLANE_ROTATE_180; switch (fb->pixel_format) { case DRM_FORMAT_C8: dspcntr |= DISPPLANE_8BPP; @@ -2536,14 +2552,26 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, else 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, @@ -11576,11 +11604,65 @@ static void intel_plane_destroy(struct drm_plane *plane) drm_plane_cleanup(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; + + 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 */ + 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); + /* If rotation was set earlier and new rotation is 0, + we might have disabled fbc earlier. So update it now */ + else if (intel_plane->rotation == BIT(DRM_ROTATE_0) + && old_val != BIT(DRM_ROTATE_0)) { + mutex_lock(&dev->struct_mutex); + intel_update_fbc(dev); + mutex_unlock(&dev->struct_mutex); + } + } + + dev_priv->display.update_primary_plane(crtc, + crtc->primary->fb, 0, 0); + + } else { + DRM_DEBUG_KMS("[CRTC:%d] inactive. Only rotation" + "property is updated\n", crtc->base.id); + } + } + return 0; +} 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, @@ -11598,6 +11680,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; @@ -11613,6 +11696,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 9585f15..0210b1e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -578,6 +578,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())