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
