[PATCH 0/4] IPu tiled scanout support

[PATCH 0/4] IPu tiled scanout support

[Lucas Stach]

This adds the required bits to the ipu-v3 and imx-drm driver to make
scanout of the Vivante (super-)tiled single buffer format work on
i.MX6 QuadPlus. A userspace taking advantage of this support is able
to skip the linear resolve step in the GPU, saving quite a lot of memory
bandwidth especially with higher resolution displays.

The first 3 patches can go in as-is, while the last patch depends on
Ben Widawsky's series to add a format modifier blob property to the
DRM planes.

Regards,
Lucas

Lucas Stach (4):
  gpu: ipu-v3: pre: add tiled prefetch support
  gpu: ipu-v3: prg: add modifier support
  drm/imx: add FB modifier support
  drm/imx: advertise supported plane format modifiers

 drivers/gpu/drm/imx/imx-drm-core.c |  1 +
 drivers/gpu/drm/imx/ipuv3-plane.c  | 92 +++++++++++++++++++++++++++++++++++---
 drivers/gpu/ipu-v3/ipu-pre.c       | 24 +++++++++-
 drivers/gpu/ipu-v3/ipu-prg.c       | 13 ++++--
 drivers/gpu/ipu-v3/ipu-prv.h       |  4 +-
 include/video/imx-ipu-v3.h         |  2 +-
 6 files changed, 122 insertions(+), 14 deletions(-)

-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] gpu: ipu-v3: pre: add tiled prefetch support

[Lucas Stach]

This configures the TPR unit, using the DRM format modifier. For now only
the single buffer modifiers are supported, as split buffer needs more
configuration for the required cropping.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/ipu-v3/ipu-pre.c | 24 ++++++++++++++++++++++--
 drivers/gpu/ipu-v3/ipu-prg.c |  2 +-
 drivers/gpu/ipu-v3/ipu-prv.h |  4 ++--
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-pre.c b/drivers/gpu/ipu-v3/ipu-pre.c
index c55563379e2e..d895f92d8d33 100644
--- a/drivers/gpu/ipu-v3/ipu-pre.c
+++ b/drivers/gpu/ipu-v3/ipu-pre.c
@@ -49,6 +49,10 @@
 #define IPU_PRE_TPR_CTRL				0x070
 #define  IPU_PRE_TPR_CTRL_TILE_FORMAT(v)		((v & 0xff) << 0)
 #define  IPU_PRE_TPR_CTRL_TILE_FORMAT_MASK		0xff
+#define  IPU_PRE_TPR_CTRL_TILE_FORMAT_16_BIT		(1 << 0)
+#define  IPU_PRE_TPR_CTRL_TILE_FORMAT_SPLIT_BUF		(1 << 4)
+#define  IPU_PRE_TPR_CTRL_TILE_FORMAT_SINGLE_BUF	(1 << 5)
+#define  IPU_PRE_TPR_CTRL_TILE_FORMAT_SUPER_TILED	(1 << 6)
 
 #define IPU_PRE_PREFETCH_ENG_CTRL			0x080
 #define  IPU_PRE_PREF_ENG_CTRL_PREFETCH_EN		(1 << 0)
@@ -140,7 +144,7 @@ int ipu_pre_get(struct ipu_pre *pre)
 	val = IPU_PRE_CTRL_HANDSHAKE_ABORT_SKIP_EN |
 	      IPU_PRE_CTRL_HANDSHAKE_EN |
 	      IPU_PRE_CTRL_TPR_REST_SEL |
-	      IPU_PRE_CTRL_BLOCK_16 | IPU_PRE_CTRL_SDW_UPDATE;
+	      IPU_PRE_CTRL_SDW_UPDATE;
 	writel(val, pre->regs + IPU_PRE_CTRL);
 
 	pre->in_use = true;
@@ -161,7 +165,7 @@ void ipu_pre_put(struct ipu_pre *pre)
 
 void ipu_pre_configure(struct ipu_pre *pre, unsigned int width,
 		       unsigned int height, unsigned int stride, u32 format,
-		       unsigned int bufaddr)
+		       uint64_t modifier, unsigned int bufaddr)
 {
 	const struct drm_format_info *info = drm_format_info(format);
 	u32 active_bpp = info->cpp[0] >> 1;
@@ -198,9 +202,25 @@ void ipu_pre_configure(struct ipu_pre *pre, unsigned int width,
 
 	writel(pre->buffer_paddr, pre->regs + IPU_PRE_STORE_ENG_ADDR);
 
+	val = readl(pre->regs + IPU_PRE_TPR_CTRL);
+	val &= ~IPU_PRE_TPR_CTRL_TILE_FORMAT_MASK;
+	if (modifier != DRM_FORMAT_MOD_LINEAR) {
+		/* only support single buffer formats for now */
+		val |= IPU_PRE_TPR_CTRL_TILE_FORMAT_SINGLE_BUF;
+		if (modifier == DRM_FORMAT_MOD_VIVANTE_SUPER_TILED)
+			val |= IPU_PRE_TPR_CTRL_TILE_FORMAT_SUPER_TILED;
+		if (info->cpp[0] == 2)
+			val |= IPU_PRE_TPR_CTRL_TILE_FORMAT_16_BIT;
+	}
+	writel(val, pre->regs + IPU_PRE_TPR_CTRL);
+
 	val = readl(pre->regs + IPU_PRE_CTRL);
 	val |= IPU_PRE_CTRL_EN_REPEAT | IPU_PRE_CTRL_ENABLE |
 	       IPU_PRE_CTRL_SDW_UPDATE;
+	if (modifier == DRM_FORMAT_MOD_LINEAR)
+		val &= ~IPU_PRE_CTRL_BLOCK_EN;
+	else
+		val |= IPU_PRE_CTRL_BLOCK_EN;
 	writel(val, pre->regs + IPU_PRE_CTRL);
 }
 
diff --git a/drivers/gpu/ipu-v3/ipu-prg.c b/drivers/gpu/ipu-v3/ipu-prg.c
index ecc9ea44dc50..454b1f1eb5df 100644
--- a/drivers/gpu/ipu-v3/ipu-prg.c
+++ b/drivers/gpu/ipu-v3/ipu-prg.c
@@ -295,7 +295,7 @@ int ipu_prg_channel_configure(struct ipuv3_channel *ipu_chan,
 		return ret;
 
 	ipu_pre_configure(prg->pres[chan->used_pre],
-			  width, height, stride, format, *eba);
+			  width, height, stride, format, 0, *eba);
 
 
 	ret = clk_prepare_enable(prg->clk_ipg);
diff --git a/drivers/gpu/ipu-v3/ipu-prv.h b/drivers/gpu/ipu-v3/ipu-prv.h
index ca2a223a0d1e..11be49de2eba 100644
--- a/drivers/gpu/ipu-v3/ipu-prv.h
+++ b/drivers/gpu/ipu-v3/ipu-prv.h
@@ -274,8 +274,8 @@ int ipu_pre_get(struct ipu_pre *pre);
 void ipu_pre_put(struct ipu_pre *pre);
 u32 ipu_pre_get_baddr(struct ipu_pre *pre);
 void ipu_pre_configure(struct ipu_pre *pre, unsigned int width,
-		       unsigned int height,
-		       unsigned int stride, u32 format, unsigned int bufaddr);
+		       unsigned int height, unsigned int stride, u32 format,
+		       uint64_t modifier, unsigned int bufaddr);
 void ipu_pre_update(struct ipu_pre *pre, unsigned int bufaddr);
 
 struct ipu_prg *ipu_prg_lookup_by_phandle(struct device *dev, const char *name,
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] gpu: ipu-v3: prg: add modifier support

[Lucas Stach]

Allow to pass through the modifier to the PRE unit and extend the
format check with the supported modifiers.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/ipuv3-plane.c |  4 +++-
 drivers/gpu/ipu-v3/ipu-prg.c      | 13 ++++++++++---
 include/video/imx-ipu-v3.h        |  2 +-
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 6c708c3b1cdc..705ca93847ff 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -526,7 +526,9 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 					  drm_rect_width(&state->src) >> 16,
 					  drm_rect_height(&state->src) >> 16,
 					  state->fb->pitches[0],
