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

Commit Message

Philipp Zabel May 19, 2015, 4:06 p.m. UTC

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 <[e-mail]>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 72 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/imx/ipuv3-plane.h |  2 ++
 2 files changed, 74 insertions(+)

Comments

Daniel Vetter May 19, 2015, 4:58 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Philipp Zabel May 20, 2015, 10:51 a.m. UTC | #2

Am Dienstag, den 19.05.2015, 18:58 +0200 schrieb Daniel Vetter:
[...]
> > @@ -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

Ow, right. I'll have a look at this.

thanks
Philipp

Patch

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;
+		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,