| Message ID | 1403081847-4364-11-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 18, 2014 at 02:27:26PM +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. > > Testcase: igt/kms_rotation_crc > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch> > Cc: Jani Nikula <jani.nikula at linux.intel.com> > Cc: David Airlie <airlied at linux.ie> > Cc: dri-devel at lists.freedesktop.org > Cc: linux-kernel at vger.kernel.org > Cc: vijay.a.purushothaman at intel.com > Signed-off-by: Uma Shankar <uma.shankar at intel.com> > Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com> Some checkpatch.pl warnings: ERROR: code indent should use tabs where possible #145: FILE: drivers/gpu/drm/i915/intel_display.c:2488: + (intel_crtc->config.pipe_src_w - 1) * pixel_size;$ WARNING: please, no spaces at the start of a line #145: FILE: drivers/gpu/drm/i915/intel_display.c:2488: + (intel_crtc->config.pipe_src_w - 1) * pixel_size;$ > --- > drivers/gpu/drm/i915/i915_dma.c | 17 +++++++ > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_display.c | 91 ++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_pm.c | 8 +++ > 4 files changed, 113 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 5e583a1..4c91fbc 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1939,6 +1939,8 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file) > void i915_driver_lastclose(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_crtc *crtc; > + struct intel_plane *plane; > > /* On gen6+ we refuse to init without kms enabled, but then the drm core > * goes right around and calls lastclose. Check for this and don't clean > @@ -1946,6 +1948,21 @@ void i915_driver_lastclose(struct drm_device *dev) > if (!dev_priv) > return; > > + if (dev_priv->rotation_property) { > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) { > + to_intel_plane(crtc->base.primary)->rotation = BIT(DRM_ROTATE_0); > + drm_object_property_set_value(&crtc->base.base, > + dev_priv->rotation_property, > + to_intel_plane(crtc->base.primary)->rotation); > + } > + list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) { > + plane->rotation = BIT(DRM_ROTATE_0); > + drm_object_property_set_value(&plane->base.base, > + dev_priv->rotation_property, > + plane->rotation); > + } > + } > + The purpose of this seems to be restoring rotation to 0 and rely on the next modeset to re-program the register. This code, is orthogonal to the pure primary plane rotation enabling, spans through both sprite and primary planes and may actually be a bit tricky to get right. -> This shouldn't be part of the same commit as the primary plane rotation. Please span a new commit for it. Now, we also need something like this when switching VT, and probably for kernel panic and debug handling as well, so lastclose() is not enough (and we can probably do better). One idea would be for the core DRM code to know about this property, and make sure we put the rotation back to 0 in restore_fbdev_mode(), we do something similar for the for the sprite planes in there already. Another idea would be to add a vfunc to execute driver specific code there in restore_fbdev_mode(). There is probably a better way to do it, I have to say I'm not super familiar with this part of the driver. > if (drm_core_check_feature(dev, DRIVER_MODESET)) { > intel_fbdev_restore_mode(dev); > vga_switcheroo_process_delayed_switch(); > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index c70c804..c600d3b 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4087,6 +4087,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 5e8e711..1dc8b68 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2414,7 +2414,9 @@ 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); > intel_fb = to_intel_framebuffer(fb); > obj = intel_fb->obj; > > @@ -2422,6 +2424,8 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, > 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; > @@ -2463,8 +2467,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) { > @@ -2477,6 +2479,17 @@ 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); > + 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]); > @@ -2504,7 +2517,9 @@ 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); > intel_fb = to_intel_framebuffer(fb); > obj = intel_fb->obj; > > @@ -2512,6 +2527,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > 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; > @@ -2549,8 +2566,6 @@ 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, > @@ -2558,6 +2573,19 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > 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, > fb->pitches[0]); > @@ -11324,10 +11352,51 @@ 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 intel_crtc *intel_crtc = to_intel_crtc(plane->crtc); > + struct drm_crtc *crtc = &intel_crtc->base; > + uint64_t old_val; > + int ret = -ENOENT; > + > + if (prop == dev_priv->rotation_property) { > + /* exactly one rotation angle please */ > + if (hweight32(val & 0xf) != 1) > + return -EINVAL; > + > + old_val = intel_plane->rotation; This value is set but never used again? > + intel_plane->rotation = val; > + > + if (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 (dev_priv->fbc.plane == intel_crtc->plane && > + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && > + intel_plane->rotation != BIT(DRM_ROTATE_0)) > + intel_disable_fbc(dev); > + > + dev_priv->display.update_primary_plane(crtc, crtc->primary->fb, 0, 0); > + } else { > + DRM_DEBUG_KMS("[CRTC:%d] is not active. Only rotation property is updated\n", > + crtc->base.id); > + ret = 0; > + } > + } > + > + return ret; > +} > + > 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, > @@ -11335,6 +11404,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > { > struct intel_plane *primary; > const uint32_t *intel_primary_formats; > + struct drm_i915_private *dev_priv = dev->dev_private; > int num_formats; > > primary = kzalloc(sizeof(*primary), GFP_KERNEL); > @@ -11345,6 +11415,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; > > @@ -11360,6 +11431,18 @@ 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_priv->rotation_property) > + dev_priv->rotation_property = > + drm_mode_create_rotation_property(dev, > + BIT(DRM_ROTATE_0) | > + BIT(DRM_ROTATE_180)); > + if (dev_priv->rotation_property) > + drm_object_attach_property(&primary->base.base, > + dev_priv->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 2043c4b..cabccfb 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -562,6 +562,14 @@ void intel_update_fbc(struct drm_device *dev) > 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("mode incompatible with compression, " > + "disabling\n"); This debug message isn't helpful. You need to add that's because of the rotation. > + goto out_disable; > + } > + > /* If the kernel debugger is active, always disable compression */ > if (in_dbg_master()) > goto out_disable; > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 6/18/2014 10:32 PM, Damien Lespiau wrote: > On Wed, Jun 18, 2014 at 02:27:26PM +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. >> >> Testcase: igt/kms_rotation_crc >> >> Cc: Daniel Vetter <daniel.vetter at ffwll.ch> >> Cc: Jani Nikula <jani.nikula at linux.intel.com> >> Cc: David Airlie <airlied at linux.ie> >> Cc: dri-devel at lists.freedesktop.org >> Cc: linux-kernel at vger.kernel.org >> Cc: vijay.a.purushothaman at intel.com >> Signed-off-by: Uma Shankar <uma.shankar at intel.com> >> Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com> > > Some checkpatch.pl warnings: > > ERROR: code indent should use tabs where possible > #145: FILE: drivers/gpu/drm/i915/intel_display.c:2488: > + (intel_crtc->config.pipe_src_w - 1) * pixel_size;$ > > WARNING: please, no spaces at the start of a line > #145: FILE: drivers/gpu/drm/i915/intel_display.c:2488: > + (intel_crtc->config.pipe_src_w - 1) * pixel_size;$ > > >> --- >> drivers/gpu/drm/i915/i915_dma.c | 17 +++++++ >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> drivers/gpu/drm/i915/intel_display.c | 91 ++++++++++++++++++++++++++++++++-- >> drivers/gpu/drm/i915/intel_pm.c | 8 +++ >> 4 files changed, 113 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c >> index 5e583a1..4c91fbc 100644 >> --- a/drivers/gpu/drm/i915/i915_dma.c >> +++ b/drivers/gpu/drm/i915/i915_dma.c >> @@ -1939,6 +1939,8 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file) >> void i915_driver_lastclose(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_crtc *crtc; >> + struct intel_plane *plane; >> >> /* On gen6+ we refuse to init without kms enabled, but then the drm core >> * goes right around and calls lastclose. Check for this and don't clean >> @@ -1946,6 +1948,21 @@ void i915_driver_lastclose(struct drm_device *dev) >> if (!dev_priv) >> return; >> >> + if (dev_priv->rotation_property) { >> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) { >> + to_intel_plane(crtc->base.primary)->rotation = BIT(DRM_ROTATE_0); >> + drm_object_property_set_value(&crtc->base.base, >> + dev_priv->rotation_property, >> + to_intel_plane(crtc->base.primary)->rotation); >> + } >> + list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) { >> + plane->rotation = BIT(DRM_ROTATE_0); >> + drm_object_property_set_value(&plane->base.base, >> + dev_priv->rotation_property, >> + plane->rotation); >> + } >> + } >> + > > The purpose of this seems to be restoring rotation to 0 and rely on the next > modeset to re-program the register. This code, is orthogonal to the pure > primary plane rotation enabling, spans through both sprite and primary planes > and may actually be a bit tricky to get right. > > -> This shouldn't be part of the same commit as the primary plane rotation. > Please span a new commit for it. Sure I will add another patch. > > Now, we also need something like this when switching VT, and probably for > kernel panic and debug handling as well, so lastclose() is not enough (and we > can probably do better). > > One idea would be for the core DRM code to know about this property, and make > sure we put the rotation back to 0 in restore_fbdev_mode(), we do something > similar for the for the sprite planes in there already. Another idea would be > to add a vfunc to execute driver specific code there in restore_fbdev_mode(). > > There is probably a better way to do it, I have to say I'm not super familiar > with this part of the driver. > I see that in omap driver too it is done in lastclose of the driver. Also, from driver fbdev_restore is only called during lastclose. Again I don't have more knowledge on this. Can we keep it here in this lastclose function to comply with omap driver? > >> if (drm_core_check_feature(dev, DRIVER_MODESET)) { >> intel_fbdev_restore_mode(dev); >> vga_switcheroo_process_delayed_switch(); >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index c70c804..c600d3b 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -4087,6 +4087,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 5e8e711..1dc8b68 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -2414,7 +2414,9 @@ 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); >> intel_fb = to_intel_framebuffer(fb); >> obj = intel_fb->obj; >> >> @@ -2422,6 +2424,8 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, >> 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; >> @@ -2463,8 +2467,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) { >> @@ -2477,6 +2479,17 @@ 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); >> + 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]); >> @@ -2504,7 +2517,9 @@ 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); >> intel_fb = to_intel_framebuffer(fb); >> obj = intel_fb->obj; >> >> @@ -2512,6 +2527,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, >> 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; >> @@ -2549,8 +2566,6 @@ 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, >> @@ -2558,6 +2573,19 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, >> 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, >> fb->pitches[0]); >> @@ -11324,10 +11352,51 @@ 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 intel_crtc *intel_crtc = to_intel_crtc(plane->crtc); >> + struct drm_crtc *crtc = &intel_crtc->base; >> + uint64_t old_val; >> + int ret = -ENOENT; >> + >> + if (prop == dev_priv->rotation_property) { >> + /* exactly one rotation angle please */ >> + if (hweight32(val & 0xf) != 1) >> + return -EINVAL; >> + >> + old_val = intel_plane->rotation; > > This value is set but never used again? This was a miss from the last version of the patch. Will remove it. > >> + intel_plane->rotation = val; >> + >> + if (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 (dev_priv->fbc.plane == intel_crtc->plane && >> + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && >> + intel_plane->rotation != BIT(DRM_ROTATE_0)) >> + intel_disable_fbc(dev); >> + >> + dev_priv->display.update_primary_plane(crtc, crtc->primary->fb, 0, 0); >> + } else { >> + DRM_DEBUG_KMS("[CRTC:%d] is not active. Only rotation property is updated\n", >> + crtc->base.id); >> + ret = 0; >> + } >> + } >> + >> + return ret; >> +} >> + >> 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, >> @@ -11335,6 +11404,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, >> { >> struct intel_plane *primary; >> const uint32_t *intel_primary_formats; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> int num_formats; >> >> primary = kzalloc(sizeof(*primary), GFP_KERNEL); >> @@ -11345,6 +11415,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; >> >> @@ -11360,6 +11431,18 @@ 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_priv->rotation_property) >> + dev_priv->rotation_property = >> + drm_mode_create_rotation_property(dev, >> + BIT(DRM_ROTATE_0) | >> + BIT(DRM_ROTATE_180)); >> + if (dev_priv->rotation_property) >> + drm_object_attach_property(&primary->base.base, >> + dev_priv->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 2043c4b..cabccfb 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -562,6 +562,14 @@ void intel_update_fbc(struct drm_device *dev) >> 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("mode incompatible with compression, " >> + "disabling\n"); > > This debug message isn't helpful. You need to add that's because of the > rotation. Sure, will add. > >> + goto out_disable; >> + } >> + >> /* If the kernel debugger is active, always disable compression */ >> if (in_dbg_master()) >> goto out_disable; >> -- >> 1.7.10.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jun 19, 2014 at 12:13:54PM +0530, Jindal, Sonika wrote: > On 6/18/2014 10:32 PM, Damien Lespiau wrote: > >On Wed, Jun 18, 2014 at 02:27:26PM +0530, sonika.jindal@intel.com wrote: > >>From: Sonika Jindal <sonika.jindal@intel.com> > >>diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > >>index 5e583a1..4c91fbc 100644 > >>--- a/drivers/gpu/drm/i915/i915_dma.c > >>+++ b/drivers/gpu/drm/i915/i915_dma.c > >>@@ -1939,6 +1939,8 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file) > >> void i915_driver_lastclose(struct drm_device *dev) > >> { > >> struct drm_i915_private *dev_priv = dev->dev_private; > >>+ struct intel_crtc *crtc; > >>+ struct intel_plane *plane; > >> > >> /* On gen6+ we refuse to init without kms enabled, but then the drm core > >> * goes right around and calls lastclose. Check for this and don't clean > >>@@ -1946,6 +1948,21 @@ void i915_driver_lastclose(struct drm_device *dev) > >> if (!dev_priv) > >> return; > >> > >>+ if (dev_priv->rotation_property) { > >>+ list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) { > >>+ to_intel_plane(crtc->base.primary)->rotation = BIT(DRM_ROTATE_0); > >>+ drm_object_property_set_value(&crtc->base.base, > >>+ dev_priv->rotation_property, > >>+ to_intel_plane(crtc->base.primary)->rotation); > >>+ } > >>+ list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) { > >>+ plane->rotation = BIT(DRM_ROTATE_0); > >>+ drm_object_property_set_value(&plane->base.base, > >>+ dev_priv->rotation_property, > >>+ plane->rotation); > >>+ } > >>+ } > >>+ > > > >The purpose of this seems to be restoring rotation to 0 and rely on the next > >modeset to re-program the register. This code, is orthogonal to the pure > >primary plane rotation enabling, spans through both sprite and primary planes > >and may actually be a bit tricky to get right. > > > >-> This shouldn't be part of the same commit as the primary plane rotation. > >Please span a new commit for it. > Sure I will add another patch. > > > >Now, we also need something like this when switching VT, and probably for > >kernel panic and debug handling as well, so lastclose() is not enough (and we > >can probably do better). > > > >One idea would be for the core DRM code to know about this property, and make > >sure we put the rotation back to 0 in restore_fbdev_mode(), we do something > >similar for the for the sprite planes in there already. Another idea would be > >to add a vfunc to execute driver specific code there in restore_fbdev_mode(). > > > >There is probably a better way to do it, I have to say I'm not super familiar > >with this part of the driver. > > > I see that in omap driver too it is done in lastclose of the driver. Also, > from driver fbdev_restore is only called during lastclose. > Again I don't have more knowledge on this. > Can we keep it here in this lastclose function to comply with omap driver? No, this really should be done in drm_fb_helper_restore_fbdev_mode_unlocked in drm_fb_helper.c. Well, in the restore_fbdev_mode function in there. Once that's done and once omap is also using the generic rotation properties (I think it is already) we can remove the rotation handling code from omap's last_close. Please also throw a (compile-tested-only) patch on top for that so that Rob Clark can pick it up. -Daniel
On 6/19/2014 12:37 PM, Daniel Vetter wrote: > On Thu, Jun 19, 2014 at 12:13:54PM +0530, Jindal, Sonika wrote: >> On 6/18/2014 10:32 PM, Damien Lespiau wrote: >>> On Wed, Jun 18, 2014 at 02:27:26PM +0530, sonika.jindal@intel.com wrote: >>>> From: Sonika Jindal <sonika.jindal@intel.com> > > >>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c >>>> index 5e583a1..4c91fbc 100644 >>>> --- a/drivers/gpu/drm/i915/i915_dma.c >>>> +++ b/drivers/gpu/drm/i915/i915_dma.c >>>> @@ -1939,6 +1939,8 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file) >>>> void i915_driver_lastclose(struct drm_device *dev) >>>> { >>>> struct drm_i915_private *dev_priv = dev->dev_private; >>>> + struct intel_crtc *crtc; >>>> + struct intel_plane *plane; >>>> >>>> /* On gen6+ we refuse to init without kms enabled, but then the drm core >>>> * goes right around and calls lastclose. Check for this and don't clean >>>> @@ -1946,6 +1948,21 @@ void i915_driver_lastclose(struct drm_device *dev) >>>> if (!dev_priv) >>>> return; >>>> >>>> + if (dev_priv->rotation_property) { >>>> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) { >>>> + to_intel_plane(crtc->base.primary)->rotation = BIT(DRM_ROTATE_0); >>>> + drm_object_property_set_value(&crtc->base.base, >>>> + dev_priv->rotation_property, >>>> + to_intel_plane(crtc->base.primary)->rotation); >>>> + } >>>> + list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) { >>>> + plane->rotation = BIT(DRM_ROTATE_0); >>>> + drm_object_property_set_value(&plane->base.base, >>>> + dev_priv->rotation_property, >>>> + plane->rotation); >>>> + } >>>> + } >>>> + >>> >>> The purpose of this seems to be restoring rotation to 0 and rely on the next >>> modeset to re-program the register. This code, is orthogonal to the pure >>> primary plane rotation enabling, spans through both sprite and primary planes >>> and may actually be a bit tricky to get right. >>> >>> -> This shouldn't be part of the same commit as the primary plane rotation. >>> Please span a new commit for it. >> Sure I will add another patch. >>> >>> Now, we also need something like this when switching VT, and probably for >>> kernel panic and debug handling as well, so lastclose() is not enough (and we >>> can probably do better). >>> >>> One idea would be for the core DRM code to know about this property, and make >>> sure we put the rotation back to 0 in restore_fbdev_mode(), we do something >>> similar for the for the sprite planes in there already. Another idea would be >>> to add a vfunc to execute driver specific code there in restore_fbdev_mode(). >>> >>> There is probably a better way to do it, I have to say I'm not super familiar >>> with this part of the driver. >>> >> I see that in omap driver too it is done in lastclose of the driver. Also, >> from driver fbdev_restore is only called during lastclose. >> Again I don't have more knowledge on this. >> Can we keep it here in this lastclose function to comply with omap driver? > > No, this really should be done in > drm_fb_helper_restore_fbdev_mode_unlocked in drm_fb_helper.c. Well, in the > restore_fbdev_mode function in there. Once that's done and once omap is > also using the generic rotation properties (I think it is already) we can > remove the rotation handling code from omap's last_close. Please also > throw a (compile-tested-only) patch on top for that so that Rob Clark can > pick it up. > -Daniel > Ok, I will add it. So should I add a function pointer say reset_properties in crtc, which will be called from restore_fbdev_mode?
On Thu, Jun 19, 2014 at 9:52 AM, Jindal, Sonika <sonika.jindal@intel.com> wrote: >> No, this really should be done in >> drm_fb_helper_restore_fbdev_mode_unlocked in drm_fb_helper.c. Well, in the >> restore_fbdev_mode function in there. Once that's done and once omap is >> also using the generic rotation properties (I think it is already) we can >> remove the rotation handling code from omap's last_close. Please also >> throw a (compile-tested-only) patch on top for that so that Rob Clark can >> pick it up. >> -Daniel >> > Ok, I will add it. So should I add a function pointer say reset_properties > in crtc, which will be called from restore_fbdev_mode? No, I think it should directly reset the relevant properties. This might mean that we have to move the rotation property pointer to struct drm_plane so that restore_fbdev_mode can get at it. Or we wrap up a helper for internal property setting purposes which bails out if the property isn't attached to the relevant object. This way it will automatically work for all drivers that support rotation. With a callback we still have the same problem that each driver needs to do their own magic, which means they'll get it wrong. Letting helpers take care of such details gives us a much stronger platfrom with drm drivers. -Daniel
On 6/19/2014 1:25 PM, Daniel Vetter wrote: > On Thu, Jun 19, 2014 at 9:52 AM, Jindal, Sonika <sonika.jindal@intel.com> wrote: >>> No, this really should be done in >>> drm_fb_helper_restore_fbdev_mode_unlocked in drm_fb_helper.c. Well, in the >>> restore_fbdev_mode function in there. Once that's done and once omap is >>> also using the generic rotation properties (I think it is already) we can >>> remove the rotation handling code from omap's last_close. Please also >>> throw a (compile-tested-only) patch on top for that so that Rob Clark can >>> pick it up. >>> -Daniel >>> >> Ok, I will add it. So should I add a function pointer say reset_properties >> in crtc, which will be called from restore_fbdev_mode? > > No, I think it should directly reset the relevant properties. This > might mean that we have to move the rotation property pointer to > struct drm_plane so that restore_fbdev_mode can get at it. Or we wrap > up a helper for internal property setting purposes which bails out if > the property isn't attached to the relevant object. So, I will move rotation_property to drm_plane and for each plane where this property is attached, will call drm_object_property_set_value. Please correct me if I am wrong. > > This way it will automatically work for all drivers that support > rotation. With a callback we still have the same problem that each > driver needs to do their own magic, which means they'll get it wrong. > Letting helpers take care of such details gives us a much stronger > platfrom with drm drivers. > -Daniel >
On Thu, Jun 19, 2014 at 01:39:17PM +0530, Jindal, Sonika wrote: > > > On 6/19/2014 1:25 PM, Daniel Vetter wrote: > >On Thu, Jun 19, 2014 at 9:52 AM, Jindal, Sonika <sonika.jindal@intel.com> wrote: > >>>No, this really should be done in > >>>drm_fb_helper_restore_fbdev_mode_unlocked in drm_fb_helper.c. Well, in the > >>>restore_fbdev_mode function in there. Once that's done and once omap is > >>>also using the generic rotation properties (I think it is already) we can > >>>remove the rotation handling code from omap's last_close. Please also > >>>throw a (compile-tested-only) patch on top for that so that Rob Clark can > >>>pick it up. > >>>-Daniel > >>> > >>Ok, I will add it. So should I add a function pointer say reset_properties > >>in crtc, which will be called from restore_fbdev_mode? > > > >No, I think it should directly reset the relevant properties. This > >might mean that we have to move the rotation property pointer to > >struct drm_plane so that restore_fbdev_mode can get at it. Or we wrap > >up a helper for internal property setting purposes which bails out if > >the property isn't attached to the relevant object. > So, I will move rotation_property to drm_plane and for each plane where this > property is attached, will call drm_object_property_set_value. > Please correct me if I am wrong. Yeah, that sounds like a plan. -Daniel > > > >This way it will automatically work for all drivers that support > >rotation. With a callback we still have the same problem that each > >driver needs to do their own magic, which means they'll get it wrong. > >Letting helpers take care of such details gives us a much stronger > >platfrom with drm drivers. > >-Daniel > >
On Thu, Jun 19, 2014 at 01:22:25PM +0530, Jindal, Sonika wrote: > >>I see that in omap driver too it is done in lastclose of the driver. Also, > >>from driver fbdev_restore is only called during lastclose. > >>Again I don't have more knowledge on this. > >>Can we keep it here in this lastclose function to comply with omap driver? Just to be thorough, there's an important path that is not from lastclose(): + drm_fb_helper_set_par() + drm_fb_helper_restore_fbdev_mode_unlocked() + restore_fbdev_mode()
On Thu, Jun 19, 2014 at 12:07 PM, Damien Lespiau <damien.lespiau@intel.com> wrote: > On Thu, Jun 19, 2014 at 01:22:25PM +0530, Jindal, Sonika wrote: >> >>I see that in omap driver too it is done in lastclose of the driver. Also, >> >>from driver fbdev_restore is only called during lastclose. >> >>Again I don't have more knowledge on this. >> >>Can we keep it here in this lastclose function to comply with omap driver? > > Just to be thorough, there's an important path that is not from > lastclose(): > > + drm_fb_helper_set_par() > + drm_fb_helper_restore_fbdev_mode_unlocked() > + restore_fbdev_mode() Yeah, that's the vt restore thing when leaving X/wayland. So we want want this in restore_fbdev_mode even just for i915 - fixing up other drivers is just a bit a bonus. -Daniel
From: Sonika Jindal <sonika.jindal@intel.com>
As suggested by Daniel and Damien, moved rotation_property to drm_plane.
Also moved resetting of rotation_property to restore_fbdev_mode which will be
used in switching VT use case along with the driver lastclose path.
Sonika Jindal (2):
drm/i915: Add 180 degree primary plane rotation support
drm: Resetting rotation property
Ville Syrjälä (1):
drm/i915: Add rotation property for sprites
drivers/gpu/drm/drm_fb_helper.c | 14 ++++-
drivers/gpu/drm/i915/i915_dma.c | 5 --
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/intel_display.c | 93 ++++++++++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_pm.c | 8 +++
drivers/gpu/drm/i915/intel_sprite.c | 40 ++++++++++++++-
include/drm/drm_crtc.h | 1 +
7 files changed, 150 insertions(+), 12 deletions(-)
From: Sonika Jindal <sonika.jindal@intel.com>
As suggested by Daniel and Damien, moved rotation_property to drm_plane.
Also moved resetting of rotation_property to restore_fbdev_mode which will be
used in switching VT use case along with the driver lastclose path.
v2: Removing unused dev_priv from second patch instead of third (some mixup
due to earlier reformatting).
Sonika Jindal (2):
drm/i915: Add 180 degree primary plane rotation support
drm: Resetting rotation property
Ville Syrjälä (1):
drm/i915: Add rotation property for sprites
drivers/gpu/drm/drm_fb_helper.c | 14 ++++-
drivers/gpu/drm/i915/i915_dma.c | 5 --
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/intel_display.c | 93 ++++++++++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_pm.c | 8 +++
drivers/gpu/drm/i915/intel_sprite.c | 40 ++++++++++++++-
include/drm/drm_crtc.h | 1 +
7 files changed, 150 insertions(+), 12 deletions(-)
Hi, On 06/18/2014 09:57 AM, sonika.jindal@intel.com wrote: [snip] > +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 intel_crtc *intel_crtc = to_intel_crtc(plane->crtc); > + struct drm_crtc *crtc = &intel_crtc->base; > + uint64_t old_val; > + int ret = -ENOENT; > + > + if (prop == dev_priv->rotation_property) { > + /* exactly one rotation angle please */ > + if (hweight32(val & 0xf) != 1) > + return -EINVAL; > + > + old_val = intel_plane->rotation; > + intel_plane->rotation = val; > + > + if (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 (dev_priv->fbc.plane == intel_crtc->plane && > + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && > + intel_plane->rotation != BIT(DRM_ROTATE_0)) > + intel_disable_fbc(dev); > + > + dev_priv->display.update_primary_plane(crtc, crtc->primary->fb, 0, 0); > + } else { > + DRM_DEBUG_KMS("[CRTC:%d] is not active. Only rotation property is updated\n", > + crtc->base.id); > + ret = 0; > + } > + } > + > + return ret; > +} It looks like this will incorrectly propagate -ENOENT if property on an active plane is modified. Regards, Tvrtko
Hi, On 06/18/2014 09:57 AM, sonika.jindal@intel.com wrote: [snip] > +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 intel_crtc *intel_crtc = to_intel_crtc(plane->crtc); > + struct drm_crtc *crtc = &intel_crtc->base; > + uint64_t old_val; > + int ret = -ENOENT; > + > + if (prop == dev_priv->rotation_property) { > + /* exactly one rotation angle please */ > + if (hweight32(val & 0xf) != 1) > + return -EINVAL; > + > + old_val = intel_plane->rotation; > + intel_plane->rotation = val; > + > + if (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 (dev_priv->fbc.plane == intel_crtc->plane && > + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && > + intel_plane->rotation != BIT(DRM_ROTATE_0)) > + intel_disable_fbc(dev); Also, do we need a path for turning FBC back on once plane orientation goes back to a supported configuration? Regards, Tvrtko
On 6/27/2014 4:04 PM, Tvrtko Ursulin wrote: > Hi, > > On 06/18/2014 09:57 AM, sonika.jindal@intel.com wrote: > [snip] >> +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 intel_crtc *intel_crtc = to_intel_crtc(plane->crtc); >> + struct drm_crtc *crtc = &intel_crtc->base; >> + uint64_t old_val; >> + int ret = -ENOENT; >> + >> + if (prop == dev_priv->rotation_property) { >> + /* exactly one rotation angle please */ >> + if (hweight32(val & 0xf) != 1) >> + return -EINVAL; >> + >> + old_val = intel_plane->rotation; >> + intel_plane->rotation = val; >> + >> + if (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 (dev_priv->fbc.plane == intel_crtc->plane && >> + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && >> + intel_plane->rotation != BIT(DRM_ROTATE_0)) >> + intel_disable_fbc(dev); >> + >> + dev_priv->display.update_primary_plane(crtc, >> crtc->primary->fb, 0, 0); >> + } else { >> + DRM_DEBUG_KMS("[CRTC:%d] is not active. Only rotation >> property is updated\n", >> + crtc->base.id); >> + ret = 0; >> + } >> + } >> + >> + return ret; >> +} > > It looks like this will incorrectly propagate -ENOENT if property on an > active plane is modified. > > Regards, > > Tvrtko > > Yes, this was corrected in the next patch set. Can you please refer to the latest patches where we moved the property to drm_plane instead of intel_plane: http://lists.freedesktop.org/archives/intel-gfx/2014-June/047910.html -Sonika
On 06/27/2014 11:49 AM, Jindal, Sonika wrote: > > > On 6/27/2014 4:04 PM, Tvrtko Ursulin wrote: >> Hi, >> >> On 06/18/2014 09:57 AM, sonika.jindal@intel.com wrote: >> [snip] >>> +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 intel_crtc *intel_crtc = to_intel_crtc(plane->crtc); >>> + struct drm_crtc *crtc = &intel_crtc->base; >>> + uint64_t old_val; >>> + int ret = -ENOENT; >>> + >>> + if (prop == dev_priv->rotation_property) { >>> + /* exactly one rotation angle please */ >>> + if (hweight32(val & 0xf) != 1) >>> + return -EINVAL; >>> + >>> + old_val = intel_plane->rotation; >>> + intel_plane->rotation = val; >>> + >>> + if (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 (dev_priv->fbc.plane == intel_crtc->plane && >>> + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && >>> + intel_plane->rotation != BIT(DRM_ROTATE_0)) >>> + intel_disable_fbc(dev); >>> + >>> + dev_priv->display.update_primary_plane(crtc, >>> crtc->primary->fb, 0, 0); >>> + } else { >>> + DRM_DEBUG_KMS("[CRTC:%d] is not active. Only rotation >>> property is updated\n", >>> + crtc->base.id); >>> + ret = 0; >>> + } >>> + } >>> + >>> + return ret; >>> +} >> >> It looks like this will incorrectly propagate -ENOENT if property on an >> active plane is modified. >> >> Regards, >> >> Tvrtko >> >> > Yes, this was corrected in the next patch set. Can you please refer to > the latest patches where we moved the property to drm_plane instead of > intel_plane: > http://lists.freedesktop.org/archives/intel-gfx/2014-June/047910.html Alright, I missed that series since it is bit indented in the thread. Does it replace only patch 10 from the original series? Or in other words first nine should be applied first, then these three? Tvrtko
On 6/27/2014 4:42 PM, Tvrtko Ursulin wrote: > > On 06/27/2014 11:49 AM, Jindal, Sonika wrote: >> >> >> On 6/27/2014 4:04 PM, Tvrtko Ursulin wrote: >>> Hi, >>> >>> On 06/18/2014 09:57 AM, sonika.jindal@intel.com wrote: >>> [snip] >>>> +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 intel_crtc *intel_crtc = to_intel_crtc(plane->crtc); >>>> + struct drm_crtc *crtc = &intel_crtc->base; >>>> + uint64_t old_val; >>>> + int ret = -ENOENT; >>>> + >>>> + if (prop == dev_priv->rotation_property) { >>>> + /* exactly one rotation angle please */ >>>> + if (hweight32(val & 0xf) != 1) >>>> + return -EINVAL; >>>> + >>>> + old_val = intel_plane->rotation; >>>> + intel_plane->rotation = val; >>>> + >>>> + if (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 (dev_priv->fbc.plane == intel_crtc->plane && >>>> + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && >>>> + intel_plane->rotation != BIT(DRM_ROTATE_0)) >>>> + intel_disable_fbc(dev); >>>> + >>>> + dev_priv->display.update_primary_plane(crtc, >>>> crtc->primary->fb, 0, 0); >>>> + } else { >>>> + DRM_DEBUG_KMS("[CRTC:%d] is not active. Only rotation >>>> property is updated\n", >>>> + crtc->base.id); >>>> + ret = 0; >>>> + } >>>> + } >>>> + >>>> + return ret; >>>> +} >>> >>> It looks like this will incorrectly propagate -ENOENT if property on an >>> active plane is modified. >>> >>> Regards, >>> >>> Tvrtko >>> >>> >> Yes, this was corrected in the next patch set. Can you please refer to >> the latest patches where we moved the property to drm_plane instead of >> intel_plane: >> http://lists.freedesktop.org/archives/intel-gfx/2014-June/047910.html > > Alright, I missed that series since it is bit indented in the thread. > Does it replace only patch 10 from the original series? Or in other > words first nine should be applied first, then these three? > > Tvrtko > > > It replaces last two patches in the series, rotation property for sprites as well as primary planes. -Sonika
On 6/27/2014 4:08 PM, Tvrtko Ursulin wrote: > Hi, > > On 06/18/2014 09:57 AM, sonika.jindal@intel.com wrote: > [snip] >> +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 intel_crtc *intel_crtc = to_intel_crtc(plane->crtc); >> + struct drm_crtc *crtc = &intel_crtc->base; >> + uint64_t old_val; >> + int ret = -ENOENT; >> + >> + if (prop == dev_priv->rotation_property) { >> + /* exactly one rotation angle please */ >> + if (hweight32(val & 0xf) != 1) >> + return -EINVAL; >> + >> + old_val = intel_plane->rotation; >> + intel_plane->rotation = val; >> + >> + if (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 (dev_priv->fbc.plane == intel_crtc->plane && >> + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && >> + intel_plane->rotation != BIT(DRM_ROTATE_0)) >> + intel_disable_fbc(dev); > > Also, do we need a path for turning FBC back on once plane orientation > goes back to a supported configuration? > > Regards, > > Tvrtko > True, looks like it should be added. I'l add and post. -Sonika
Hi, Did anybody get a chance to review these patches? Thanks, Sonika On 6/24/2014 5:38 PM, sonika.jindal@intel.com wrote: > From: Sonika Jindal <sonika.jindal@intel.com> > > As suggested by Daniel and Damien, moved rotation_property to drm_plane. > Also moved resetting of rotation_property to restore_fbdev_mode which will be > used in switching VT use case along with the driver lastclose path. > > v2: Removing unused dev_priv from second patch instead of third (some mixup > due to earlier reformatting). > > Sonika Jindal (2): > drm/i915: Add 180 degree primary plane rotation support > drm: Resetting rotation property > > Ville Syrjälä (1): > drm/i915: Add rotation property for sprites > > drivers/gpu/drm/drm_fb_helper.c | 14 ++++- > drivers/gpu/drm/i915/i915_dma.c | 5 -- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_display.c | 93 ++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_pm.c | 8 +++ > drivers/gpu/drm/i915/intel_sprite.c | 40 ++++++++++++++- > include/drm/drm_crtc.h | 1 + > 7 files changed, 150 insertions(+), 12 deletions(-) >
On Wed, Jul 02, 2014 at 02:21:52PM +0530, Jindal, Sonika wrote: > Hi, > > Did anybody get a chance to review these patches? Ok, now those patches are all over the place. It's pretty much impossible to have the big picture again with the latest changes (the rotation on drm_plane) nor the limit of the the series, esp. as the documentation patches have been split off. I'm not sure why, but it seems like the Cc: tags in your emails have never actually ended up in the Cc: header of your mails. Which means they never hit dri-devel. Can you make sure you fix that for the next resend. This means we haven't gathered their feedback during all this time... What you can do: Use --dry-run to make sure you're sending them the dri-devel. Maybe use the --cc command line option instead? Also, as already mentioned, don't send the patches to the LKML, noone will review them there. Could you resend the full series with the reviewed-by tags gathered so far? with dri-devel in copy? in a separarte mail thread? We usually version the big resend of series in the cover letter, explaning the changes. Thanks,
On Wed, Jul 02, 2014 at 02:17:00PM +0100, Damien Lespiau wrote: > On Wed, Jul 02, 2014 at 02:21:52PM +0530, Jindal, Sonika wrote: > > Hi, > > > > Did anybody get a chance to review these patches? > > Ok, now those patches are all over the place. It's pretty much > impossible to have the big picture again with the latest changes (the > rotation on drm_plane) nor the limit of the the series, esp. as the > documentation patches have been split off. > > I'm not sure why, but it seems like the Cc: tags in your emails have > never actually ended up in the Cc: header of your mails. Which means > they never hit dri-devel. Can you make sure you fix that for the next > resend. This means we haven't gathered their feedback during all this > time... > > What you can do: Use --dry-run to make sure you're sending them the > dri-devel. Maybe use the --cc command line option instead? > > Also, as already mentioned, don't send the patches to the LKML, noone > will review them there. > > Could you resend the full series with the reviewed-by tags gathered so > far? with dri-devel in copy? in a separarte mail thread? We usually > version the big resend of series in the cover letter, explaning the > changes. BKM for resending patches: When you resend the full series or large parts of it, start a new thread with _all_ the patches. But don't do that before a few days have passed to make sure that the review won't get get split between two threads. If you have some small fixups and want to get quick feedback just resubmit _individual_ patches in-reply-to the previous version. Yes that's quite a bit of work since you need to send out each patch individually with the right --in-reply-to parameters. Following these bkms will make sure that the discussion stays together as much as possible and that everyone can find the latest patch versions even in a really busy review thread. Resending only part of patch series and then in-reply-to somewehere deep in an existing thread as on group will make a mess. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 5e583a1..4c91fbc 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1939,6 +1939,8 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file) void i915_driver_lastclose(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_crtc *crtc; + struct intel_plane *plane; /* On gen6+ we refuse to init without kms enabled, but then the drm core * goes right around and calls lastclose. Check for this and don't clean @@ -1946,6 +1948,21 @@ void i915_driver_lastclose(struct drm_device *dev) if (!dev_priv) return; + if (dev_priv->rotation_property) { + list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) { + to_intel_plane(crtc->base.primary)->rotation = BIT(DRM_ROTATE_0); + drm_object_property_set_value(&crtc->base.base, + dev_priv->rotation_property, + to_intel_plane(crtc->base.primary)->rotation); + } + list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) { + plane->rotation = BIT(DRM_ROTATE_0); + drm_object_property_set_value(&plane->base.base, + dev_priv->rotation_property, + plane->rotation); + } + } + if (drm_core_check_feature(dev, DRIVER_MODESET)) { intel_fbdev_restore_mode(dev); vga_switcheroo_process_delayed_switch(); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index c70c804..c600d3b 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4087,6 +4087,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 5e8e711..1dc8b68 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2414,7 +2414,9 @@ 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); intel_fb = to_intel_framebuffer(fb); obj = intel_fb->obj; @@ -2422,6 +2424,8 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, 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; @@ -2463,8 +2467,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) { @@ -2477,6 +2479,17 @@ 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); + 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]); @@ -2504,7 +2517,9 @@ 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); intel_fb = to_intel_framebuffer(fb); obj = intel_fb->obj; @@ -2512,6 +2527,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, 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; @@ -2549,8 +2566,6 @@ 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, @@ -2558,6 +2573,19 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, 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, fb->pitches[0]); @@ -11324,10 +11352,51 @@ 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 intel_crtc *intel_crtc = to_intel_crtc(plane->crtc); + struct drm_crtc *crtc = &intel_crtc->base; + uint64_t old_val; + int ret = -ENOENT; + + if (prop == dev_priv->rotation_property) { + /* exactly one rotation angle please */ + if (hweight32(val & 0xf) != 1) + return -EINVAL; + + old_val = intel_plane->rotation; + intel_plane->rotation = val; + + if (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 (dev_priv->fbc.plane == intel_crtc->plane && + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && + intel_plane->rotation != BIT(DRM_ROTATE_0)) + intel_disable_fbc(dev); + + dev_priv->display.update_primary_plane(crtc, crtc->primary->fb, 0, 0); + } else { + DRM_DEBUG_KMS("[CRTC:%d] is not active. Only rotation property is updated\n", + crtc->base.id); + ret = 0; + } + } + + return ret; +} + 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, @@ -11335,6 +11404,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, { struct intel_plane *primary; const uint32_t *intel_primary_formats; + struct drm_i915_private *dev_priv = dev->dev_private; int num_formats; primary = kzalloc(sizeof(*primary), GFP_KERNEL); @@ -11345,6 +11415,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; @@ -11360,6 +11431,18 @@ 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_priv->rotation_property) + dev_priv->rotation_property = + drm_mode_create_rotation_property(dev, + BIT(DRM_ROTATE_0) | + BIT(DRM_ROTATE_180)); + if (dev_priv->rotation_property) + drm_object_attach_property(&primary->base.base, + dev_priv->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 2043c4b..cabccfb 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -562,6 +562,14 @@ void intel_update_fbc(struct drm_device *dev) 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("mode incompatible with compression, " + "disabling\n"); + goto out_disable; + } + /* If the kernel debugger is active, always disable compression */ if (in_dbg_master()) goto out_disable;