Re: [Intel-gfx] [PATCH 4/4] drm/i915: Register primary plane for each CRTC

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Feb 27, 2014 at 02:14:43PM -0800, Matt Roper wrote:
> Create a primary plane at CRTC init and hook up handlers for the various
> operations that may be performed on it.  The DRM core will only
> advertise the primary planes to clients that set the appropriate
> capability bit.
> 
> Since we're limited to the legacy plane operations at the moment
> (SetPlane and such) this isn't terribly interesting yet; the plane
> update handler will perform an MMIO flip of the display plane and the
> disable handler will disable the CRTC.  Once we migrate more of the
> plane and CRTC info over to properties in preparation for atomic/nuclear
> operations, primary planes will be more useful.
> 
> 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 | 92 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9757010..d9a5cd5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8260,6 +8260,10 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
>  
>  	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
>  
> +	drm_plane_cleanup(crtc->primary_plane);
> +	kfree(crtc->primary_plane);
> +	crtc->primary_plane = NULL;
> +
>  	drm_crtc_cleanup(crtc);
>  
>  	kfree(intel_crtc);
> @@ -10272,17 +10276,105 @@ 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;
> +
> +	/* setplane API takes shifted source rectangle values; unshift them */
> +	src_x >>= 16;
> +	src_y >>= 16;
> +	src_w >>= 16;
> +	src_h >>= 16;
> +
> +	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384) {
> +		DRM_DEBUG_KMS("Unsuitable framebuffer for primary plane\n");
> +		return -EINVAL;
> +	}

These aren't quite right for all gens. Actually I think the primary plane
limits are genereally more relaxed than the sprite plane limits, so 
intel_framebuffer_init() should have already done the necessary checks,
at least as far the stride is concerned. So I'd just drop the stride
check. In the future I suppose we might need to add some extra checks
here too, but for now I think we should fine w/o them.

> +
> +	/*
> +	 * Current hardware can't reposition the primary plane or scale it
> +	 * (although this could change in the future).  This means that we
> +	 * don't actually need any of the destination (crtc) rectangle values,
> +	 * or the source rectangle width/height; only the source x/y winds up
> +	 * getting used for panning.  Nevertheless, let's sanity check the
> +	 * incoming values to make sure userspace didn't think it could scale
> +	 * or reposition this plane.
> +	 */
> +	if (crtc_w != crtc->mode.hdisplay || crtc_h != crtc->mode.vdisplay ||
> +	    crtc_x != 0 || crtc_y != 0) {
> +		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;
> +	}

I'd do the clipping anyway, and then check that the dst size matches the
pipe src size post clipping. Otherwise the behaviour is rather
inconsistent with the sprite planes.

> +
> +	intel_pipe_set_base(crtc, src_x, src_y, fb);

This can return an error.

> +	dev_priv->display.crtc_enable(crtc);

Shouldn't enable the entire pipe, just the plane.

> +
> +	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;
> +
> +	if (!plane->fb)
> +		return 0;
> +
> +	if (WARN_ON(!plane->crtc || plane->crtc->primary_plane != plane))
> +		return -EINVAL;
> +
> +	dev_priv->display.crtc_disable(plane->crtc);

This should just disable the plane, not the entire pipe.

> +
> +	return 0;
> +}
> +
> +static void intel_primary_plane_destroy(struct drm_plane *plane)
> +{
> +	/*
> +	 * Since primary planes are never put on the mode_config plane list,
> +	 * this entry point should never be called.  Primary plane cleanup
> +	 * happens during CRTC destruction.
> +	 */
> +	BUG();

Just leave the func pointer as NULL, you get a backtrace either way.

> +}
> +
> +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 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_plane;
>  	int i;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
>  	if (intel_crtc == NULL)
>  		return;
>  
> +	primary_plane = kzalloc(sizeof(*primary_plane), GFP_KERNEL);
> +	if (primary_plane == NULL) {
> +		kfree(intel_crtc);
> +		return;
> +	}
> +
>  	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> +	drm_plane_set_primary(dev, primary_plane, &intel_crtc->base,
> +			      &intel_primary_plane_funcs, NULL, 0);
>  
>  	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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC