Re: [PATCH 7/7] drm/imx: ipuv3-plane: add support for separate alpha planes

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 19, 2015 at 06:06:01PM +0200, Philipp Zabel wrote:
> The IPUv3 can read 8-bit alpha values from a separate plane buffer using a
> companion IDMAC channel driven by the Alpha Transparency Controller (ATC)
> for the graphics channels. The conditional read mechanism allows to reduce
> memory bandwidth by skipping reads of color data for completely transparent
> bursts.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/ipuv3-plane.c | 72 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/imx/ipuv3-plane.h |  2 ++
>  2 files changed, 74 insertions(+)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index d030990..ca10d55 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -41,6 +41,12 @@ static const uint32_t ipu_plane_formats[] = {
>  	DRM_FORMAT_YVYU,
>  	DRM_FORMAT_YUV420,
>  	DRM_FORMAT_YVU420,
> +	DRM_FORMAT_RGB565_A8,
> +	DRM_FORMAT_BGR565_A8,
> +	DRM_FORMAT_RGB888_A8,
> +	DRM_FORMAT_BGR888_A8,
> +	DRM_FORMAT_RGBX8888_A8,
> +	DRM_FORMAT_BGRX8888_A8,
>  };
>  
>  int ipu_plane_irq(struct ipu_plane *ipu_plane)
> @@ -71,6 +77,7 @@ int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
>  		       int x, int y)
>  {
>  	struct drm_gem_cma_object *cma_obj;
> +	unsigned long alpha_eba = 0;
>  	unsigned long eba;
>  	int active;
>  
> @@ -86,13 +93,36 @@ int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
>  	eba = cma_obj->paddr + fb->offsets[0] +
>  	      fb->pitches[0] * y + (fb->bits_per_pixel >> 3) * x;
>  
> +	switch (fb->pixel_format) {
> +	case DRM_FORMAT_RGB565_A8:
> +	case DRM_FORMAT_BGR565_A8:
> +	case DRM_FORMAT_RGB888_A8:
> +	case DRM_FORMAT_BGR888_A8:
> +	case DRM_FORMAT_RGBX8888_A8:
> +	case DRM_FORMAT_BGRX8888_A8:
> +		alpha_eba = cma_obj->paddr + fb->offsets[1] +
> +			    fb->pitches[1] * y + x;

You need to look at the 2nd cma_obj here, i.e. drm_fb_cma_get_gem_obj(fb, 1);

Yes, userspace is allowed to hand in non-matching. And given that you
you just reuse the cma helpers and don't reject framebuffers with
non-matching cma objects your current planar yuv support is also already
broken. Would be good if you could also patch modetest a bit to exercise
this ...
-Daniel

> +		break;
> +	}
> +
>  	if (ipu_plane->enabled) {
>  		active = ipu_idmac_get_current_buffer(ipu_plane->ipu_ch);
>  		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, !active, eba);
>  		ipu_idmac_select_buffer(ipu_plane->ipu_ch, !active);
> +		if (alpha_eba) {
> +			active = ipu_idmac_get_current_buffer(
> +						ipu_plane->alpha_ch);
> +			ipu_cpmem_set_buffer(ipu_plane->alpha_ch, !active,
> +					     alpha_eba);
> +			ipu_idmac_select_buffer(ipu_plane->alpha_ch, !active);
> +		}
>  	} else {
>  		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
>  		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 1, eba);
> +		if (alpha_eba) {
> +			ipu_cpmem_set_buffer(ipu_plane->alpha_ch, 0, alpha_eba);
> +			ipu_cpmem_set_buffer(ipu_plane->alpha_ch, 1, alpha_eba);
> +		}
>  	}
>  
>  	/* cache offsets for subsequent pageflips */
> @@ -163,6 +193,7 @@ int ipu_plane_mode_set(struct ipu_plane *ipu_plane, struct drm_crtc *crtc,
>  		return ipu_plane_set_base(ipu_plane, fb, src_x, src_y);
>  	}
>  
> +	ipu_plane->separate_alpha = false;
>  	switch (ipu_plane->dp_flow) {
>  	case IPU_DP_FLOW_SYNC_BG:
>  		ret = ipu_dp_setup_channel(ipu_plane->dp,
> @@ -183,6 +214,16 @@ int ipu_plane_mode_set(struct ipu_plane *ipu_plane, struct drm_crtc *crtc,
>  		ipu_dp_set_window_pos(ipu_plane->dp, crtc_x, crtc_y);
>  		/* Enable local alpha on partial plane */
>  		switch (fb->pixel_format) {
> +		case DRM_FORMAT_RGB565_A8:
> +		case DRM_FORMAT_BGR565_A8:
> +		case DRM_FORMAT_RGB888_A8:
> +		case DRM_FORMAT_BGR888_A8:
> +		case DRM_FORMAT_RGBX8888_A8:
> +		case DRM_FORMAT_BGRX8888_A8:
> +			if (!ipu_plane->alpha_ch)
> +				return -EINVAL;
> +			ipu_plane->separate_alpha = true;
> +			/* fallthrough */
>  		case DRM_FORMAT_ARGB1555:
>  		case DRM_FORMAT_ABGR1555:
>  		case DRM_FORMAT_RGBA5551:
> @@ -224,6 +265,18 @@ int ipu_plane_mode_set(struct ipu_plane *ipu_plane, struct drm_crtc *crtc,
>  	ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
>  	ipu_cpmem_set_stride(ipu_plane->ipu_ch, fb->pitches[0]);
>  
> +	if (ipu_plane->separate_alpha) {
> +		ipu_cpmem_set_burstsize(ipu_plane->ipu_ch, 16);
> +
> +		ipu_cpmem_zero(ipu_plane->alpha_ch);
> +		ipu_cpmem_set_resolution(ipu_plane->alpha_ch, src_w, src_h);
> +		ipu_cpmem_set_format_passthrough(ipu_plane->alpha_ch, 8);
> +		ipu_cpmem_set_high_priority(ipu_plane->alpha_ch);
> +		ipu_idmac_set_double_buffer(ipu_plane->alpha_ch, 1);
> +		ipu_cpmem_set_stride(ipu_plane->alpha_ch, fb->pitches[1]);
> +		ipu_cpmem_set_burstsize(ipu_plane->alpha_ch, 16);
> +	}
> +
>  	ret = ipu_plane_set_base(ipu_plane, fb, src_x, src_y);
>  	if (ret < 0)
>  		return ret;
> @@ -244,11 +297,14 @@ void ipu_plane_put_resources(struct ipu_plane *ipu_plane)
>  		ipu_dmfc_put(ipu_plane->dmfc);
>  	if (!IS_ERR_OR_NULL(ipu_plane->ipu_ch))
>  		ipu_idmac_put(ipu_plane->ipu_ch);
> +	if (!IS_ERR_OR_NULL(ipu_plane->alpha_ch))
> +		ipu_idmac_put(ipu_plane->alpha_ch);
>  }
>  
>  int ipu_plane_get_resources(struct ipu_plane *ipu_plane)
>  {
>  	int ret;
> +	int alpha_ch;
>  
>  	ipu_plane->ipu_ch = ipu_idmac_get(ipu_plane->ipu, ipu_plane->dma);
>  	if (IS_ERR(ipu_plane->ipu_ch)) {
> @@ -257,6 +313,18 @@ int ipu_plane_get_resources(struct ipu_plane *ipu_plane)
>  		return ret;
>  	}
>  
> +	alpha_ch = ipu_channel_alpha_channel(ipu_plane->dma);
> +	if (alpha_ch >= 0) {
> +		ipu_plane->alpha_ch = ipu_idmac_get(ipu_plane->ipu, alpha_ch);
> +		if (IS_ERR(ipu_plane->alpha_ch)) {
> +			ipu_idmac_put(ipu_plane->ipu_ch);
> +			ret = PTR_ERR(ipu_plane->alpha_ch);
> +			DRM_ERROR("failed to get alpha idmac channel %d: %d\n",
> +				  alpha_ch, ret);
> +			return ret;
> +		}
> +	}
> +
>  	ipu_plane->dmfc = ipu_dmfc_get(ipu_plane->ipu, ipu_plane->dma);
>  	if (IS_ERR(ipu_plane->dmfc)) {
>  		ret = PTR_ERR(ipu_plane->dmfc);
> @@ -286,6 +354,8 @@ void ipu_plane_enable(struct ipu_plane *ipu_plane)
>  		ipu_dp_enable(ipu_plane->ipu);
>  	ipu_dmfc_enable_channel(ipu_plane->dmfc);
>  	ipu_idmac_enable_channel(ipu_plane->ipu_ch);
> +	if (ipu_plane->separate_alpha)
> +		ipu_idmac_enable_channel(ipu_plane->alpha_ch);
>  	if (ipu_plane->dp)
>  		ipu_dp_enable_channel(ipu_plane->dp);
>  
> @@ -300,6 +370,8 @@ void ipu_plane_disable(struct ipu_plane *ipu_plane)
>  
>  	if (ipu_plane->dp)
>  		ipu_dp_disable_channel(ipu_plane->dp);
> +	if (ipu_plane->separate_alpha)
> +		ipu_idmac_disable_channel(ipu_plane->alpha_ch);
>  	ipu_idmac_disable_channel(ipu_plane->ipu_ch);
>  	ipu_dmfc_disable_channel(ipu_plane->dmfc);
>  	if (ipu_plane->dp)
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.h b/drivers/gpu/drm/imx/ipuv3-plane.h
> index 9b5eff1..c8913ed 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.h
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.h
> @@ -18,6 +18,7 @@ struct ipu_plane {
>  
>  	struct ipu_soc		*ipu;
>  	struct ipuv3_channel	*ipu_ch;
> +	struct ipuv3_channel	*alpha_ch;
>  	struct dmfc_channel	*dmfc;
>  	struct ipu_dp		*dp;
>  
> @@ -30,6 +31,7 @@ struct ipu_plane {
>  	int			h;
>  
>  	bool			enabled;
> +	bool			separate_alpha;
>  };
>  
>  struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel