[PATCH 0/6] drm/vc4: Allow scaling on cursor planes

[PATCH 0/6] drm/vc4: Allow scaling on cursor planes

[Boris Brezillon]

Hello,

A few months back I tried to add support for the underscan props to the
VC4 driver [1] and this lead to the conclusion that, if we want to use
the HW scaler to implement underscan we should scale all planes by the
same factor, including the cursor plane which currently does not
support scaling.

This series addresses this limitation by allowing a cursor plane to be
scaled. When the scaling factors stay the same, async updates can take
place, when they change, the driver will fallback to sync updates.

I will submit a new version of the underscan patchset as soon as this
rework is accepted/merged.

Regards,

Boris

[1]https://patchwork.kernel.org/patch/10394563/

Boris Brezillon (6):
  drm/vc4: Make vc4_lbm_size() return 0 when vertical scaling is
    disabled
  drm/vc4: Move LBM creation out of vc4_plane_mode_set()
  drm/vc4: Don't check plane state more than once
  drm/vc4: Rework the async update logic
  drm/vc4: Allow scaling on cursor plane
  drm/vc4: Allow YUV formats on cursor planes

 drivers/gpu/drm/vc4/vc4_drv.h   |   6 +
 drivers/gpu/drm/vc4/vc4_plane.c | 223 ++++++++++++++++++++------------
 2 files changed, 148 insertions(+), 81 deletions(-)

-- 
2.17.1

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

[PATCH 1/6] drm/vc4: Make vc4_lbm_size() return 0 when vertical scaling is disabled

[Boris Brezillon]

LBM is not needed when vertical scaling is disabled. Return 0 in this
case to avoid allocating LBM memory that will anyway be unused.

While at it, drop the test on ->is_unity which is now redundant.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_plane.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index c3ded0ba0441..f6e3e8d33115 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -395,10 +395,13 @@ static u32 vc4_lbm_size(struct drm_plane_state *state)
 	u32 pix_per_line = max(vc4_state->src_w[0], (u32)vc4_state->crtc_w);
 	u32 lbm;
 
+	/* LBM is not needed when there's no vertical scaling. */
+	if (vc4_state->y_scaling[0] == VC4_SCALING_NONE &&
+	    vc4_state->y_scaling[1] == VC4_SCALING_NONE)
+		return 0;
+
 	if (!vc4_state->is_yuv) {
-		if (vc4_state->is_unity)
-			return 0;
-		else if (vc4_state->y_scaling[0] == VC4_SCALING_TPZ)
+		if (vc4_state->y_scaling[0] == VC4_SCALING_TPZ)
 			lbm = pix_per_line * 8;
 		else {
 			/* In special cases, this multiplier might be 12. */
-- 
2.17.1

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

Re: [PATCH 1/6] drm/vc4: Make vc4_lbm_size() return 0 when vertical scaling is disabled

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 267 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> LBM is not needed when vertical scaling is disabled. Return 0 in this
> case to avoid allocating LBM memory that will anyway be unused.
>
> While at it, drop the test on ->is_unity which is now redundant.

r-b

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 2/6] drm/vc4: Move LBM creation out of vc4_plane_mode_set()

[Boris Brezillon]

We are about to use vc4_plane_mode_set() in the async check path, and
async updates require that LBM size stay the same since they reuse the
LBM from the previous state. So we definitely don't want to allocate a
new LBM region that we know for sure will be free right away.

Move the LBM allocation out of vc4_plane_mode_set() and call the new
function (vc4_plane_update_lbm()) from vc4_plane_atomic_check().

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_drv.h   |  1 +
 drivers/gpu/drm/vc4/vc4_plane.c | 82 ++++++++++++++++++++++-----------
 2 files changed, 55 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index bd6ef1f31822..9ed05fb61eb6 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -338,6 +338,7 @@ struct vc4_plane_state {
 	u32 pos0_offset;
 	u32 pos2_offset;
 	u32 ptr0_offset;
+	u32 lbm_offset;
 
 	/* Offset where the plane's dlist was last stored in the
 	 * hardware at vc4_crtc_atomic_flush() time.
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index f6e3e8d33115..392a51f2bf8f 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -452,6 +452,43 @@ static void vc4_write_scaling_parameters(struct drm_plane_state *state,
 	}
 }
 
+static int vc4_plane_update_lbm(struct drm_plane_state *state)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(state->plane->dev);
+	struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
+	unsigned long irqflags;
+	u32 lbm_size;
+
+	lbm_size = vc4_lbm_size(state);
+	if (!lbm_size)
+		return 0;
+
+	if (WARN_ON(!vc4_state->lbm_offset))
+		return -EINVAL;
+
+	/* Allocate the LBM memory that the HVS will use for temporary
+	 * storage due to our scaling/format conversion.
+	 */
+	if (!vc4_state->lbm.allocated) {
+		int ret;
+
+		spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags);
+		ret = drm_mm_insert_node_generic(&vc4->hvs->lbm_mm,
+						 &vc4_state->lbm,
+						 lbm_size, 32, 0, 0);
+		spin_unlock_irqrestore(&vc4->hvs->mm_lock, irqflags);
+
+		if (ret)
+			return ret;
+	} else {
+		WARN_ON_ONCE(lbm_size != vc4_state->lbm.size);
+	}
+
+	vc4_state->dlist[vc4_state->lbm_offset] = vc4_state->lbm.start;
+
+	return 0;
+}
+
 /* Writes out a full display list for an active plane to the plane's
  * private dlist state.
  */
@@ -469,31 +506,12 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 	bool mix_plane_alpha;
 	bool covers_screen;
 	u32 scl0, scl1, pitch0;
-	u32 lbm_size, tiling;
-	unsigned long irqflags;
+	u32 tiling;
 	u32 hvs_format = format->hvs;
 	int ret, i;
 
-	ret = vc4_plane_setup_clipping_and_scaling(state);
-	if (ret)
-		return ret;
-
-	/* Allocate the LBM memory that the HVS will use for temporary
-	 * storage due to our scaling/format conversion.
-	 */
-	lbm_size = vc4_lbm_size(state);
-	if (lbm_size) {
-		if (!vc4_state->lbm.allocated) {
-			spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags);
-			ret = drm_mm_insert_node_generic(&vc4->hvs->lbm_mm,
-							 &vc4_state->lbm,
-							 lbm_size, 32, 0, 0);
-			spin_unlock_irqrestore(&vc4->hvs->mm_lock, irqflags);
-		} else {
-			WARN_ON_ONCE(lbm_size != vc4_state->lbm.size);
-		}
-	}
 
+	ret = vc4_plane_setup_clipping_and_scaling(state);
 	if (ret)
 		return ret;
 
@@ -717,15 +735,18 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 		vc4_dlist_write(vc4_state, SCALER_CSC2_ITR_R_601_5);
 	}
 
+	vc4_state->lbm_offset = 0;
+
 	if (vc4_state->x_scaling[0] != VC4_SCALING_NONE ||
 	    vc4_state->x_scaling[1] != VC4_SCALING_NONE ||
 	    vc4_state->y_scaling[0] != VC4_SCALING_NONE ||
 	    vc4_state->y_scaling[1] != VC4_SCALING_NONE) {
-		/* LBM Base Address. */
+		/* Reserve a slot for the LBM Base Address. The real value will
+		 * be set when calling vc4_plane_update_lbm().
+		 */
 		if (vc4_state->y_scaling[0] != VC4_SCALING_NONE ||
-		    vc4_state->y_scaling[1] != VC4_SCALING_NONE) {
-			vc4_dlist_write(vc4_state, vc4_state->lbm.start);
-		}
+		    vc4_state->y_scaling[1] != VC4_SCALING_NONE)
+			vc4_state->lbm_offset = vc4_state->dlist_count++;
 
 		if (num_planes > 1) {
 			/* Emit Cb/Cr as channel 0 and Y as channel
@@ -785,13 +806,18 @@ static int vc4_plane_atomic_check(struct drm_plane *plane,
 				  struct drm_plane_state *state)
 {
 	struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
+	int ret;
 
 	vc4_state->dlist_count = 0;
 
-	if (plane_enabled(state))
-		return vc4_plane_mode_set(plane, state);
-	else
+	if (!plane_enabled(state))
 		return 0;
+
+	ret = vc4_plane_mode_set(plane, state);
+	if (ret)
+		return ret;
+
+	return vc4_plane_update_lbm(state);
 }
 
 static void vc4_plane_atomic_update(struct drm_plane *plane,
-- 
2.17.1

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

Re: [PATCH 2/6] drm/vc4: Move LBM creation out of vc4_plane_mode_set()

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 5301 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> We are about to use vc4_plane_mode_set() in the async check path, and
> async updates require that LBM size stay the same since they reuse the
> LBM from the previous state. So we definitely don't want to allocate a
> new LBM region that we know for sure will be free right away.
>
> Move the LBM allocation out of vc4_plane_mode_set() and call the new
> function (vc4_plane_update_lbm()) from vc4_plane_atomic_check().
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_drv.h   |  1 +
>  drivers/gpu/drm/vc4/vc4_plane.c | 82 ++++++++++++++++++++++-----------
>  2 files changed, 55 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index bd6ef1f31822..9ed05fb61eb6 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -338,6 +338,7 @@ struct vc4_plane_state {
>  	u32 pos0_offset;
>  	u32 pos2_offset;
>  	u32 ptr0_offset;
> +	u32 lbm_offset;
>  
>  	/* Offset where the plane's dlist was last stored in the
>  	 * hardware at vc4_crtc_atomic_flush() time.
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index f6e3e8d33115..392a51f2bf8f 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -452,6 +452,43 @@ static void vc4_write_scaling_parameters(struct drm_plane_state *state,
>  	}
>  }
>  
> +static int vc4_plane_update_lbm(struct drm_plane_state *state)
> +{
> +	struct vc4_dev *vc4 = to_vc4_dev(state->plane->dev);
> +	struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
> +	unsigned long irqflags;
> +	u32 lbm_size;
> +
> +	lbm_size = vc4_lbm_size(state);
> +	if (!lbm_size)
> +		return 0;
> +
> +	if (WARN_ON(!vc4_state->lbm_offset))
> +		return -EINVAL;
> +
> +	/* Allocate the LBM memory that the HVS will use for temporary
> +	 * storage due to our scaling/format conversion.
> +	 */
> +	if (!vc4_state->lbm.allocated) {
> +		int ret;
> +
> +		spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags);
> +		ret = drm_mm_insert_node_generic(&vc4->hvs->lbm_mm,
> +						 &vc4_state->lbm,
> +						 lbm_size, 32, 0, 0);
> +		spin_unlock_irqrestore(&vc4->hvs->mm_lock, irqflags);
> +
> +		if (ret)
> +			return ret;
> +	} else {
> +		WARN_ON_ONCE(lbm_size != vc4_state->lbm.size);
> +	}
> +
> +	vc4_state->dlist[vc4_state->lbm_offset] = vc4_state->lbm.start;
> +
> +	return 0;
> +}
> +
>  /* Writes out a full display list for an active plane to the plane's
>   * private dlist state.
>   */
> @@ -469,31 +506,12 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
>  	bool mix_plane_alpha;
>  	bool covers_screen;
>  	u32 scl0, scl1, pitch0;
> -	u32 lbm_size, tiling;
> -	unsigned long irqflags;
> +	u32 tiling;
>  	u32 hvs_format = format->hvs;
>  	int ret, i;
>  
> -	ret = vc4_plane_setup_clipping_and_scaling(state);
> -	if (ret)
> -		return ret;
> -
> -	/* Allocate the LBM memory that the HVS will use for temporary
> -	 * storage due to our scaling/format conversion.
> -	 */
> -	lbm_size = vc4_lbm_size(state);
> -	if (lbm_size) {
> -		if (!vc4_state->lbm.allocated) {
> -			spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags);
> -			ret = drm_mm_insert_node_generic(&vc4->hvs->lbm_mm,
> -							 &vc4_state->lbm,
> -							 lbm_size, 32, 0, 0);
> -			spin_unlock_irqrestore(&vc4->hvs->mm_lock, irqflags);
> -		} else {
> -			WARN_ON_ONCE(lbm_size != vc4_state->lbm.size);
> -		}
> -	}
>  
> +	ret = vc4_plane_setup_clipping_and_scaling(state);
>  	if (ret)
>  		return ret;
>  
> @@ -717,15 +735,18 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
>  		vc4_dlist_write(vc4_state, SCALER_CSC2_ITR_R_601_5);
>  	}
>  
> +	vc4_state->lbm_offset = 0;
> +
>  	if (vc4_state->x_scaling[0] != VC4_SCALING_NONE ||
>  	    vc4_state->x_scaling[1] != VC4_SCALING_NONE ||
>  	    vc4_state->y_scaling[0] != VC4_SCALING_NONE ||
>  	    vc4_state->y_scaling[1] != VC4_SCALING_NONE) {
> -		/* LBM Base Address. */
> +		/* Reserve a slot for the LBM Base Address. The real value will
> +		 * be set when calling vc4_plane_update_lbm().
> +		 */
>  		if (vc4_state->y_scaling[0] != VC4_SCALING_NONE ||
> -		    vc4_state->y_scaling[1] != VC4_SCALING_NONE) {
> -			vc4_dlist_write(vc4_state, vc4_state->lbm.start);
> -		}
> +		    vc4_state->y_scaling[1] != VC4_SCALING_NONE)
> +			vc4_state->lbm_offset = vc4_state->dlist_count++;
>  
>  		if (num_planes > 1) {
>  			/* Emit Cb/Cr as channel 0 and Y as channel
> @@ -785,13 +806,18 @@ static int vc4_plane_atomic_check(struct drm_plane *plane,
>  				  struct drm_plane_state *state)
>  {
>  	struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
> +	int ret;
>  
>  	vc4_state->dlist_count = 0;
>  
> -	if (plane_enabled(state))
> -		return vc4_plane_mode_set(plane, state);
> -	else
> +	if (!plane_enabled(state))
>  		return 0;
> +
> +	ret = vc4_plane_mode_set(plane, state);
> +	if (ret)
> +		return ret;
> +
> +	return vc4_plane_update_lbm(state);
>  }

Couldn't you call update_lbm() (maybe called allocate_lbm()) first, and
then leave the dlist setup as is without needing lbm_offset?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 2/6] drm/vc4: Move LBM creation out of vc4_plane_mode_set()

[Boris Brezillon]

On Thu, 15 Nov 2018 12:39:42 -0800
Eric Anholt <eric@anholt.net> wrote:

> Boris Brezillon <boris.brezillon@bootlin.com> writes:
> 
> > We are about to use vc4_plane_mode_set() in the async check path, and
> > async updates require that LBM size stay the same since they reuse the
> > LBM from the previous state. So we definitely don't want to allocate a
> > new LBM region that we know for sure will be free right away.
> >
> > Move the LBM allocation out of vc4_plane_mode_set() and call the new
> > function (vc4_plane_update_lbm()) from vc4_plane_atomic_check().
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_drv.h   |  1 +
> >  drivers/gpu/drm/vc4/vc4_plane.c | 82 ++++++++++++++++++++++-----------
> >  2 files changed, 55 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index bd6ef1f31822..9ed05fb61eb6 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -338,6 +338,7 @@ struct vc4_plane_state {
> >  	u32 pos0_offset;
> >  	u32 pos2_offset;
> >  	u32 ptr0_offset;
> > +	u32 lbm_offset;
> >  
> >  	/* Offset where the plane's dlist was last stored in the
> >  	 * hardware at vc4_crtc_atomic_flush() time.
> > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > index f6e3e8d33115..392a51f2bf8f 100644
> > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > @@ -452,6 +452,43 @@ static void vc4_write_scaling_parameters(struct drm_plane_state *state,
> >  	}
> >  }
> >  
> > +static int vc4_plane_update_lbm(struct drm_plane_state *state)
> > +{
> > +	struct vc4_dev *vc4 = to_vc4_dev(state->plane->dev);
> > +	struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
> > +	unsigned long irqflags;
> > +	u32 lbm_size;
> > +
> > +	lbm_size = vc4_lbm_size(state);
> > +	if (!lbm_size)
> > +		return 0;
> > +
> > +	if (WARN_ON(!vc4_state->lbm_offset))
> > +		return -EINVAL;
> > +
> > +	/* Allocate the LBM memory that the HVS will use for temporary
> > +	 * storage due to our scaling/format conversion.
> > +	 */
> > +	if (!vc4_state->lbm.allocated) {
> > +		int ret;
> > +
> > +		spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags);
> > +		ret = drm_mm_insert_node_generic(&vc4->hvs->lbm_mm,
> > +						 &vc4_state->lbm,
> > +						 lbm_size, 32, 0, 0);
> > +		spin_unlock_irqrestore(&vc4->hvs->mm_lock, irqflags);
> > +
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		WARN_ON_ONCE(lbm_size != vc4_state->lbm.size);
> > +	}
> > +
> > +	vc4_state->dlist[vc4_state->lbm_offset] = vc4_state->lbm.start;
> > +
> > +	return 0;
> > +}
> > +
> >  /* Writes out a full display list for an active plane to the plane's
> >   * private dlist state.
> >   */
> > @@ -469,31 +506,12 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
> >  	bool mix_plane_alpha;
> >  	bool covers_screen;
> >  	u32 scl0, scl1, pitch0;
> > -	u32 lbm_size, tiling;
> > -	unsigned long irqflags;
> > +	u32 tiling;
> >  	u32 hvs_format = format->hvs;
> >  	int ret, i;
> >  
> > -	ret = vc4_plane_setup_clipping_and_scaling(state);
> > -	if (ret)
> > -		return ret;
> > -
> > -	/* Allocate the LBM memory that the HVS will use for temporary
> > -	 * storage due to our scaling/format conversion.
> > -	 */
> > -	lbm_size = vc4_lbm_size(state);
> > -	if (lbm_size) {
> > -		if (!vc4_state->lbm.allocated) {
> > -			spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags);
> > -			ret = drm_mm_insert_node_generic(&vc4->hvs->lbm_mm,
> > -							 &vc4_state->lbm,
> > -							 lbm_size, 32, 0, 0);
> > -			spin_unlock_irqrestore(&vc4->hvs->mm_lock, irqflags);
> > -		} else {
> > -			WARN_ON_ONCE(lbm_size != vc4_state->lbm.size);
> > -		}
> > -	}
> >  
> > +	ret = vc4_plane_setup_clipping_and_scaling(state);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -717,15 +735,18 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
> >  		vc4_dlist_write(vc4_state, SCALER_CSC2_ITR_R_601_5);
> >  	}
> >  
> > +	vc4_state->lbm_offset = 0;
> > +
> >  	if (vc4_state->x_scaling[0] != VC4_SCALING_NONE ||
> >  	    vc4_state->x_scaling[1] != VC4_SCALING_NONE ||
> >  	    vc4_state->y_scaling[0] != VC4_SCALING_NONE ||
> >  	    vc4_state->y_scaling[1] != VC4_SCALING_NONE) {
> > -		/* LBM Base Address. */
> > +		/* Reserve a slot for the LBM Base Address. The real value will
> > +		 * be set when calling vc4_plane_update_lbm().
> > +		 */
> >  		if (vc4_state->y_scaling[0] != VC4_SCALING_NONE ||
> > -		    vc4_state->y_scaling[1] != VC4_SCALING_NONE) {
> > -			vc4_dlist_write(vc4_state, vc4_state->lbm.start);
> > -		}
> > +		    vc4_state->y_scaling[1] != VC4_SCALING_NONE)
> > +			vc4_state->lbm_offset = vc4_state->dlist_count++;
> >  
> >  		if (num_planes > 1) {
> >  			/* Emit Cb/Cr as channel 0 and Y as channel
> > @@ -785,13 +806,18 @@ static int vc4_plane_atomic_check(struct drm_plane *plane,
> >  				  struct drm_plane_state *state)
> >  {
> >  	struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
> > +	int ret;
> >  
> >  	vc4_state->dlist_count = 0;
> >  
> > -	if (plane_enabled(state))
> > -		return vc4_plane_mode_set(plane, state);
> > -	else
> > +	if (!plane_enabled(state))
> >  		return 0;
> > +
> > +	ret = vc4_plane_mode_set(plane, state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return vc4_plane_update_lbm(state);
> >  }  
> 
> Couldn't you call update_lbm() (maybe called allocate_lbm()) first, and
> then leave the dlist setup as is without needing lbm_offset?

I'll anyway need lbm_offset for the dlist check I do in async_check().
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] drm/vc4: Move LBM creation out of vc4_plane_mode_set()

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 6048 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> On Thu, 15 Nov 2018 12:39:42 -0800
> Eric Anholt <eric@anholt.net> wrote:
>
>> Boris Brezillon <boris.brezillon@bootlin.com> writes:
>> 
>> > We are about to use vc4_plane_mode_set() in the async check path, and
>> > async updates require that LBM size stay the same since they reuse the
>> > LBM from the previous state. So we definitely don't want to allocate a
>> > new LBM region that we know for sure will be free right away.
>> >
>> > Move the LBM allocation out of vc4_plane_mode_set() and call the new
>> > function (vc4_plane_update_lbm()) from vc4_plane_atomic_check().
>> >
>> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
>> > ---
>> >  drivers/gpu/drm/vc4/vc4_drv.h   |  1 +
>> >  drivers/gpu/drm/vc4/vc4_plane.c | 82 ++++++++++++++++++++++-----------
>> >  2 files changed, 55 insertions(+), 28 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
>> > index bd6ef1f31822..9ed05fb61eb6 100644
>> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
>> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
>> > @@ -338,6 +338,7 @@ struct vc4_plane_state {
>> >  	u32 pos0_offset;
>> >  	u32 pos2_offset;
>> >  	u32 ptr0_offset;
>> > +	u32 lbm_offset;
>> >  
>> >  	/* Offset where the plane's dlist was last stored in the
>> >  	 * hardware at vc4_crtc_atomic_flush() time.
>> > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
>> > index f6e3e8d33115..392a51f2bf8f 100644
>> > --- a/drivers/gpu/drm/vc4/vc4_plane.c
>> > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
>> > @@ -452,6 +452,43 @@ static void vc4_write_scaling_parameters(struct drm_plane_state *state,
>> >  	}
>> >  }
>> >  
>> > +static int vc4_plane_update_lbm(struct drm_plane_state *state)
>> > +{
>> > +	struct vc4_dev *vc4 = to_vc4_dev(state->plane->dev);
>> > +	struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
>> > +	unsigned long irqflags;
>> > +	u32 lbm_size;
>> > +
>> > +	lbm_size = vc4_lbm_size(state);
>> > +	if (!lbm_size)
>> > +		return 0;
>> > +
>> > +	if (WARN_ON(!vc4_state->lbm_offset))
>> > +		return -EINVAL;
>> > +
>> > +	/* Allocate the LBM memory that the HVS will use for temporary
>> > +	 * storage due to our scaling/format conversion.
>> > +	 */
>> > +	if (!vc4_state->lbm.allocated) {
>> > +		int ret;
>> > +
>> > +		spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags);
>> > +		ret = drm_mm_insert_node_generic(&vc4->hvs->lbm_mm,
>> > +						 &vc4_state->lbm,
>> > +						 lbm_size, 32, 0, 0);
>> > +		spin_unlock_irqrestore(&vc4->hvs->mm_lock, irqflags);
>> > +
>> > +		if (ret)
>> > +			return ret;
>> > +	} else {
>> > +		WARN_ON_ONCE(lbm_size != vc4_state->lbm.size);
>> > +	}
>> > +
>> > +	vc4_state->dlist[vc4_state->lbm_offset] = vc4_state->lbm.start;
>> > +
>> > +	return 0;
>> > +}
>> > +
>> >  /* Writes out a full display list for an active plane to the plane's
>> >   * private dlist state.
>> >   */
>> > @@ -469,31 +506,12 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
>> >  	bool mix_plane_alpha;
>> >  	bool covers_screen;
>> >  	u32 scl0, scl1, pitch0;
>> > -	u32 lbm_size, tiling;
>> > -	unsigned long irqflags;
>> > +	u32 tiling;
>> >  	u32 hvs_format = format->hvs;
>> >  	int ret, i;
>> >  
>> > -	ret = vc4_plane_setup_clipping_and_scaling(state);
>> > -	if (ret)
>> > -		return ret;
>> > -
>> > -	/* Allocate the LBM memory that the HVS will use for temporary
>> > -	 * storage due to our scaling/format conversion.
>> > -	 */
>> > -	lbm_size = vc4_lbm_size(state);
>> > -	if (lbm_size) {
>> > -		if (!vc4_state->lbm.allocated) {
>> > -			spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags);
>> > -			ret = drm_mm_insert_node_generic(&vc4->hvs->lbm_mm,
>> > -							 &vc4_state->lbm,
>> > -							 lbm_size, 32, 0, 0);
>> > -			spin_unlock_irqrestore(&vc4->hvs->mm_lock, irqflags);
>> > -		} else {
>> > -			WARN_ON_ONCE(lbm_size != vc4_state->lbm.size);
>> > -		}
>> > -	}
>> >  
>> > +	ret = vc4_plane_setup_clipping_and_scaling(state);
>> >  	if (ret)
>> >  		return ret;
>> >  
>> > @@ -717,15 +735,18 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
>> >  		vc4_dlist_write(vc4_state, SCALER_CSC2_ITR_R_601_5);
>> >  	}
>> >  
>> > +	vc4_state->lbm_offset = 0;
>> > +
>> >  	if (vc4_state->x_scaling[0] != VC4_SCALING_NONE ||
>> >  	    vc4_state->x_scaling[1] != VC4_SCALING_NONE ||
>> >  	    vc4_state->y_scaling[0] != VC4_SCALING_NONE ||
>> >  	    vc4_state->y_scaling[1] != VC4_SCALING_NONE) {
>> > -		/* LBM Base Address. */
>> > +		/* Reserve a slot for the LBM Base Address. The real value will
>> > +		 * be set when calling vc4_plane_update_lbm().
>> > +		 */
>> >  		if (vc4_state->y_scaling[0] != VC4_SCALING_NONE ||
>> > -		    vc4_state->y_scaling[1] != VC4_SCALING_NONE) {
>> > -			vc4_dlist_write(vc4_state, vc4_state->lbm.start);
>> > -		}
>> > +		    vc4_state->y_scaling[1] != VC4_SCALING_NONE)
>> > +			vc4_state->lbm_offset = vc4_state->dlist_count++;
>> >  
>> >  		if (num_planes > 1) {
>> >  			/* Emit Cb/Cr as channel 0 and Y as channel
>> > @@ -785,13 +806,18 @@ static int vc4_plane_atomic_check(struct drm_plane *plane,
>> >  				  struct drm_plane_state *state)
>> >  {
>> >  	struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
>> > +	int ret;
>> >  
>> >  	vc4_state->dlist_count = 0;
>> >  
>> > -	if (plane_enabled(state))
>> > -		return vc4_plane_mode_set(plane, state);
>> > -	else
>> > +	if (!plane_enabled(state))
>> >  		return 0;
>> > +
>> > +	ret = vc4_plane_mode_set(plane, state);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	return vc4_plane_update_lbm(state);
>> >  }  
>> 
>> Couldn't you call update_lbm() (maybe called allocate_lbm()) first, and
>> then leave the dlist setup as is without needing lbm_offset?
>
> I'll anyway need lbm_offset for the dlist check I do in async_check().

Fair enough.

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 3/6] drm/vc4: Don't check plane state more than once

[Boris Brezillon]

We are about to use vc4_plane_mode_set() in the async check path, but
async check can decide that async update is not possible and force the
driver to fallback to a sync update.

All the checks that have been done on the plane state during async check
stay valid, and checking it again is not necessary. Add a ->checked
field to vc4_plane_state, and use it to track the status of the state
(checked or not).

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_drv.h   |  5 +++++
 drivers/gpu/drm/vc4/vc4_plane.c | 10 ++++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 9ed05fb61eb6..d1000c4805c2 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -370,6 +370,11 @@ struct vc4_plane_state {
 	 * to enable background color fill.
 	 */
 	bool needs_bg_fill;
+
+	/* Mark the state as checked. Useful to avoid checking it twice when
+	 * async update is not possible.
+	 */
+	bool checked;
 };
 
 static inline struct vc4_plane_state *
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 392a51f2bf8f..09c7478b095b 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -154,6 +154,7 @@ static struct drm_plane_state *vc4_plane_duplicate_state(struct drm_plane *plane
 		return NULL;
 
 	memset(&vc4_state->lbm, 0, sizeof(vc4_state->lbm));
+	vc4_state->checked = 0;
 
 	__drm_atomic_helper_plane_duplicate_state(plane, &vc4_state->base);
 
@@ -510,6 +511,8 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 	u32 hvs_format = format->hvs;
 	int ret, i;
 
+	if (vc4_state->checked)
+		return 0;
 
 	ret = vc4_plane_setup_clipping_and_scaling(state);
 	if (ret)
@@ -792,6 +795,13 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 	vc4_state->needs_bg_fill = fb->format->has_alpha || !covers_screen ||
 				   state->alpha != DRM_BLEND_ALPHA_OPAQUE;
 
+	/* Flag the state as already checked to avoid checking it twice in case
+	 * the async update check already called vc4_plane_mode_set() and
+	 * decided to fallback to sync update because async update was not
+	 * possible.
+	 */
+	vc4_state->checked = 1;
+
 	return 0;
 }
 
-- 
2.17.1

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

Re: [PATCH 3/6] drm/vc4: Don't check plane state more than once

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1300 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> We are about to use vc4_plane_mode_set() in the async check path, but
> async check can decide that async update is not possible and force the
> driver to fallback to a sync update.
>
> All the checks that have been done on the plane state during async check
> stay valid, and checking it again is not necessary. Add a ->checked
> field to vc4_plane_state, and use it to track the status of the state
> (checked or not).
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_drv.h   |  5 +++++
>  drivers/gpu/drm/vc4/vc4_plane.c | 10 ++++++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 9ed05fb61eb6..d1000c4805c2 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -370,6 +370,11 @@ struct vc4_plane_state {
>  	 * to enable background color fill.
>  	 */
>  	bool needs_bg_fill;
> +
> +	/* Mark the state as checked. Useful to avoid checking it twice when
> +	 * async update is not possible.
> +	 */
> +	bool checked;
>  };

Since this doesn't cover the whole atomic_check process, which won't
have been called from async update, maybe rename to dlist_initialized or
something?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 3/6] drm/vc4: Don't check plane state more than once

[Boris Brezillon]

On Thu, 15 Nov 2018 12:41:36 -0800
Eric Anholt <eric@anholt.net> wrote:

> Boris Brezillon <boris.brezillon@bootlin.com> writes:
> 
> > We are about to use vc4_plane_mode_set() in the async check path, but
> > async check can decide that async update is not possible and force the
> > driver to fallback to a sync update.
> >
> > All the checks that have been done on the plane state during async check
> > stay valid, and checking it again is not necessary. Add a ->checked
> > field to vc4_plane_state, and use it to track the status of the state
> > (checked or not).
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_drv.h   |  5 +++++
> >  drivers/gpu/drm/vc4/vc4_plane.c | 10 ++++++++++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index 9ed05fb61eb6..d1000c4805c2 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -370,6 +370,11 @@ struct vc4_plane_state {
> >  	 * to enable background color fill.
> >  	 */
> >  	bool needs_bg_fill;
> > +
> > +	/* Mark the state as checked. Useful to avoid checking it twice when
> > +	 * async update is not possible.
> > +	 */
> > +	bool checked;
> >  };  
> 
> Since this doesn't cover the whole atomic_check process, which won't
> have been called from async update, maybe rename to dlist_initialized or
> something?

Do you mean that vc4_plane_mode_set() should be run again from the sync
check path when async check failed, or is it just the name you don't
like?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm/vc4: Don't check plane state more than once

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1801 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> On Thu, 15 Nov 2018 12:41:36 -0800
> Eric Anholt <eric@anholt.net> wrote:
>
>> Boris Brezillon <boris.brezillon@bootlin.com> writes:
>> 
>> > We are about to use vc4_plane_mode_set() in the async check path, but
>> > async check can decide that async update is not possible and force the
>> > driver to fallback to a sync update.
>> >
>> > All the checks that have been done on the plane state during async check
>> > stay valid, and checking it again is not necessary. Add a ->checked
>> > field to vc4_plane_state, and use it to track the status of the state
>> > (checked or not).
>> >
>> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
>> > ---
>> >  drivers/gpu/drm/vc4/vc4_drv.h   |  5 +++++
>> >  drivers/gpu/drm/vc4/vc4_plane.c | 10 ++++++++++
>> >  2 files changed, 15 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
>> > index 9ed05fb61eb6..d1000c4805c2 100644
>> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
>> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
>> > @@ -370,6 +370,11 @@ struct vc4_plane_state {
>> >  	 * to enable background color fill.
>> >  	 */
>> >  	bool needs_bg_fill;
>> > +
>> > +	/* Mark the state as checked. Useful to avoid checking it twice when
>> > +	 * async update is not possible.
>> > +	 */
>> > +	bool checked;
>> >  };  
>> 
>> Since this doesn't cover the whole atomic_check process, which won't
>> have been called from async update, maybe rename to dlist_initialized or
>> something?
>
> Do you mean that vc4_plane_mode_set() should be run again from the sync
> check path when async check failed, or is it just the name you don't
> like?

Just the name, s/checked/dlist_initialized/ (or something)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 4/6] drm/vc4: Rework the async update logic

[Boris Brezillon]

vc4_plane_atomic_async_check() was only based on the
state->{crtc,src}_{w,h} which was fine since scaling was not allowed on
the cursor plane.

We are about to change that to properly support underscan, and, in order
to make the async check more reliable, we call vc4_plane_mode_set()
from there and check that only the pos0, pos2 and ptr0 entries in the
dlist have changed.

In vc4_plane_atomic_async_update(), we no longer call
vc4_plane_atomic_check() since vc4_plane_mode_set() has already been
called in vc4_plane_atomic_async_check(), and we don't need to allocate
a new LBM region (we reuse the one from the current state).

Note that we now have to manually update each field of the current
plane state since it's no longer updated in place (not sure we have
to sync all of them, but it's harmless if we do).
We also drop the vc4_plane_async_set_fb() call (ptr0 dlist entry has
been properly updated in vc4_plane_mode_set())

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_plane.c | 87 +++++++++++++++++++++++++--------
 1 file changed, 66 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 09c7478b095b..31c7b63dd723 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -895,30 +895,50 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane,
 {
 	struct vc4_plane_state *vc4_state, *new_vc4_state;
 
-	if (plane->state->fb != state->fb) {
-		vc4_plane_async_set_fb(plane, state->fb);
-		drm_atomic_set_fb_for_plane(plane->state, state->fb);
-	}
-
-	/* Set the cursor's position on the screen.  This is the
-	 * expected change from the drm_mode_cursor_universal()
-	 * helper.
-	 */
+	drm_atomic_set_fb_for_plane(plane->state, state->fb);
 	plane->state->crtc_x = state->crtc_x;
 	plane->state->crtc_y = state->crtc_y;
-
-	/* Allow changing the start position within the cursor BO, if
-	 * that matters.
-	 */
+	plane->state->crtc_w = state->crtc_w;
+	plane->state->crtc_h = state->crtc_h;
 	plane->state->src_x = state->src_x;
 	plane->state->src_y = state->src_y;
-
-	/* Update the display list based on the new crtc_x/y. */
-	vc4_plane_atomic_check(plane, state);
+	plane->state->src_w = state->src_w;
+	plane->state->src_h = state->src_h;
+	plane->state->src_h = state->src_h;
+	plane->state->alpha = state->alpha;
+	plane->state->pixel_blend_mode = state->pixel_blend_mode;
+	plane->state->rotation = state->rotation;
+	plane->state->zpos = state->zpos;
+	plane->state->normalized_zpos = state->normalized_zpos;
+	plane->state->color_encoding = state->color_encoding;
+	plane->state->color_range = state->color_range;
+	plane->state->src = state->src;
+	plane->state->dst = state->dst;
+	plane->state->visible = state->visible;
 
 	new_vc4_state = to_vc4_plane_state(state);
 	vc4_state = to_vc4_plane_state(plane->state);
 
+	vc4_state->crtc_x = new_vc4_state->crtc_x;
+	vc4_state->crtc_y = new_vc4_state->crtc_y;
+	vc4_state->crtc_h = new_vc4_state->crtc_h;
+	vc4_state->crtc_w = new_vc4_state->crtc_w;
+	vc4_state->src_x = new_vc4_state->src_x;
+	vc4_state->src_y = new_vc4_state->src_y;
+	memcpy(vc4_state->src_w, new_vc4_state->src_w,
+	       sizeof(vc4_state->src_w));
+	memcpy(vc4_state->src_h, new_vc4_state->src_h,
+	       sizeof(vc4_state->src_h));
+	memcpy(vc4_state->x_scaling, new_vc4_state->x_scaling,
+	       sizeof(vc4_state->x_scaling));
+	memcpy(vc4_state->y_scaling, new_vc4_state->y_scaling,
+	       sizeof(vc4_state->y_scaling));
+	vc4_state->is_unity = new_vc4_state->is_unity;
+	vc4_state->is_yuv = new_vc4_state->is_yuv;
+	memcpy(vc4_state->offsets, new_vc4_state->offsets,
+	       sizeof(vc4_state->offsets));
+	vc4_state->needs_bg_fill = new_vc4_state->needs_bg_fill;
+
 	/* Update the current vc4_state pos0, pos2 and ptr0 dlist entries. */
 	vc4_state->dlist[vc4_state->pos0_offset] =
 		new_vc4_state->dlist[vc4_state->pos0_offset];
@@ -942,13 +962,38 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane,
 static int vc4_plane_atomic_async_check(struct drm_plane *plane,
 					struct drm_plane_state *state)
 {
-	/* No configuring new scaling in the fast path. */
-	if (plane->state->crtc_w != state->crtc_w ||
-	    plane->state->crtc_h != state->crtc_h ||
-	    plane->state->src_w != state->src_w ||
-	    plane->state->src_h != state->src_h)
+	struct vc4_plane_state *old_vc4_state, *new_vc4_state;
+	int ret;
+	u32 i;
+
+	ret = vc4_plane_mode_set(plane, state);
+	if (ret)
+		return ret;
+
+	old_vc4_state = to_vc4_plane_state(plane->state);
+	new_vc4_state = to_vc4_plane_state(state);
+	if (old_vc4_state->dlist_count != new_vc4_state->dlist_count ||
+	    old_vc4_state->pos0_offset != new_vc4_state->pos0_offset ||
+	    old_vc4_state->pos2_offset != new_vc4_state->pos2_offset ||
+	    old_vc4_state->ptr0_offset != new_vc4_state->ptr0_offset ||
+	    vc4_lbm_size(plane->state) != vc4_lbm_size(state))
 		return -EINVAL;
 
+	/* Only pos0, pos2 and ptr0 DWORDS can be updated in an async update
+	 * if anything else has changed, fallback to a sync update.
+	 */
+	for (i = 0; i < new_vc4_state->dlist_count; i++) {
+		if (i == new_vc4_state->pos0_offset ||
+		    i == new_vc4_state->pos2_offset ||
+		    i == new_vc4_state->ptr0_offset ||
+		    (new_vc4_state->lbm_offset &&
+		     i == new_vc4_state->lbm_offset))
+			continue;
+
+		if (new_vc4_state->dlist[i] != old_vc4_state->dlist[i])
+			return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.17.1

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

Re: [PATCH 4/6] drm/vc4: Rework the async update logic

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 6299 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> vc4_plane_atomic_async_check() was only based on the
> state->{crtc,src}_{w,h} which was fine since scaling was not allowed on
> the cursor plane.
>
> We are about to change that to properly support underscan, and, in order
> to make the async check more reliable, we call vc4_plane_mode_set()
> from there and check that only the pos0, pos2 and ptr0 entries in the
> dlist have changed.
>
> In vc4_plane_atomic_async_update(), we no longer call
> vc4_plane_atomic_check() since vc4_plane_mode_set() has already been
> called in vc4_plane_atomic_async_check(), and we don't need to allocate
> a new LBM region (we reuse the one from the current state).
>
> Note that we now have to manually update each field of the current
> plane state since it's no longer updated in place (not sure we have
> to sync all of them, but it's harmless if we do).
> We also drop the vc4_plane_async_set_fb() call (ptr0 dlist entry has
> been properly updated in vc4_plane_mode_set())
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_plane.c | 87 +++++++++++++++++++++++++--------
>  1 file changed, 66 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index 09c7478b095b..31c7b63dd723 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -895,30 +895,50 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane,
>  {
>  	struct vc4_plane_state *vc4_state, *new_vc4_state;
>  
> -	if (plane->state->fb != state->fb) {
> -		vc4_plane_async_set_fb(plane, state->fb);
> -		drm_atomic_set_fb_for_plane(plane->state, state->fb);
> -	}
> -
> -	/* Set the cursor's position on the screen.  This is the
> -	 * expected change from the drm_mode_cursor_universal()
> -	 * helper.
> -	 */
> +	drm_atomic_set_fb_for_plane(plane->state, state->fb);
>  	plane->state->crtc_x = state->crtc_x;
>  	plane->state->crtc_y = state->crtc_y;
> -
> -	/* Allow changing the start position within the cursor BO, if
> -	 * that matters.
> -	 */
> +	plane->state->crtc_w = state->crtc_w;
> +	plane->state->crtc_h = state->crtc_h;
>  	plane->state->src_x = state->src_x;
>  	plane->state->src_y = state->src_y;
> -
> -	/* Update the display list based on the new crtc_x/y. */
> -	vc4_plane_atomic_check(plane, state);
> +	plane->state->src_w = state->src_w;
> +	plane->state->src_h = state->src_h;
> +	plane->state->src_h = state->src_h;
> +	plane->state->alpha = state->alpha;
> +	plane->state->pixel_blend_mode = state->pixel_blend_mode;
> +	plane->state->rotation = state->rotation;
> +	plane->state->zpos = state->zpos;
> +	plane->state->normalized_zpos = state->normalized_zpos;
> +	plane->state->color_encoding = state->color_encoding;
> +	plane->state->color_range = state->color_range;
> +	plane->state->src = state->src;
> +	plane->state->dst = state->dst;
> +	plane->state->visible = state->visible;
>  
>  	new_vc4_state = to_vc4_plane_state(state);
>  	vc4_state = to_vc4_plane_state(plane->state);
>  
> +	vc4_state->crtc_x = new_vc4_state->crtc_x;
> +	vc4_state->crtc_y = new_vc4_state->crtc_y;
> +	vc4_state->crtc_h = new_vc4_state->crtc_h;
> +	vc4_state->crtc_w = new_vc4_state->crtc_w;
> +	vc4_state->src_x = new_vc4_state->src_x;
> +	vc4_state->src_y = new_vc4_state->src_y;
> +	memcpy(vc4_state->src_w, new_vc4_state->src_w,
> +	       sizeof(vc4_state->src_w));
> +	memcpy(vc4_state->src_h, new_vc4_state->src_h,
> +	       sizeof(vc4_state->src_h));
> +	memcpy(vc4_state->x_scaling, new_vc4_state->x_scaling,
> +	       sizeof(vc4_state->x_scaling));
> +	memcpy(vc4_state->y_scaling, new_vc4_state->y_scaling,
> +	       sizeof(vc4_state->y_scaling));
> +	vc4_state->is_unity = new_vc4_state->is_unity;
> +	vc4_state->is_yuv = new_vc4_state->is_yuv;
> +	memcpy(vc4_state->offsets, new_vc4_state->offsets,
> +	       sizeof(vc4_state->offsets));
> +	vc4_state->needs_bg_fill = new_vc4_state->needs_bg_fill;

This copying feels like a maintenance nightmare to me -- nothing's going
to be testing async updates of each random bit of state, so if something
new could change while passing atomic_async_check(), we're going to get
it wrong.

Any ideas for what we can do to handle that?

> +
>  	/* Update the current vc4_state pos0, pos2 and ptr0 dlist entries. */
>  	vc4_state->dlist[vc4_state->pos0_offset] =
>  		new_vc4_state->dlist[vc4_state->pos0_offset];
> @@ -942,13 +962,38 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane,
>  static int vc4_plane_atomic_async_check(struct drm_plane *plane,
>  					struct drm_plane_state *state)
>  {
> -	/* No configuring new scaling in the fast path. */
> -	if (plane->state->crtc_w != state->crtc_w ||
> -	    plane->state->crtc_h != state->crtc_h ||
> -	    plane->state->src_w != state->src_w ||
> -	    plane->state->src_h != state->src_h)
> +	struct vc4_plane_state *old_vc4_state, *new_vc4_state;
> +	int ret;
> +	u32 i;
> +
> +	ret = vc4_plane_mode_set(plane, state);
> +	if (ret)
> +		return ret;
> +
> +	old_vc4_state = to_vc4_plane_state(plane->state);
> +	new_vc4_state = to_vc4_plane_state(state);
> +	if (old_vc4_state->dlist_count != new_vc4_state->dlist_count ||
> +	    old_vc4_state->pos0_offset != new_vc4_state->pos0_offset ||
> +	    old_vc4_state->pos2_offset != new_vc4_state->pos2_offset ||
> +	    old_vc4_state->ptr0_offset != new_vc4_state->ptr0_offset ||
> +	    vc4_lbm_size(plane->state) != vc4_lbm_size(state))
>  		return -EINVAL;
>  
> +	/* Only pos0, pos2 and ptr0 DWORDS can be updated in an async update
> +	 * if anything else has changed, fallback to a sync update.
> +	 */
> +	for (i = 0; i < new_vc4_state->dlist_count; i++) {
> +		if (i == new_vc4_state->pos0_offset ||
> +		    i == new_vc4_state->pos2_offset ||
> +		    i == new_vc4_state->ptr0_offset ||
> +		    (new_vc4_state->lbm_offset &&
> +		     i == new_vc4_state->lbm_offset))
> +			continue;
> +
> +		if (new_vc4_state->dlist[i] != old_vc4_state->dlist[i])
> +			return -EINVAL;
> +	}
> +

I really like these new checks for whether we can do the async update,
compared to my old ones!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 4/6] drm/vc4: Rework the async update logic

[Boris Brezillon]

On Thu, 15 Nov 2018 12:49:11 -0800
Eric Anholt <eric@anholt.net> wrote:

> Boris Brezillon <boris.brezillon@bootlin.com> writes:
> 
> > vc4_plane_atomic_async_check() was only based on the
> > state->{crtc,src}_{w,h} which was fine since scaling was not allowed on
> > the cursor plane.
> >
> > We are about to change that to properly support underscan, and, in order
> > to make the async check more reliable, we call vc4_plane_mode_set()
> > from there and check that only the pos0, pos2 and ptr0 entries in the
> > dlist have changed.
> >
> > In vc4_plane_atomic_async_update(), we no longer call
> > vc4_plane_atomic_check() since vc4_plane_mode_set() has already been
> > called in vc4_plane_atomic_async_check(), and we don't need to allocate
> > a new LBM region (we reuse the one from the current state).
> >
> > Note that we now have to manually update each field of the current
> > plane state since it's no longer updated in place (not sure we have
> > to sync all of them, but it's harmless if we do).
> > We also drop the vc4_plane_async_set_fb() call (ptr0 dlist entry has
> > been properly updated in vc4_plane_mode_set())
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_plane.c | 87 +++++++++++++++++++++++++--------
> >  1 file changed, 66 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > index 09c7478b095b..31c7b63dd723 100644
> > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > @@ -895,30 +895,50 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane,
> >  {
> >  	struct vc4_plane_state *vc4_state, *new_vc4_state;
> >  
> > -	if (plane->state->fb != state->fb) {
> > -		vc4_plane_async_set_fb(plane, state->fb);
> > -		drm_atomic_set_fb_for_plane(plane->state, state->fb);
> > -	}
> > -
> > -	/* Set the cursor's position on the screen.  This is the
> > -	 * expected change from the drm_mode_cursor_universal()
> > -	 * helper.
> > -	 */
> > +	drm_atomic_set_fb_for_plane(plane->state, state->fb);
> >  	plane->state->crtc_x = state->crtc_x;
> >  	plane->state->crtc_y = state->crtc_y;
> > -
> > -	/* Allow changing the start position within the cursor BO, if
> > -	 * that matters.
> > -	 */
> > +	plane->state->crtc_w = state->crtc_w;
> > +	plane->state->crtc_h = state->crtc_h;
> >  	plane->state->src_x = state->src_x;
> >  	plane->state->src_y = state->src_y;
> > -
> > -	/* Update the display list based on the new crtc_x/y. */
> > -	vc4_plane_atomic_check(plane, state);
> > +	plane->state->src_w = state->src_w;
> > +	plane->state->src_h = state->src_h;
> > +	plane->state->src_h = state->src_h;
> > +	plane->state->alpha = state->alpha;
> > +	plane->state->pixel_blend_mode = state->pixel_blend_mode;
> > +	plane->state->rotation = state->rotation;
> > +	plane->state->zpos = state->zpos;
> > +	plane->state->normalized_zpos = state->normalized_zpos;
> > +	plane->state->color_encoding = state->color_encoding;
> > +	plane->state->color_range = state->color_range;
> > +	plane->state->src = state->src;
> > +	plane->state->dst = state->dst;
> > +	plane->state->visible = state->visible;
> >  
> >  	new_vc4_state = to_vc4_plane_state(state);
> >  	vc4_state = to_vc4_plane_state(plane->state);
> >  
> > +	vc4_state->crtc_x = new_vc4_state->crtc_x;
> > +	vc4_state->crtc_y = new_vc4_state->crtc_y;
> > +	vc4_state->crtc_h = new_vc4_state->crtc_h;
> > +	vc4_state->crtc_w = new_vc4_state->crtc_w;
> > +	vc4_state->src_x = new_vc4_state->src_x;
> > +	vc4_state->src_y = new_vc4_state->src_y;
> > +	memcpy(vc4_state->src_w, new_vc4_state->src_w,
> > +	       sizeof(vc4_state->src_w));
> > +	memcpy(vc4_state->src_h, new_vc4_state->src_h,
> > +	       sizeof(vc4_state->src_h));
> > +	memcpy(vc4_state->x_scaling, new_vc4_state->x_scaling,
> > +	       sizeof(vc4_state->x_scaling));
> > +	memcpy(vc4_state->y_scaling, new_vc4_state->y_scaling,
> > +	       sizeof(vc4_state->y_scaling));
> > +	vc4_state->is_unity = new_vc4_state->is_unity;
> > +	vc4_state->is_yuv = new_vc4_state->is_yuv;
> > +	memcpy(vc4_state->offsets, new_vc4_state->offsets,
> > +	       sizeof(vc4_state->offsets));
> > +	vc4_state->needs_bg_fill = new_vc4_state->needs_bg_fill;  
> 
> This copying feels like a maintenance nightmare to me -- nothing's going
> to be testing async updates of each random bit of state, so if something
> new could change while passing atomic_async_check(), we're going to get
> it wrong.

Yeah, I don't like it either. I'd definitely prefer if states could be
swapped somehow, but then you have the problem of migrating some of the
resources from the old state to the new one (the most obvious one being
the LBM drm_mm_node object which is already inserted in a list, but
I'm pretty we have the same issue with other fields too).

> 
> Any ideas for what we can do to handle that?

Nope.

> 
> > +
> >  	/* Update the current vc4_state pos0, pos2 and ptr0 dlist entries. */
> >  	vc4_state->dlist[vc4_state->pos0_offset] =
> >  		new_vc4_state->dlist[vc4_state->pos0_offset];
> > @@ -942,13 +962,38 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane,
> >  static int vc4_plane_atomic_async_check(struct drm_plane *plane,
> >  					struct drm_plane_state *state)
> >  {
> > -	/* No configuring new scaling in the fast path. */
> > -	if (plane->state->crtc_w != state->crtc_w ||
> > -	    plane->state->crtc_h != state->crtc_h ||
> > -	    plane->state->src_w != state->src_w ||
> > -	    plane->state->src_h != state->src_h)
> > +	struct vc4_plane_state *old_vc4_state, *new_vc4_state;
> > +	int ret;
> > +	u32 i;
> > +
> > +	ret = vc4_plane_mode_set(plane, state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	old_vc4_state = to_vc4_plane_state(plane->state);
> > +	new_vc4_state = to_vc4_plane_state(state);
> > +	if (old_vc4_state->dlist_count != new_vc4_state->dlist_count ||
> > +	    old_vc4_state->pos0_offset != new_vc4_state->pos0_offset ||
> > +	    old_vc4_state->pos2_offset != new_vc4_state->pos2_offset ||
> > +	    old_vc4_state->ptr0_offset != new_vc4_state->ptr0_offset ||
> > +	    vc4_lbm_size(plane->state) != vc4_lbm_size(state))
> >  		return -EINVAL;
> >  
> > +	/* Only pos0, pos2 and ptr0 DWORDS can be updated in an async update
> > +	 * if anything else has changed, fallback to a sync update.
> > +	 */
> > +	for (i = 0; i < new_vc4_state->dlist_count; i++) {
> > +		if (i == new_vc4_state->pos0_offset ||
> > +		    i == new_vc4_state->pos2_offset ||
> > +		    i == new_vc4_state->ptr0_offset ||
> > +		    (new_vc4_state->lbm_offset &&
> > +		     i == new_vc4_state->lbm_offset))
> > +			continue;
> > +
> > +		if (new_vc4_state->dlist[i] != old_vc4_state->dlist[i])
> > +			return -EINVAL;
> > +	}
> > +  
> 
> I really like these new checks for whether we can do the async update,
> compared to my old ones!

Yes. I'm pretty sure we can allow async updates when other of those
dlist entries change, but I wanted to be on the safe side.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] drm/vc4: Rework the async update logic

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 5661 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> On Thu, 15 Nov 2018 12:49:11 -0800
> Eric Anholt <eric@anholt.net> wrote:
>
>> Boris Brezillon <boris.brezillon@bootlin.com> writes:
>> 
>> > vc4_plane_atomic_async_check() was only based on the
>> > state->{crtc,src}_{w,h} which was fine since scaling was not allowed on
>> > the cursor plane.
>> >
>> > We are about to change that to properly support underscan, and, in order
>> > to make the async check more reliable, we call vc4_plane_mode_set()
>> > from there and check that only the pos0, pos2 and ptr0 entries in the
>> > dlist have changed.
>> >
>> > In vc4_plane_atomic_async_update(), we no longer call
>> > vc4_plane_atomic_check() since vc4_plane_mode_set() has already been
>> > called in vc4_plane_atomic_async_check(), and we don't need to allocate
>> > a new LBM region (we reuse the one from the current state).
>> >
>> > Note that we now have to manually update each field of the current
>> > plane state since it's no longer updated in place (not sure we have
>> > to sync all of them, but it's harmless if we do).
>> > We also drop the vc4_plane_async_set_fb() call (ptr0 dlist entry has
>> > been properly updated in vc4_plane_mode_set())
>> >
>> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
>> > ---
>> >  drivers/gpu/drm/vc4/vc4_plane.c | 87 +++++++++++++++++++++++++--------
>> >  1 file changed, 66 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
>> > index 09c7478b095b..31c7b63dd723 100644
>> > --- a/drivers/gpu/drm/vc4/vc4_plane.c
>> > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
>> > @@ -895,30 +895,50 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane,
>> >  {
>> >  	struct vc4_plane_state *vc4_state, *new_vc4_state;
>> >  
>> > -	if (plane->state->fb != state->fb) {
>> > -		vc4_plane_async_set_fb(plane, state->fb);
>> > -		drm_atomic_set_fb_for_plane(plane->state, state->fb);
>> > -	}
>> > -
>> > -	/* Set the cursor's position on the screen.  This is the
>> > -	 * expected change from the drm_mode_cursor_universal()
>> > -	 * helper.
>> > -	 */
>> > +	drm_atomic_set_fb_for_plane(plane->state, state->fb);
>> >  	plane->state->crtc_x = state->crtc_x;
>> >  	plane->state->crtc_y = state->crtc_y;
>> > -
>> > -	/* Allow changing the start position within the cursor BO, if
>> > -	 * that matters.
>> > -	 */
>> > +	plane->state->crtc_w = state->crtc_w;
>> > +	plane->state->crtc_h = state->crtc_h;
>> >  	plane->state->src_x = state->src_x;
>> >  	plane->state->src_y = state->src_y;
>> > -
>> > -	/* Update the display list based on the new crtc_x/y. */
>> > -	vc4_plane_atomic_check(plane, state);
>> > +	plane->state->src_w = state->src_w;
>> > +	plane->state->src_h = state->src_h;
>> > +	plane->state->src_h = state->src_h;
>> > +	plane->state->alpha = state->alpha;
>> > +	plane->state->pixel_blend_mode = state->pixel_blend_mode;
>> > +	plane->state->rotation = state->rotation;
>> > +	plane->state->zpos = state->zpos;
>> > +	plane->state->normalized_zpos = state->normalized_zpos;
>> > +	plane->state->color_encoding = state->color_encoding;
>> > +	plane->state->color_range = state->color_range;
>> > +	plane->state->src = state->src;
>> > +	plane->state->dst = state->dst;
>> > +	plane->state->visible = state->visible;
>> >  
>> >  	new_vc4_state = to_vc4_plane_state(state);
>> >  	vc4_state = to_vc4_plane_state(plane->state);
>> >  
>> > +	vc4_state->crtc_x = new_vc4_state->crtc_x;
>> > +	vc4_state->crtc_y = new_vc4_state->crtc_y;
>> > +	vc4_state->crtc_h = new_vc4_state->crtc_h;
>> > +	vc4_state->crtc_w = new_vc4_state->crtc_w;
>> > +	vc4_state->src_x = new_vc4_state->src_x;
>> > +	vc4_state->src_y = new_vc4_state->src_y;
>> > +	memcpy(vc4_state->src_w, new_vc4_state->src_w,
>> > +	       sizeof(vc4_state->src_w));
>> > +	memcpy(vc4_state->src_h, new_vc4_state->src_h,
>> > +	       sizeof(vc4_state->src_h));
>> > +	memcpy(vc4_state->x_scaling, new_vc4_state->x_scaling,
>> > +	       sizeof(vc4_state->x_scaling));
>> > +	memcpy(vc4_state->y_scaling, new_vc4_state->y_scaling,
>> > +	       sizeof(vc4_state->y_scaling));
>> > +	vc4_state->is_unity = new_vc4_state->is_unity;
>> > +	vc4_state->is_yuv = new_vc4_state->is_yuv;
>> > +	memcpy(vc4_state->offsets, new_vc4_state->offsets,
>> > +	       sizeof(vc4_state->offsets));
>> > +	vc4_state->needs_bg_fill = new_vc4_state->needs_bg_fill;  
>> 
>> This copying feels like a maintenance nightmare to me -- nothing's going
>> to be testing async updates of each random bit of state, so if something
>> new could change while passing atomic_async_check(), we're going to get
>> it wrong.
>
> Yeah, I don't like it either. I'd definitely prefer if states could be
> swapped somehow, but then you have the problem of migrating some of the
> resources from the old state to the new one (the most obvious one being
> the LBM drm_mm_node object which is already inserted in a list, but
> I'm pretty we have the same issue with other fields too).
>
>> 
>> Any ideas for what we can do to handle that?
>
> Nope.

I think I'm less concerned than I was the first time around.
Realistically, potential new fields won't be changing anyway.  If they
did, they'd have an effect on the dlists other than position/pointer,
and that would make them fail the async check.

So, while I have some reservations, I'm also unhappy with my solution
too, and I think yours has benefits that outweigh the cost of the code
in question here.

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 5/6] drm/vc4: Allow scaling on cursor plane

[Boris Brezillon]

Now that async update has been reworked to allow scaled planes to be
updated asynchronously when the scaling params do not change, we can
remove the NO_SCALING constraint on cursor planes.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_plane.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 31c7b63dd723..f2fba845da2f 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -260,14 +260,12 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane)
 
 static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state)
 {
-	struct drm_plane *plane = state->plane;
 	struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_gem_cma_object *bo = drm_fb_cma_get_gem_obj(fb, 0);
 	u32 subpixel_src_mask = (1 << 16) - 1;
 	u32 format = fb->format->format;
 	int num_planes = fb->format->num_planes;
-	int min_scale = 1, max_scale = INT_MAX;
 	struct drm_crtc_state *crtc_state;
 	u32 h_subsample, v_subsample;
 	int i, ret;
@@ -279,21 +277,8 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state)
 		return -EINVAL;
 	}
 
-	/* No configuring scaling on the cursor plane, since it gets
-	 * non-vblank-synced updates, and scaling requires LBM changes which
-	 * have to be vblank-synced.
-	 */
-	if (plane->type == DRM_PLANE_TYPE_CURSOR) {
-		min_scale = DRM_PLANE_HELPER_NO_SCALING;
-		max_scale = DRM_PLANE_HELPER_NO_SCALING;
-	} else {
-		min_scale = 1;
-		max_scale = INT_MAX;
-	}
-
-	ret = drm_atomic_helper_check_plane_state(state, crtc_state,
-						  min_scale, max_scale,
-						  true, true);
+	ret = drm_atomic_helper_check_plane_state(state, crtc_state, 1,
+						  INT_MAX, true, true);
 	if (ret)
 		return ret;
 
-- 
2.17.1

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

Re: [PATCH 5/6] drm/vc4: Allow scaling on cursor plane

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 276 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> Now that async update has been reworked to allow scaled planes to be
> updated asynchronously when the scaling params do not change, we can
> remove the NO_SCALING constraint on cursor planes.

These last 2 get my r-b.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 6/6] drm/vc4: Allow YUV formats on cursor planes

[Boris Brezillon]

Now that scaling is allowed on cursor planes, we can also allow YUV
formats.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_plane.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index f2fba845da2f..8cda0d460a6d 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -1095,7 +1095,6 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
 	struct drm_plane *plane = NULL;
 	struct vc4_plane *vc4_plane;
 	u32 formats[ARRAY_SIZE(hvs_formats)];
-	u32 num_formats = 0;
 	int ret = 0;
 	unsigned i;
 	static const uint64_t modifiers[] = {
@@ -1112,20 +1111,13 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
 	if (!vc4_plane)
 		return ERR_PTR(-ENOMEM);
 
-	for (i = 0; i < ARRAY_SIZE(hvs_formats); i++) {
-		/* Don't allow YUV in cursor planes, since that means
-		 * tuning on the scaler, which we don't allow for the
-		 * cursor.
-		 */
-		if (type != DRM_PLANE_TYPE_CURSOR ||
-		    hvs_formats[i].hvs < HVS_PIXEL_FORMAT_YCBCR_YUV420_3PLANE) {
-			formats[num_formats++] = hvs_formats[i].drm;
-		}
-	}
+	for (i = 0; i < ARRAY_SIZE(hvs_formats); i++)
+		formats[i] = hvs_formats[i].drm;
+
 	plane = &vc4_plane->base;
 	ret = drm_universal_plane_init(dev, plane, 0,
 				       &vc4_plane_funcs,
-				       formats, num_formats,
+				       formats, ARRAY_SIZE(formats),
 				       modifiers, type, NULL);
 
 	drm_plane_helper_add(plane, &vc4_plane_helper_funcs);
-- 
2.17.1

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