| Message ID | 1395188579-17191-12-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Mar 18, 2014 at 05:22:56PM -0700, Matt Roper wrote: > Intel hardware allows the primary plane to be disabled independently of > the CRTC. Provide custom primary plane handling to allow this. > > Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Overall this is imo a new feature since it exposes primary plane disabling to userspace. Which means I want a crc based igt for this. Two interesting cases imo: 1) Partially visible primary plane behind an overlay plane. Disabling it should change those areas from the primary plane to grey or something, so easy to check with CRCs. 2) Primary plane + cursor, disable primary plane. Then check that the cursor is still working. Same for overlay sprites. At least on all currently supported platforms we don't have unified planes in the hardware, so imo it's worth to check that this works properly. > --- > drivers/gpu/drm/i915/intel_display.c | 132 ++++++++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 130 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 849a241..7d6878b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -39,6 +39,7 @@ > #include "i915_trace.h" > #include <drm/drm_dp_helper.h> > #include <drm/drm_crtc_helper.h> > +#include <drm/drm_rect.h> > #include <linux/dma_remapping.h> > > static void intel_increase_pllclock(struct drm_crtc *crtc); > @@ -10589,19 +10590,144 @@ static void intel_shared_dpll_init(struct drm_device *dev) > BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); > } > > +static int > +intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, > + struct drm_framebuffer *fb, int crtc_x, int crtc_y, > + unsigned int crtc_w, unsigned int crtc_h, > + uint32_t src_x, uint32_t src_y, > + uint32_t src_w, uint32_t src_h) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct drm_framebuffer *tmpfb; > + struct drm_rect dest = { > + .x1 = crtc_x, > + .y1 = crtc_y, > + .x2 = crtc_x + crtc_w, > + .y2 = crtc_y + crtc_h, > + }; > + struct drm_rect clip = { > + .x2 = crtc->mode.hdisplay, > + .y2 = crtc->mode.vdisplay, > + }; > + int ret; > + > + /* setplane API takes shifted source rectangle values; unshift them */ > + src_x >>= 16; > + src_y >>= 16; > + src_w >>= 16; > + src_h >>= 16; Similar comments as with the generic helper: We need to check the scaling constraints here better. Looks like a good opportunity to extract the logic into some drm_rect helpers maybe? > + > + /* > + * Current hardware can't reposition the primary plane or scale it > + * (although this could change in the future). > + */ > + drm_rect_intersect(&dest, &clip); > + if (dest.x1 != 0 || dest.y1 != 0 || > + dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) { > + DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n"); > + return -EINVAL; > + } > + > + if (crtc_w != src_w || crtc_h != src_h) { > + DRM_DEBUG_KMS("Can't scale primary plane\n"); > + return -EINVAL; > + } > + > + /* > + * pipe_set_base() adjusts crtc->primary->fb; however the DRM setplane > + * code that called us expects to handle the framebuffer update and > + * reference counting; save and restore the current fb before > + * calling it. > + */ > + tmpfb = plane->fb; > + ret = intel_pipe_set_base(crtc, src_x, src_y, fb); > + if (ret) > + return ret; > + plane->fb = tmpfb; > + > + if (!intel_crtc->primary_enabled) > + intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane, > + intel_crtc->pipe); > + > + return 0; > +} > + > +static int > +intel_primary_plane_disable(struct drm_plane *plane) > +{ > + struct drm_device *dev = plane->dev; > + drm_i915_private_t *dev_priv = dev->dev_private; > + struct intel_plane *intel_plane = to_intel_plane(plane); > + struct intel_crtc *intel_crtc; > + > + if (!plane->fb) > + return 0; > + > + if (WARN_ON(!plane->crtc)) > + return -EINVAL; > + > + intel_crtc = to_intel_crtc(plane->crtc); > + if (intel_crtc->primary_enabled) > + intel_disable_primary_hw_plane(dev_priv, intel_plane->plane, > + intel_plane->pipe); > + > + return 0; > +} > + > +static void intel_primary_plane_destroy(struct drm_plane *plane) > +{ > + struct intel_plane *intel_plane = to_intel_plane(plane); > + intel_primary_plane_disable(plane); > + drm_plane_cleanup(plane); > + kfree(intel_plane); > +} > + > +static const struct drm_plane_funcs intel_primary_plane_funcs = { > + .update_plane = intel_primary_plane_setplane, > + .disable_plane = intel_primary_plane_disable, > + .destroy = intel_primary_plane_destroy, > +}; > + > +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > + int pipe) > +{ > + struct intel_plane *primary; > + > + primary = kzalloc(sizeof(*primary), GFP_KERNEL); > + if (primary == NULL) > + return NULL; > + > + primary->can_scale = false; > + primary->pipe = pipe; > + primary->plane = pipe; > + > + drm_plane_init(dev, &primary->base, 0, > + &intel_primary_plane_funcs, legacy_modeset_formats, > + ARRAY_SIZE(legacy_modeset_formats), We need our own proper format list for primary planes - we don't support yuv and other crazy stuff like that on them. Also since we can now expose this, I think we should have per-platform lists. E.g. only gen2/3 support xrgb1555 and only gen4+ support xrgb2101010. Our framebuffer creation code has all the limits properly encoded atm. > + DRM_PLANE_TYPE_PRIMARY); > + return &primary->base; > +} > + > static void intel_crtc_init(struct drm_device *dev, int pipe) > { > drm_i915_private_t *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc; > struct drm_plane *primary; > - int i; > + int i, ret; > > intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL); > if (intel_crtc == NULL) > return; > > - primary = drm_primary_helper_create_plane(dev); > - drm_crtc_init(dev, &intel_crtc->base, primary, &intel_crtc_funcs); > + primary = intel_primary_plane_create(dev, pipe); > + ret = drm_crtc_init(dev, &intel_crtc->base, primary, &intel_crtc_funcs); > + if (ret) { > + drm_crtc_cleanup(&intel_crtc->base); > + kfree(intel_crtc); > + return; > + } > > drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256); > for (i = 0; i < 256; i++) { > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 890c5cd..770f80d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -351,6 +351,7 @@ struct intel_crtc { > bool active; > unsigned long enabled_power_domains; > bool eld_vld; > + struct intel_plane *primary_plane; > bool primary_enabled; /* is the primary plane (partially) visible? */ > bool lowfreq_avail; > struct intel_overlay *overlay; > -- > 1.8.5.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Mar 19, 2014 at 01:11:26PM +0100, Daniel Vetter wrote: > On Tue, Mar 18, 2014 at 05:22:56PM -0700, Matt Roper wrote: > > Intel hardware allows the primary plane to be disabled independently of > > the CRTC. Provide custom primary plane handling to allow this. > > > > Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > Overall this is imo a new feature since it exposes primary plane disabling > to userspace. Which means I want a crc based igt for this. Two interesting > cases imo: > > 1) Partially visible primary plane behind an overlay plane. Disabling it > should change those areas from the primary plane to grey or something, so > easy to check with CRCs. > > 2) Primary plane + cursor, disable primary plane. Then check that the > cursor is still working. Same for overlay sprites. > > At least on all currently supported platforms we don't have unified planes > in the hardware, so imo it's worth to check that this works properly. One big reason I've forgotten why I really want testcase for this is that historically our code has fallen over in _really_ bad ways without a primary fb. fastboot has brought a lot of these issues to light (since we occasionally fail to wrap up the firmware's fb properly). So having a bit of a baseline testcase so that we can easily add regression tests for specific bugs once we inevitably run into them is good prep work, too. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 849a241..7d6878b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -39,6 +39,7 @@ #include "i915_trace.h" #include <drm/drm_dp_helper.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_rect.h> #include <linux/dma_remapping.h> static void intel_increase_pllclock(struct drm_crtc *crtc); @@ -10589,19 +10590,144 @@ static void intel_shared_dpll_init(struct drm_device *dev) BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); } +static int +intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, + struct drm_framebuffer *fb, int crtc_x, int crtc_y, + unsigned int crtc_w, unsigned int crtc_h, + uint32_t src_x, uint32_t src_y, + uint32_t src_w, uint32_t src_h) +{ + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_framebuffer *tmpfb; + struct drm_rect dest = { + .x1 = crtc_x, + .y1 = crtc_y, + .x2 = crtc_x + crtc_w, + .y2 = crtc_y + crtc_h, + }; + struct drm_rect clip = { + .x2 = crtc->mode.hdisplay, + .y2 = crtc->mode.vdisplay, + }; + int ret; + + /* setplane API takes shifted source rectangle values; unshift them */ + src_x >>= 16; + src_y >>= 16; + src_w >>= 16; + src_h >>= 16; + + /* + * Current hardware can't reposition the primary plane or scale it + * (although this could change in the future). + */ + drm_rect_intersect(&dest, &clip); + if (dest.x1 != 0 || dest.y1 != 0 || + dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) { + DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n"); + return -EINVAL; + } + + if (crtc_w != src_w || crtc_h != src_h) { + DRM_DEBUG_KMS("Can't scale primary plane\n"); + return -EINVAL; + } + + /* + * pipe_set_base() adjusts crtc->primary->fb; however the DRM setplane + * code that called us expects to handle the framebuffer update and + * reference counting; save and restore the current fb before + * calling it. + */ + tmpfb = plane->fb; + ret = intel_pipe_set_base(crtc, src_x, src_y, fb); + if (ret) + return ret; + plane->fb = tmpfb; + + if (!intel_crtc->primary_enabled) + intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane, + intel_crtc->pipe); + + return 0; +} + +static int +intel_primary_plane_disable(struct drm_plane *plane) +{ + struct drm_device *dev = plane->dev; + drm_i915_private_t *dev_priv = dev->dev_private; + struct intel_plane *intel_plane = to_intel_plane(plane); + struct intel_crtc *intel_crtc; + + if (!plane->fb) + return 0; + + if (WARN_ON(!plane->crtc)) + return -EINVAL; + + intel_crtc = to_intel_crtc(plane->crtc); + if (intel_crtc->primary_enabled) + intel_disable_primary_hw_plane(dev_priv, intel_plane->plane, + intel_plane->pipe); + + return 0; +} + +static void intel_primary_plane_destroy(struct drm_plane *plane) +{ + struct intel_plane *intel_plane = to_intel_plane(plane); + intel_primary_plane_disable(plane); + drm_plane_cleanup(plane); + kfree(intel_plane); +} + +static const struct drm_plane_funcs intel_primary_plane_funcs = { + .update_plane = intel_primary_plane_setplane, + .disable_plane = intel_primary_plane_disable, + .destroy = intel_primary_plane_destroy, +}; + +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, + int pipe) +{ + struct intel_plane *primary; + + primary = kzalloc(sizeof(*primary), GFP_KERNEL); + if (primary == NULL) + return NULL; + + primary->can_scale = false; + primary->pipe = pipe; + primary->plane = pipe; + + drm_plane_init(dev, &primary->base, 0, + &intel_primary_plane_funcs, legacy_modeset_formats, + ARRAY_SIZE(legacy_modeset_formats), + DRM_PLANE_TYPE_PRIMARY); + return &primary->base; +} + static void intel_crtc_init(struct drm_device *dev, int pipe) { drm_i915_private_t *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc; struct drm_plane *primary; - int i; + int i, ret; intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL); if (intel_crtc == NULL) return; - primary = drm_primary_helper_create_plane(dev); - drm_crtc_init(dev, &intel_crtc->base, primary, &intel_crtc_funcs); + primary = intel_primary_plane_create(dev, pipe); + ret = drm_crtc_init(dev, &intel_crtc->base, primary, &intel_crtc_funcs); + if (ret) { + drm_crtc_cleanup(&intel_crtc->base); + kfree(intel_crtc); + return; + } drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256); for (i = 0; i < 256; i++) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 890c5cd..770f80d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -351,6 +351,7 @@ struct intel_crtc { bool active; unsigned long enabled_power_domains; bool eld_vld; + struct intel_plane *primary_plane; bool primary_enabled; /* is the primary plane (partially) visible? */ bool lowfreq_avail; struct intel_overlay *overlay;
Intel hardware allows the primary plane to be disabled independently of the CRTC. Provide custom primary plane handling to allow this. Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 132 ++++++++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_drv.h | 1 + 2 files changed, 130 insertions(+), 3 deletions(-)