[PATCH 1/3] drm/imx: ipuv3-plane: Switch EBA buffer only when we don't need modeset

[PATCH 1/3] drm/imx: ipuv3-plane: Switch EBA buffer only when we don't need modeset

[Liu Ying]

We added active plane reconfiguration support by forcing a full modeset
operation.  So, looking at old_plane_state->fb to determine whether we need to
switch EBA buffer(for hardware double buffering) in ipu_plane_atomic_set_base()
or not is no more correct.  Instead, we should do that only when we don't need
modeset, otherwise, we initialize the two EBA buffers with the buffer address.

Fixes: c6c1f9bc798b ("drm/imx: Add active plane reconfiguration support")
Cc: stable@vger.kernel.org # 4.8
Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 29423e75..9ca5739 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -108,6 +108,7 @@ static void ipu_plane_atomic_set_base(struct ipu_plane *ipu_plane,
 {
 	struct drm_plane *plane = &ipu_plane->base;
 	struct drm_plane_state *state = plane->state;
+	struct drm_crtc_state *crtc_state = state->crtc->state;
 	struct drm_framebuffer *fb = state->fb;
 	unsigned long eba, ubo, vbo;
 	int active;
@@ -149,7 +150,7 @@ static void ipu_plane_atomic_set_base(struct ipu_plane *ipu_plane,
 		break;
 	}
 
-	if (old_state->fb) {
+	if (!drm_atomic_crtc_needs_modeset(crtc_state)) {
 		active = ipu_idmac_get_current_buffer(ipu_plane->ipu_ch);
 		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, !active, eba);
 		ipu_idmac_select_buffer(ipu_plane->ipu_ch, !active);
-- 
2.7.4

[PATCH 2/3] drm/imx: ipuv3-plane: Skip setting u/vbo only when we don't need modeset

[Liu Ying]

We added active plane reconfiguration support by forcing a full modeset
operation.  So, looking at old_plane_state->fb to determine whether we need
to set u/v offset(aka, u/vbo for IPUv3) in ipu_plane_atomic_set_base()
or not is no more correct.  Instead, we should do that only when we don't
need modeset.

Fixes: c6c1f9bc798b ("drm/imx: Add active plane reconfiguration support")
Cc: stable@vger.kernel.org # 4.8
Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 9ca5739..e33110e 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -103,8 +103,7 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
 	       (state->src_x >> 16) / 2 - eba;
 }
 
-static void ipu_plane_atomic_set_base(struct ipu_plane *ipu_plane,
-				      struct drm_plane_state *old_state)
+static void ipu_plane_atomic_set_base(struct ipu_plane *ipu_plane)
 {
 	struct drm_plane *plane = &ipu_plane->base;
 	struct drm_plane_state *state = plane->state;
@@ -118,7 +117,7 @@ static void ipu_plane_atomic_set_base(struct ipu_plane *ipu_plane,
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_YUV420:
 	case DRM_FORMAT_YVU420:
-		if (old_state->fb)
+		if (!drm_atomic_crtc_needs_modeset(crtc_state))
 			break;
 
 		/*
@@ -397,7 +396,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 		struct drm_crtc_state *crtc_state = state->crtc->state;
 
 		if (!crtc_state->mode_changed) {
-			ipu_plane_atomic_set_base(ipu_plane, old_state);
+			ipu_plane_atomic_set_base(ipu_plane);
 			return;
 		}
 
@@ -444,7 +443,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 	ipu_cpmem_set_high_priority(ipu_plane->ipu_ch);
 	ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
 	ipu_cpmem_set_stride(ipu_plane->ipu_ch, state->fb->pitches[0]);
-	ipu_plane_atomic_set_base(ipu_plane, old_state);
+	ipu_plane_atomic_set_base(ipu_plane);
 	ipu_plane_enable(ipu_plane);
 }
 
-- 
2.7.4

[PATCH 3/3] drm/imx: ipuv3-plane: Access old u/vbo properly in ->atomic_check for YU12/YV12

[Liu Ying]

Before accessing the u/v offset(aka, u/vbo for IPUv3) of the old plane state's
relevant fb, we should make sure the fb is in YU12 or YV12 pixel format(which
are the two YUV pixel formats we support only), otherwise, we are likely to
trigger BUG_ON() in drm_plane_state_to_u/vbo() since the fb's pixel format is
probably not YU12 or YV12.

Link: https://bugs.freedesktop.org/show_bug.cgi?id=98150
Fixes: c6c1f9bc798b ("drm/imx: Add active plane reconfiguration support")
Cc: stable@vger.kernel.org # 4.8
Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index e33110e..a691892 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -359,7 +359,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 		if ((ubo > 0xfffff8) || (vbo > 0xfffff8))
 			return -EINVAL;
 
-		if (old_fb) {
+		if (old_fb && old_fb->pixel_format == fb->pixel_format) {
 			old_ubo = drm_plane_state_to_ubo(old_state);
 			old_vbo = drm_plane_state_to_vbo(old_state);
 			if (ubo != old_ubo || vbo != old_vbo)
-- 
2.7.4

Re: [PATCH 3/3] drm/imx: ipuv3-plane: Access old u/vbo properly in ->atomic_check for YU12/YV12

[Philipp Zabel]

Hi Liu,

Am Montag, den 10.10.2016, 14:50 +0800 schrieb Liu Ying:
> Before accessing the u/v offset(aka, u/vbo for IPUv3) of the old plane state's
> relevant fb, we should make sure the fb is in YU12 or YV12 pixel format(which
> are the two YUV pixel formats we support only), otherwise, we are likely to
> trigger BUG_ON() in drm_plane_state_to_u/vbo() since the fb's pixel format is
> probably not YU12 or YV12.
> 
> Link: https://bugs.freedesktop.org/show_bug.cgi?id=98150
> Fixes: c6c1f9bc798b ("drm/imx: Add active plane reconfiguration support")
> Cc: stable@vger.kernel.org # 4.8
> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> ---
>  drivers/gpu/drm/imx/ipuv3-plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index e33110e..a691892 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -359,7 +359,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>  		if ((ubo > 0xfffff8) || (vbo > 0xfffff8))
>  			return -EINVAL;
>  
> -		if (old_fb) {
> +		if (old_fb && old_fb->pixel_format == fb->pixel_format) {
>  			old_ubo = drm_plane_state_to_ubo(old_state);
>  			old_vbo = drm_plane_state_to_vbo(old_state);
>  			if (ubo != old_ubo || vbo != old_vbo)

thank you for the patches. I have applied patches 1 and 2, but with this
change UBO/VBO changes are ignored when switching from YU12 to YV12.

Shouldn't we rather set crtc_state->mode_changed = true if either
(fb->pixel_format != old_fb->pixel_format) for any pixel format or
(ubo != old_ubo || vbo != old_vbo) for YUV formats, instead of returning
-EINVAL?

regards
Philipp

Re: [PATCH 3/3] drm/imx: ipuv3-plane: Access old u/vbo properly in ->atomic_check for YU12/YV12

[Liu Ying]

Hi Philipp,

2016-10-18 0:10 GMT+08:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Hi Liu,
>
> Am Montag, den 10.10.2016, 14:50 +0800 schrieb Liu Ying:
>> Before accessing the u/v offset(aka, u/vbo for IPUv3) of the old plane state's
>> relevant fb, we should make sure the fb is in YU12 or YV12 pixel format(which
>> are the two YUV pixel formats we support only), otherwise, we are likely to
>> trigger BUG_ON() in drm_plane_state_to_u/vbo() since the fb's pixel format is
>> probably not YU12 or YV12.
>>
>> Link: https://bugs.freedesktop.org/show_bug.cgi?id=98150
>> Fixes: c6c1f9bc798b ("drm/imx: Add active plane reconfiguration support")
>> Cc: stable@vger.kernel.org # 4.8
>> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
>> ---
>>  drivers/gpu/drm/imx/ipuv3-plane.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
>> index e33110e..a691892 100644
>> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
>> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
>> @@ -359,7 +359,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>>               if ((ubo > 0xfffff8) || (vbo > 0xfffff8))
>>                       return -EINVAL;
>>
>> -             if (old_fb) {
>> +             if (old_fb && old_fb->pixel_format == fb->pixel_format) {
>>                       old_ubo = drm_plane_state_to_ubo(old_state);
>>                       old_vbo = drm_plane_state_to_vbo(old_state);
>>                       if (ubo != old_ubo || vbo != old_vbo)
>
> thank you for the patches. I have applied patches 1 and 2, but with this
> change UBO/VBO changes are ignored when switching from YU12 to YV12.

Good catch.  Does this change look okay, then?

-               if (old_fb) {
+               if (old_fb &&
+                   (old_fb->pixel_format == DRM_FORMAT_YUV420 ||
+                    old_fb->pixel_format == DRM_FORMAT_YVU420)) {

>
> Shouldn't we rather set crtc_state->mode_changed = true if either
> (fb->pixel_format != old_fb->pixel_format) for any pixel format or
> (ubo != old_ubo || vbo != old_vbo) for YUV formats, instead of returning
> -EINVAL?

I thought about this and determined that this could be done
in an additional patch later.
It would be good if we change as little as we can in a patch
with stable Cced.

Regards,
Liu Ying

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



-- 
Best Regards,
Liu Ying

Re: [PATCH 3/3] drm/imx: ipuv3-plane: Access old u/vbo properly in ->atomic_check for YU12/YV12

[Philipp Zabel]

Am Dienstag, den 18.10.2016, 11:39 +0800 schrieb Liu Ying:
[...]
> >> @@ -359,7 +359,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >>               if ((ubo > 0xfffff8) || (vbo > 0xfffff8))
> >>                       return -EINVAL;
> >>
> >> -             if (old_fb) {
> >> +             if (old_fb && old_fb->pixel_format == fb->pixel_format) {
> >>                       old_ubo = drm_plane_state_to_ubo(old_state);
> >>                       old_vbo = drm_plane_state_to_vbo(old_state);
> >>                       if (ubo != old_ubo || vbo != old_vbo)
> >
> > thank you for the patches. I have applied patches 1 and 2, but with this
> > change UBO/VBO changes are ignored when switching from YU12 to YV12.
> 
> Good catch.  Does this change look okay, then?
> 
> -               if (old_fb) {
> +               if (old_fb &&
> +                   (old_fb->pixel_format == DRM_FORMAT_YUV420 ||
> +                    old_fb->pixel_format == DRM_FORMAT_YVU420)) {
> 
> >
> > Shouldn't we rather set crtc_state->mode_changed = true if either
> > (fb->pixel_format != old_fb->pixel_format) for any pixel format or
> > (ubo != old_ubo || vbo != old_vbo) for YUV formats, instead of returning
> > -EINVAL?
> 
> I thought about this and determined that this could be done
> in an additional patch later.
> It would be good if we change as little as we can in a patch
> with stable Cced.

Alright, the change you suggest above should work then.

regards
Philipp