-					  state->fb->format->format, &eba);
+					  state->fb->format->format,
+					  0,
+					  &eba);
 	}
 
 	if (old_state->fb && !drm_atomic_crtc_needs_modeset(crtc_state)) {
diff --git a/drivers/gpu/ipu-v3/ipu-prg.c b/drivers/gpu/ipu-v3/ipu-prg.c
index 454b1f1eb5df..22d36dba09dc 100644
--- a/drivers/gpu/ipu-v3/ipu-prg.c
+++ b/drivers/gpu/ipu-v3/ipu-prg.c
@@ -131,7 +131,14 @@ bool ipu_prg_format_supported(struct ipu_soc *ipu, uint32_t format,
 	if (info->num_planes != 1)
 		return false;
 
-	return true;
+	switch (modifier) {
+	case DRM_FORMAT_MOD_LINEAR:
+	case DRM_FORMAT_MOD_VIVANTE_TILED:
+	case DRM_FORMAT_MOD_VIVANTE_SUPER_TILED:
+		return true;
+	default:
+		return false;
+	}
 }
 EXPORT_SYMBOL_GPL(ipu_prg_format_supported);
 
@@ -274,7 +281,7 @@ EXPORT_SYMBOL_GPL(ipu_prg_channel_disable);
 int ipu_prg_channel_configure(struct ipuv3_channel *ipu_chan,
 			      unsigned int axi_id, unsigned int width,
 			      unsigned int height, unsigned int stride,
-			      u32 format, unsigned long *eba)
+			      u32 format, uint64_t modifier, unsigned long *eba)
 {
 	int prg_chan = ipu_prg_ipu_to_prg_chan(ipu_chan->num);
 	struct ipu_prg *prg = ipu_chan->ipu->prg_priv;
@@ -295,7 +302,7 @@ int ipu_prg_channel_configure(struct ipuv3_channel *ipu_chan,
 		return ret;
 
 	ipu_pre_configure(prg->pres[chan->used_pre],
-			  width, height, stride, format, 0, *eba);
+			  width, height, stride, format, modifier, *eba);
 
 
 	ret = clk_prepare_enable(prg->clk_ipg);
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index 8cb07680fb41..755728cf58d8 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -343,7 +343,7 @@ void ipu_prg_channel_disable(struct ipuv3_channel *ipu_chan);
 int ipu_prg_channel_configure(struct ipuv3_channel *ipu_chan,
 			      unsigned int axi_id,  unsigned int width,
 			      unsigned int height, unsigned int stride,
-			      u32 format, unsigned long *eba);
+			      u32 format, uint64_t modifier, unsigned long *eba);
 
 /*
  * IPU CMOS Sensor Interface (csi) functions
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/imx: add FB modifier support

[Lucas Stach]

This adds FB modifier support for the Vivante single buffer tiled formats,
when the PRG/PRE engines are present.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-drm-core.c |  1 +
 drivers/gpu/drm/imx/ipuv3-plane.c  | 55 ++++++++++++++++++++++++++++++++++----
 2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 50add2f9e250..d21e7db95979 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -266,6 +266,7 @@ static int imx_drm_bind(struct device *dev)
 	drm->mode_config.max_height = 4096;
 	drm->mode_config.funcs = &imx_drm_mode_config_funcs;
 	drm->mode_config.helper_private = &imx_drm_mode_config_helpers;
+	drm->mode_config.allow_fb_modifiers = true;
 
 	drm_mode_config_init(drm);
 
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 705ca93847ff..44593dd6ba31 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -527,7 +527,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 					  drm_rect_height(&state->src) >> 16,
 					  state->fb->pitches[0],
 					  state->fb->format->format,
-					  0,
+					  state->fb->modifier,
 					  &eba);
 	}
 
@@ -673,17 +673,62 @@ int ipu_planes_assign_pre(struct drm_device *dev,
 			  struct drm_atomic_state *state)
 {
 	struct drm_plane_state *plane_state;
+	struct ipu_plane_state *ipu_state;
+	struct ipu_plane *ipu_plane;
 	struct drm_plane *plane;
 	int available_pres = ipu_prg_max_active_channels();
 	int i;
 
+	/*
+	 * We are going over the planes in 2 passes: first we assign PREs to
+	 * planes with a tiling modifier, which need the PREs to resolve into
+	 * linear. Any failure to assign a PRE there is fatal. In the second
+	 * pass we try to assign PREs to linear FBs, to improve memory access
+	 * patterns for them. Failure at this point is non-fatal, as we can
+	 * scan out linear FBs without a PRE.
+	 */
 	for_each_plane_in_state(state, plane, plane_state, i) {
-		struct ipu_plane_state *ipu_state =
-				to_ipu_plane_state(plane_state);
-		struct ipu_plane *ipu_plane = to_ipu_plane(plane);
+		ipu_state = to_ipu_plane_state(plane_state);
+		ipu_plane = to_ipu_plane(plane);
+
+		if (!plane_state->fb) {
+			ipu_state->use_pre = false;
+			continue;
+		}
+
+		if (!(plane_state->fb->flags & DRM_MODE_FB_MODIFIERS) ||
+		    plane_state->fb->modifier == DRM_FORMAT_MOD_LINEAR)
+			continue;
+
+		if (!ipu_prg_present(ipu_plane->ipu) || !available_pres)
+			return -EINVAL;
+
+		if (!ipu_prg_format_supported(ipu_plane->ipu,
+					      plane_state->fb->format->format,
+					      plane_state->fb->modifier))
+			return -EINVAL;
+
+		ipu_state->use_pre = true;
+		available_pres--;
+	}
+
+	for_each_plane_in_state(state, plane, plane_state, i) {
+		ipu_state = to_ipu_plane_state(plane_state);
+		ipu_plane = to_ipu_plane(plane);
+
+		if (!plane_state->fb) {
+			ipu_state->use_pre = false;
+			continue;
+		}
+
+		if ((plane_state->fb->flags & DRM_MODE_FB_MODIFIERS) &&
+		    plane_state->fb->modifier != DRM_FORMAT_MOD_LINEAR)
+			continue;
+
+		/* make sure that modifier is initialized */
+		plane_state->fb->modifier = DRM_FORMAT_MOD_LINEAR;
 
 		if (ipu_prg_present(ipu_plane->ipu) && available_pres &&
-		    plane_state->fb &&
 		    ipu_prg_format_supported(ipu_plane->ipu,
 					     plane_state->fb->format->format,
 					     plane_state->fb->modifier)) {
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/imx: advertise supported plane format modifiers

[Lucas Stach]

Let userspace know about the supported modifiers, to make automatic
selection of a proper modifier possible.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 44593dd6ba31..8b54fcb4ae5c 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -76,6 +76,18 @@ static const uint32_t ipu_plane_formats[] = {
 	DRM_FORMAT_BGRX8888_A8,
 };
 
+static const uint64_t ipu_format_modifiers[] = {
+	DRM_FORMAT_MOD_LINEAR,
+	DRM_FORMAT_MOD_INVALID
+};
+
+static const uint64_t pre_format_modifiers[] = {
+	DRM_FORMAT_MOD_LINEAR,
+	DRM_FORMAT_MOD_VIVANTE_TILED,
+	DRM_FORMAT_MOD_VIVANTE_SUPER_TILED,
+	DRM_FORMAT_MOD_INVALID
+};
+
 int ipu_plane_irq(struct ipu_plane *ipu_plane)
 {
 	return ipu_idmac_channel_irq(ipu_plane->ipu, ipu_plane->ipu_ch,
@@ -302,6 +314,22 @@ void ipu_plane_destroy_state(struct drm_plane *plane,
 	kfree(ipu_state);
 }
 
+static bool ipu_plane_format_mod_supported(struct drm_plane *plane,
+					   uint32_t format, uint64_t modifier)
+{
+	struct ipu_soc *ipu = to_ipu_plane(plane)->ipu;
+
+	/* linear is supported for all planes and formats */
+	if (modifier == DRM_FORMAT_MOD_LINEAR)
+		return true;
+
+	/* without a PRG there are no supported modifiers */
+	if (!ipu_prg_present(ipu))
+		return false;
+
+	return ipu_prg_format_supported(ipu, format, modifier);
+}
+
 static const struct drm_plane_funcs ipu_plane_funcs = {
 	.update_plane	= drm_atomic_helper_update_plane,
 	.disable_plane	= drm_atomic_helper_disable_plane,
@@ -309,6 +337,7 @@ static const struct drm_plane_funcs ipu_plane_funcs = {
 	.reset		= ipu_plane_state_reset,
 	.atomic_duplicate_state	= ipu_plane_duplicate_state,
 	.atomic_destroy_state	= ipu_plane_destroy_state,
+	.format_mod_supported = ipu_plane_format_mod_supported,
 };
 
 static int ipu_plane_atomic_check(struct drm_plane *plane,
@@ -748,6 +777,7 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
 				 enum drm_plane_type type)
 {
 	struct ipu_plane *ipu_plane;
+	const uint64_t *modifiers = ipu_format_modifiers;
 	int ret;
 
 	DRM_DEBUG_KMS("channel %d, dp flow %d, possible_crtcs=0x%x\n",
@@ -763,10 +793,13 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
 	ipu_plane->dma = dma;
 	ipu_plane->dp_flow = dp;
 
+	if (ipu_prg_present(ipu))
+		modifiers = pre_format_modifiers;
+
 	ret = drm_universal_plane_init(dev, &ipu_plane->base, possible_crtcs,
 				       &ipu_plane_funcs, ipu_plane_formats,
 				       ARRAY_SIZE(ipu_plane_formats),
-				       NULL, type, NULL);
+				       modifiers, type, NULL);
 	if (ret) {
 		DRM_ERROR("failed to initialize plane\n");
 		kfree(ipu_plane);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/imx: add FB modifier support

[Daniel Stone]

Hi Lucas,

On 3 May 2017 at 17:28, Lucas Stach <l.stach@pengutronix.de> wrote:
>         int available_pres = ipu_prg_max_active_channels();
>         int i;
>
> +       /*
> +        * We are going over the planes in 2 passes: first we assign PREs to
> +        * planes with a tiling modifier, which need the PREs to resolve into
> +        * linear. Any failure to assign a PRE there is fatal. In the second
> +        * pass we try to assign PREs to linear FBs, to improve memory access
> +        * patterns for them. Failure at this point is non-fatal, as we can
> +        * scan out linear FBs without a PRE.
> +        */
>         for_each_plane_in_state(state, plane, plane_state, i) {
> -               struct ipu_plane_state *ipu_state =
> -                               to_ipu_plane_state(plane_state);
> -               struct ipu_plane *ipu_plane = to_ipu_plane(plane);
> +               ipu_state = to_ipu_plane_state(plane_state);
> +               ipu_plane = to_ipu_plane(plane);
> +
> +               if (!plane_state->fb) {
> +                       ipu_state->use_pre = false;
> +                       continue;
> +               }
> +
> +               if (!(plane_state->fb->flags & DRM_MODE_FB_MODIFIERS) ||
> +                   plane_state->fb->modifier == DRM_FORMAT_MOD_LINEAR)
> +                       continue;
> +
> +               if (!ipu_prg_present(ipu_plane->ipu) || !available_pres)
> +                       return -EINVAL;

What about planes which aren't present in this commit, but are still
taking up a PRE unit? Will they have their PRE stolen, or am I missing
something?

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/imx: add FB modifier support

[Lucas Stach]

Am Mittwoch, den 03.05.2017, 18:15 +0100 schrieb Daniel Stone:
> Hi Lucas,
> 
> On 3 May 2017 at 17:28, Lucas Stach <l.stach@pengutronix.de> wrote:
> >         int available_pres = ipu_prg_max_active_channels();
> >         int i;
> >
> > +       /*
> > +        * We are going over the planes in 2 passes: first we assign PREs to
> > +        * planes with a tiling modifier, which need the PREs to resolve into
> > +        * linear. Any failure to assign a PRE there is fatal. In the second
> > +        * pass we try to assign PREs to linear FBs, to improve memory access
> > +        * patterns for them. Failure at this point is non-fatal, as we can
> > +        * scan out linear FBs without a PRE.
> > +        */
> >         for_each_plane_in_state(state, plane, plane_state, i) {
> > -               struct ipu_plane_state *ipu_state =
> > -                               to_ipu_plane_state(plane_state);
> > -               struct ipu_plane *ipu_plane = to_ipu_plane(plane);
> > +               ipu_state = to_ipu_plane_state(plane_state);
> > +               ipu_plane = to_ipu_plane(plane);
> > +
> > +               if (!plane_state->fb) {
> > +                       ipu_state->use_pre = false;
> > +                       continue;
> > +               }
> > +
> > +               if (!(plane_state->fb->flags & DRM_MODE_FB_MODIFIERS) ||
> > +                   plane_state->fb->modifier == DRM_FORMAT_MOD_LINEAR)
> > +                       continue;
> > +
> > +               if (!ipu_prg_present(ipu_plane->ipu) || !available_pres)
> > +                       return -EINVAL;
> 
> What about planes which aren't present in this commit, but are still
> taking up a PRE unit? Will they have their PRE stolen, or am I missing
> something?

Yes, the plane->PRE assignment is configurable by matching different AXI
IDs in the PRG. So what's happening here is that we basically construct
a new assignment for each commit. Planes without a assigned PRE will
revert back to pass-through mode in the PRG on plane commit or plane
disable.

Regards,
Lucas

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/imx: add FB modifier support

[Daniel Stone]

Hi,

On 4 May 2017 at 09:59, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Mittwoch, den 03.05.2017, 18:15 +0100 schrieb Daniel Stone:
>> What about planes which aren't present in this commit, but are still
>> taking up a PRE unit? Will they have their PRE stolen, or am I missing
>> something?
>
> Yes, the plane->PRE assignment is configurable by matching different AXI
> IDs in the PRG. So what's happening here is that we basically construct
> a new assignment for each commit. Planes without a assigned PRE will
> revert back to pass-through mode in the PRG on plane commit or plane
> disable.

So those other planes are fine/untouched, because the PRE has already
resolved to linear? Specifically if plane A has a tiled FB, then
userspace does a commit which _only_ contains plane B (also with a
tiled FB), and plane B steals the PRE that was previously assigned to
plane A, then plane A continues displaying just fine? Sorry for the
stupid questions. :)

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/imx: add FB modifier support

[Lucas Stach]

Am Donnerstag, den 04.05.2017, 10:02 +0100 schrieb Daniel Stone:
> Hi,
> 
> On 4 May 2017 at 09:59, Lucas Stach <l.stach@pengutronix.de> wrote:
> > Am Mittwoch, den 03.05.2017, 18:15 +0100 schrieb Daniel Stone:
> >> What about planes which aren't present in this commit, but are still
> >> taking up a PRE unit? Will they have their PRE stolen, or am I missing
> >> something?
> >
> > Yes, the plane->PRE assignment is configurable by matching different AXI
> > IDs in the PRG. So what's happening here is that we basically construct
> > a new assignment for each commit. Planes without a assigned PRE will
> > revert back to pass-through mode in the PRG on plane commit or plane
> > disable.
> 
> So those other planes are fine/untouched, because the PRE has already
> resolved to linear? Specifically if plane A has a tiled FB, then
> userspace does a commit which _only_ contains plane B (also with a
> tiled FB), and plane B steals the PRE that was previously assigned to
> plane A, then plane A continues displaying just fine? Sorry for the
> stupid questions. :)

If userspace does a commit with only plane B it's going to disable plane
A, right? The PRG/PRE register sets are double buffered (or at least we
always use them in this mode). So if plane B steals the PRE from plane
A, which is going to get disabled, plane A will continue to use the PRE
until the end of frame, then the register set will be latched and the
PRE is switched to plane B for the next frame.

Regards,
Lucas

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/imx: add FB modifier support

[Daniel Vetter]

On Thu, May 4, 2017 at 11:12 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Donnerstag, den 04.05.2017, 10:02 +0100 schrieb Daniel Stone:
>> On 4 May 2017 at 09:59, Lucas Stach <l.stach@pengutronix.de> wrote:
>> > Am Mittwoch, den 03.05.2017, 18:15 +0100 schrieb Daniel Stone:
>> >> What about planes which aren't present in this commit, but are still
>> >> taking up a PRE unit? Will they have their PRE stolen, or am I missing
>> >> something?
>> >
>> > Yes, the plane->PRE assignment is configurable by matching different AXI
>> > IDs in the PRG. So what's happening here is that we basically construct
>> > a new assignment for each commit. Planes without a assigned PRE will
>> > revert back to pass-through mode in the PRG on plane commit or plane
>> > disable.
>>
>> So those other planes are fine/untouched, because the PRE has already
>> resolved to linear? Specifically if plane A has a tiled FB, then
>> userspace does a commit which _only_ contains plane B (also with a
>> tiled FB), and plane B steals the PRE that was previously assigned to
>> plane A, then plane A continues displaying just fine? Sorry for the
>> stupid questions. :)
>
> If userspace does a commit with only plane B it's going to disable plane
> A, right? The PRG/PRE register sets are double buffered (or at least we
> always use them in this mode). So if plane B steals the PRE from plane
> A, which is going to get disabled, plane A will continue to use the PRE
> until the end of frame, then the register set will be latched and the
> PRE is switched to plane B for the next frame.

Atomic is incremental, if a property is unchanged it's supposed to
keep as-is. Same with entire objects.

What's more, did you wire up the fb->dirty hook to make sure this
stuff gets flushed correctly for frontbuffer rendering? Userspace is
allowed to render into the current frontbuffer using gl, call all the
flush/clear stuff and then call the DIRTY_FB ioctl, and things are
supposed to show up on the screen. There's some ideas from the simple
pipe helpers to essentially unify frontbuffer flushing and treat it
like any atomic commit.

tldr;  you need a PRE for all planes, you're looking for
drm_atomic_add_affected_planes() to make sure all planes are in the
update.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/imx: add FB modifier support

[Lucas Stach]

Am Donnerstag, den 04.05.2017, 11:17 +0200 schrieb Daniel Vetter:
> On Thu, May 4, 2017 at 11:12 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > Am Donnerstag, den 04.05.2017, 10:02 +0100 schrieb Daniel Stone:
> >> On 4 May 2017 at 09:59, Lucas Stach <l.stach@pengutronix.de> wrote:
> >> > Am Mittwoch, den 03.05.2017, 18:15 +0100 schrieb Daniel Stone:
> >> >> What about planes which aren't present in this commit, but are still
> >> >> taking up a PRE unit? Will they have their PRE stolen, or am I missing
> >> >> something?
> >> >
> >> > Yes, the plane->PRE assignment is configurable by matching different AXI
> >> > IDs in the PRG. So what's happening here is that we basically construct
> >> > a new assignment for each commit. Planes without a assigned PRE will
> >> > revert back to pass-through mode in the PRG on plane commit or plane
> >> > disable.
> >>
> >> So those other planes are fine/untouched, because the PRE has already
> >> resolved to linear? Specifically if plane A has a tiled FB, then
> >> userspace does a commit which _only_ contains plane B (also with a
> >> tiled FB), and plane B steals the PRE that was previously assigned to
> >> plane A, then plane A continues displaying just fine? Sorry for the
> >> stupid questions. :)
> >
> > If userspace does a commit with only plane B it's going to disable plane
> > A, right? The PRG/PRE register sets are double buffered (or at least we
> > always use them in this mode). So if plane B steals the PRE from plane
> > A, which is going to get disabled, plane A will continue to use the PRE
> > until the end of frame, then the register set will be latched and the
> > PRE is switched to plane B for the next frame.
> 
> Atomic is incremental, if a property is unchanged it's supposed to
> keep as-is. Same with entire objects.
> 
> What's more, did you wire up the fb->dirty hook to make sure this
> stuff gets flushed correctly for frontbuffer rendering? Userspace is
> allowed to render into the current frontbuffer using gl, call all the
> flush/clear stuff and then call the DIRTY_FB ioctl, and things are
> supposed to show up on the screen. There's some ideas from the simple
> pipe helpers to essentially unify frontbuffer flushing and treat it
> like any atomic commit.

The PREs are stream processors, they are only prefetching (and
resolving) a few scanlines to internal SRAM. Any rendering to the
frontbuffer will properly show up without any additional dirty tracking.
You can think of the PREs as slightly bigger display FIFOs.

> tldr;  you need a PRE for all planes, you're looking for
> drm_atomic_add_affected_planes() to make sure all planes are in the
> update.

Thanks, I'll look at this.

Regards,
Lucas

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel