On Fri, Apr 11, 2014 at 02:13:42PM -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. > > v4: > - Don't add a primary_plane field to intel_crtc; that was left over > from a much earlier iteration of this patch series, but is no longer > needed/used now that the DRM core primary plane support has been > merged. > v3: > - Provide gen-specific primary plane format lists (suggested by Daniel > Vetter). > - If the primary plane is already enabled, go ahead and just call the > primary plane helper to do the update (suggested by Daniel Vetter). > - Don't try to disable the primary plane on destruction; the DRM layer > should have already taken care of this for us. > v2: > - Unpin fb properly on primary plane disable > - Provide an Intel-specific set of primary plane formats > - Additional sanity checks on setplane (in line with the checks > currently being done by the DRM core primary plane helper) > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx> > --- > drivers/gpu/drm/i915/intel_display.c | 197 ++++++++++++++++++++++++++++++++++- > 1 file changed, 195 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 1390ab5..3e4d36a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -39,8 +39,35 @@ > #include "i915_trace.h" > #include <drm/drm_dp_helper.h> > #include <drm/drm_crtc_helper.h> > +#include <drm/drm_plane_helper.h> > +#include <drm/drm_rect.h> > #include <linux/dma_remapping.h> > > +/* Primary plane formats supported by all gen */ > +#define COMMON_PRIMARY_FORMATS \ > + DRM_FORMAT_C8, \ > + DRM_FORMAT_RGB565, \ > + DRM_FORMAT_XRGB8888, \ > + DRM_FORMAT_ARGB8888 > + > +/* Primary plane formats for gen <= 3 */ > +const static uint32_t intel_primary_formats_gen3[] = {q 'static const' is the more typical order > + COMMON_PRIMARY_FORMATS, > + DRM_FORMAT_XRGB1555, > + DRM_FORMAT_ARGB1555, > +}; > + > +/* Primary plane formats for gen >= 4 */ > +const static uint32_t intel_primary_formats_gen4[] = { ditto > + COMMON_PRIMARY_FORMATS, \ > + DRM_FORMAT_XBGR8888, > + DRM_FORMAT_ABGR8888, > + DRM_FORMAT_XRGB2101010, > + DRM_FORMAT_ARGB2101010, > + DRM_FORMAT_XBGR2101010, > + DRM_FORMAT_ABGR2101010, > +}; > + > static void intel_increase_pllclock(struct drm_crtc *crtc); > static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on); > > @@ -10556,17 +10583,183 @@ 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, > + }; 'clip' can be const, and these should be pipe_src_{w,h} just like we have in the sprite code. > + int ret; > + > + /* > + * At the moment we use the same set of setplane restrictions as the > + * DRM primary plane helper, so go ahead and just call the helper if > + * the primary plane is already enabled. We only need to take special > + * action if the primary plane is disabled (something i915 can do but > + * the generic helper can't). > + */ > + if (intel_crtc->primary_enabled) > + return drm_primary_helper_update(plane, crtc, fb, > + crtc_x, crtc_y, > + crtc_w, crtc_h, > + src_x, src_y, > + src_w, src_h); Why would we want to call that if we have a custom implementation anyway? > + > + /* 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); src needs to be clipped too. I guess we should get a sufficiently good result if we just call 'drm_rect_clip_scaled(..., 1, 1)' here. Ie. clip after throwing away the sub-pixel parts from the src coordinates. To match the sprite behaviour a bit better we should also allow cases where the primary plane becomes fully clipped. In such cases we should just disable the plane. > + if (dest.x1 != 0 || dest.y1 != 0 || > + dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) { !drm_rect_equals(&dest, &clip) > + 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; > + } This check could be moved to happen before we clip. > + > + /* Primary planes are locked to their owning CRTC */ > + if (plane->possible_crtcs != drm_crtc_mask(crtc)) { > + DRM_DEBUG_KMS("Cannot change primary plane CRTC\n"); > + return -EINVAL; > + } Looks like possible_crtcs check could be in drm_mode_setplane(). No need to special case primary planes here. > + > + /* Framebuffer must be big enough to cover entire plane */ > + ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb); > + if (ret) > + return ret; drm_mode_setplane() should already have checked that the viewport doesn't exceed the fb. > + > + /* > + * 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; > + > + intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane, > + intel_crtc->pipe); Needs a primary_enabled check (+ a visibility check for the fully clipped case). > + > + return 0; > +} > + > +static int > +intel_primary_plane_disable(struct drm_plane *plane) > +{ > + 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; > + > + 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); > + > + /* > + * N.B. The DRM setplane code will update the plane->fb pointer after > + * we finish here. > + */ > + intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj); > + > + return 0; > +} > + > +static void intel_primary_plane_destroy(struct drm_plane *plane) > +{ > + struct intel_plane *intel_plane = to_intel_plane(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; > + const uint32_t *intel_primary_formats; > + int num_formats; > + > + primary = kzalloc(sizeof(*primary), GFP_KERNEL); > + if (primary == NULL) > + return NULL; > + > + primary->can_scale = false; > + primary->pipe = pipe; > + primary->plane = pipe; We need to handle the pre-gen4 fbc primary plane swapping somewhere. I guess we still have intel_crtc->plane as well. That should probably get converted to use crtc->primary->plane instead. > + > + if (INTEL_INFO(dev)->gen <= 3) { > + intel_primary_formats = intel_primary_formats_gen3; > + num_formats = ARRAY_SIZE(intel_primary_formats_gen3); > + } else { > + intel_primary_formats = intel_primary_formats_gen4; > + num_formats = ARRAY_SIZE(intel_primary_formats_gen4); > + } > + > + drm_plane_init(dev, &primary->base, 0, > + &intel_primary_plane_funcs, intel_primary_formats, > + num_formats, DRM_PLANE_TYPE_PRIMARY); > + return &primary->base; > +} > + > static void intel_crtc_init(struct drm_device *dev, int pipe) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc; > - int i; > + struct drm_plane *primary; > + int i, ret; > > intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL); > if (intel_crtc == NULL) > return; > > - drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs); > + primary = intel_primary_plane_create(dev, pipe); > + ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary, > + NULL, &intel_crtc_funcs); > + if (ret) { > + drm_crtc_cleanup(&intel_crtc->base); That should be drm_plane_cleanup() no? > + kfree(intel_crtc); > + return; > + } > > drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256); > for (i = 0; i < 256; i++) { > -- > 1.8.5.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx