[PATCH 1/4] drm/hibmc: Use drm_atomic_helper_check_plane_state()

[PATCH 1/4] drm/hibmc: Use drm_atomic_helper_check_plane_state()

[Thomas Zimmermann]

Call drm_atomic_helper_check_plane_state() from the primary plane's
atomic-check helper and replace the custom implementation.

All plane's implementations of atomic_check should call the shared
_check_plane_state() helper first. It adjusts the plane state for
correct positioning, rotation and scaling of the plane. If the plane
is not visible, it clears the corresponding flag in the plane state.

On errors or if the plane is not visible, the atomic-check helper can
return early. Implement all this in hibmc and drop the custom code
that does some of it.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: da52605eea8f ("drm/hisilicon/hibmc: Add support for display engine")
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Baihan Li <libaihan@huawei.com>
Cc: Yongbang Shi <shiyongbang@huawei.com>
Cc: <stable@vger.kernel.org> # v4.10+
---
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    | 46 ++++++-------------
 1 file changed, 14 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index 89bed78f1466..8fa2a95bcdd1 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -55,46 +55,28 @@ static const struct hibmc_dislay_pll_config hibmc_pll_table[] = {
 static int hibmc_plane_atomic_check(struct drm_plane *plane,
 				    struct drm_atomic_state *state)
 {
-	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
-										 plane);
-	struct drm_framebuffer *fb = new_plane_state->fb;
-	struct drm_crtc *crtc = new_plane_state->crtc;
-	struct drm_crtc_state *crtc_state;
-	u32 src_w = new_plane_state->src_w >> 16;
-	u32 src_h = new_plane_state->src_h >> 16;
-
-	if (!crtc || !fb)
-		return 0;
+	struct drm_plane_state *new_plane_state =
+		drm_atomic_get_new_plane_state(state, plane);
+	struct drm_crtc_state *new_crtc_state = NULL;
+	int ret;
 
-	crtc_state = drm_atomic_get_crtc_state(state, crtc);
-	if (IS_ERR(crtc_state))
-		return PTR_ERR(crtc_state);
+	if (new_plane_state->crtc)
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
 
-	if (src_w != new_plane_state->crtc_w || src_h != new_plane_state->crtc_h) {
-		drm_dbg_atomic(plane->dev, "scale not support\n");
-		return -EINVAL;
-	}
-
-	if (new_plane_state->crtc_x < 0 || new_plane_state->crtc_y < 0) {
-		drm_dbg_atomic(plane->dev, "crtc_x/y of drm_plane state is invalid\n");
-		return -EINVAL;
-	}
-
-	if (!crtc_state->enable)
+	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
+						  DRM_PLANE_NO_SCALING,
+						  DRM_PLANE_NO_SCALING,
+						  false, true);
+	if (ret)
+		return ret;
+	else if (!new_plane_state->visible)
 		return 0;
 
-	if (new_plane_state->crtc_x + new_plane_state->crtc_w >
-	    crtc_state->adjusted_mode.hdisplay ||
-	    new_plane_state->crtc_y + new_plane_state->crtc_h >
-	    crtc_state->adjusted_mode.vdisplay) {
-		drm_dbg_atomic(plane->dev, "visible portion of plane is invalid\n");
-		return -EINVAL;
-	}
-
 	if (new_plane_state->fb->pitches[0] % 128 != 0) {
 		drm_dbg_atomic(plane->dev, "wrong stride with 128-byte aligned\n");
 		return -EINVAL;
 	}
+
 	return 0;
 }
 
-- 
2.53.0

[PATCH 2/4] drm/hibmc: Fix list of formats on the primary plane

[Thomas Zimmermann]

Remove all formats from the primary plane that are unsupported for
various reasons.

* Formats with alpha channel: planes should not announce alpha channels
unless they support transparency. There's no transparency support in
the primary plane's implementation.

* Formats with BGR order. The common format is in RGB channel order.
There's no BGR support in the primary plane's implementation.

* RGB888: atomic_update programs the format from cpp[0] * 8 / 16. For
RGB888's cpp value of 3 this returns 1.5; rounded to 1. Programming
the value of 1 to HIBMC_CRT_DISP_CTL_FORMAT sets up RGB565. Hence, the
output is distorted. This can be tested by booting with video=1024x768-24.

Removing all unsupported formats leaves XRGB8888 and RGB565. Both of
which are supported and work correctly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: da52605eea8f ("drm/hisilicon/hibmc: Add support for display engine")
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Yongbang Shi <shiyongbang@huawei.com>
Cc: Baihan Li <libaihan@huawei.com>
Cc: <stable@vger.kernel.org> # v4.10+
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index 8fa2a95bcdd1..c4f9ebd9250d 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -118,10 +118,8 @@ static void hibmc_plane_atomic_update(struct drm_plane *plane,
 }
 
 static const u32 channel_formats1[] = {
-	DRM_FORMAT_RGB565, DRM_FORMAT_BGR565, DRM_FORMAT_RGB888,
-	DRM_FORMAT_BGR888, DRM_FORMAT_XRGB8888, DRM_FORMAT_XBGR8888,
-	DRM_FORMAT_RGBA8888, DRM_FORMAT_BGRA8888, DRM_FORMAT_ARGB8888,
-	DRM_FORMAT_ABGR8888
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_RGB565,
 };
 
 static const struct drm_plane_funcs hibmc_plane_funcs = {
-- 
2.53.0

Re: [PATCH 2/4] drm/hibmc: Fix list of formats on the primary plane

[Yongbang Shi]

> Remove all formats from the primary plane that are unsupported for
> various reasons.
> 
> * Formats with alpha channel: planes should not announce alpha channels
> unless they support transparency. There's no transparency support in
> the primary plane's implementation.
> 
> * Formats with BGR order. The common format is in RGB channel order.
> There's no BGR support in the primary plane's implementation.
> 
> * RGB888: atomic_update programs the format from cpp[0] * 8 / 16. For
> RGB888's cpp value of 3 this returns 1.5; rounded to 1. Programming
> the value of 1 to HIBMC_CRT_DISP_CTL_FORMAT sets up RGB565. Hence, the
> output is distorted. This can be tested by booting with video=1024x768-24.
> 

I tested this method and was able to reproduce the issue. Thank you for
pointing it out.


> Removing all unsupported formats leaves XRGB8888 and RGB565. Both of
> which are supported and work correctly.
> 

You're right, I checked the DataSheet and confirmed that it only
supports RGB565 and XRGB8888.

Also, I looked at the commit history, and this format was already
present in the first version of the hibmc driver. This is a historic
issue, and it's a bit difficult to determine why it was implemented this
way.


> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: da52605eea8f ("drm/hisilicon/hibmc: Add support for display engine")
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Xinliang Liu <xinliang.liu@linaro.org>
> Cc: Dmitry Baryshkov <lumag@kernel.org>
> Cc: Yongbang Shi <shiyongbang@huawei.com>
> Cc: Baihan Li <libaihan@huawei.com>
> Cc: <stable@vger.kernel.org> # v4.10+
> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> index 8fa2a95bcdd1..c4f9ebd9250d 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> @@ -118,10 +118,8 @@ static void hibmc_plane_atomic_update(struct drm_plane *plane,
>  }
>  
>  static const u32 channel_formats1[] = {
> -	DRM_FORMAT_RGB565, DRM_FORMAT_BGR565, DRM_FORMAT_RGB888,
> -	DRM_FORMAT_BGR888, DRM_FORMAT_XRGB8888, DRM_FORMAT_XBGR8888,
> -	DRM_FORMAT_RGBA8888, DRM_FORMAT_BGRA8888, DRM_FORMAT_ARGB8888,
> -	DRM_FORMAT_ABGR8888
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_RGB565,
>  };
>  
>  static const struct drm_plane_funcs hibmc_plane_funcs = {

Reviewed-by: Yongbang Shi <shiyongbang@huawei.com>

Re: [PATCH 2/4] drm/hibmc: Fix list of formats on the primary plane

[Thomas Zimmermann]

Hi

Am 15.04.26 um 05:19 schrieb Yongbang Shi:
[...]
> Also, I looked at the commit history, and this format was already
> present in the first version of the hibmc driver. This is a historic
> issue, and it's a bit difficult to determine why it was implemented this
> way.

It's probably left over from the prototype. Back then, DRM was also less 
well structured and it might have been harder to see what's really needed.

>>
>> Reviewed-by: Yongbang Shi <shiyongbang@huawei.com>

Thanks.

Best regards
Thomas

>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH 1/4] drm/hibmc: Use drm_atomic_helper_check_plane_state()

[Yongbang Shi]

> Call drm_atomic_helper_check_plane_state() from the primary plane's
> atomic-check helper and replace the custom implementation.
> 
> All plane's implementations of atomic_check should call the shared
> _check_plane_state() helper first. It adjusts the plane state for
> correct positioning, rotation and scaling of the plane. If the plane
> is not visible, it clears the corresponding flag in the plane state.
> 
> On errors or if the plane is not visible, the atomic-check helper can
> return early. Implement all this in hibmc and drop the custom code
> that does some of it.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: da52605eea8f ("drm/hisilicon/hibmc: Add support for display engine")
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Xinliang Liu <xinliang.liu@linaro.org>
> Cc: Dmitry Baryshkov <lumag@kernel.org>
> Cc: Baihan Li <libaihan@huawei.com>
> Cc: Yongbang Shi <shiyongbang@huawei.com>
> Cc: <stable@vger.kernel.org> # v4.10+
> ---
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    | 46 ++++++-------------
>  1 file changed, 14 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> index 89bed78f1466..8fa2a95bcdd1 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> @@ -55,46 +55,28 @@ static const struct hibmc_dislay_pll_config hibmc_pll_table[] = {
>  static int hibmc_plane_atomic_check(struct drm_plane *plane,
>  				    struct drm_atomic_state *state)
>  {
> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
> -										 plane);
> -	struct drm_framebuffer *fb = new_plane_state->fb;
> -	struct drm_crtc *crtc = new_plane_state->crtc;
> -	struct drm_crtc_state *crtc_state;
> -	u32 src_w = new_plane_state->src_w >> 16;
> -	u32 src_h = new_plane_state->src_h >> 16;
> -
> -	if (!crtc || !fb)
> -		return 0;
> +	struct drm_plane_state *new_plane_state =
> +		drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_crtc_state *new_crtc_state = NULL;
> +	int ret;
>  
> -	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -	if (IS_ERR(crtc_state))
> -		return PTR_ERR(crtc_state);
> +	if (new_plane_state->crtc)
> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
>  
> -	if (src_w != new_plane_state->crtc_w || src_h != new_plane_state->crtc_h) {
> -		drm_dbg_atomic(plane->dev, "scale not support\n");
> -		return -EINVAL;
> -	}
> -
> -	if (new_plane_state->crtc_x < 0 || new_plane_state->crtc_y < 0) {
> -		drm_dbg_atomic(plane->dev, "crtc_x/y of drm_plane state is invalid\n");
> -		return -EINVAL;
> -	}
> -
> -	if (!crtc_state->enable)
> +	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> +						  DRM_PLANE_NO_SCALING,
> +						  DRM_PLANE_NO_SCALING,
> +						  false, true);

The last parameter, "can_update_disabled", if set to true, causes the
condition "if (!crtc_state->enable && !can_update_disabled)" in the
function `drm_atomic_helper_check_plane_state` to always evaluate to
false, meaning `crtc_state->enable` will not be checked. This differs
from the behavior prior to the modification.

before:
	- crtc_state->enable(true)  --> continue check
	- crtc_state->enable(false) --> return 0(atomic check success)

after:
	- crtc_state->enable(true)   --> _helper_check_plane_ ->  continue check
	- crtc_state->enable(false)  --> _helper_check_plane_ ->  continue check


> +	if (ret)
> +		return ret;
> +	else if (!new_plane_state->visible)
>  		return 0;
>  
> -	if (new_plane_state->crtc_x + new_plane_state->crtc_w >
> -	    crtc_state->adjusted_mode.hdisplay ||
> -	    new_plane_state->crtc_y + new_plane_state->crtc_h >
> -	    crtc_state->adjusted_mode.vdisplay) {
> -		drm_dbg_atomic(plane->dev, "visible portion of plane is invalid\n");
> -		return -EINVAL;
> -	}
> -

The purpose of this check is to ensure that the right and bottom
boundaries of the plane do not extend beyond the crtc. In
`drm_atomic_helper_check_plane_state`, `drm_mode_get_hv_timing` is
called to retrieve the crtc boundaries, and `drm_rect_clip_scaled` is
used to clip the plane, any portions extending beyond the right and
bottom boundaries are discarded.

I'd like to confirm that my understanding is correct? previously, the
check failed if the plane exceeded the boundaries, but now, after
`drm_atomic_helper_check_plane_state` is called, the plane is clipped to
fit within the boundaries.

in function drm_rect_clip_scaled:

diff = dst->x2 - clip->x2;
if (diff > 0) {
	...
	dst->x2 -= diff;
}
diff = dst->y2 - clip->y2;
if (diff > 0) {
	...
	dst->y2 -= diff;
}


Thanks.

Best regards
Yongbang.


>  	if (new_plane_state->fb->pitches[0] % 128 != 0) {
>  		drm_dbg_atomic(plane->dev, "wrong stride with 128-byte aligned\n");
>  		return -EINVAL;
>  	}
> +
>  	return 0;
>  }
>  

Re: [PATCH 1/4] drm/hibmc: Use drm_atomic_helper_check_plane_state()

[Thomas Zimmermann]

Hi

Am 16.04.26 um 08:53 schrieb Yongbang Shi:
>> Call drm_atomic_helper_check_plane_state() from the primary plane's
>> atomic-check helper and replace the custom implementation.
>>
>> All plane's implementations of atomic_check should call the shared
>> _check_plane_state() helper first. It adjusts the plane state for
>> correct positioning, rotation and scaling of the plane. If the plane
>> is not visible, it clears the corresponding flag in the plane state.
>>
>> On errors or if the plane is not visible, the atomic-check helper can
>> return early. Implement all this in hibmc and drop the custom code
>> that does some of it.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: da52605eea8f ("drm/hisilicon/hibmc: Add support for display engine")
>> Cc: Rongrong Zou <zourongrong@gmail.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: Xinliang Liu <xinliang.liu@linaro.org>
>> Cc: Dmitry Baryshkov <lumag@kernel.org>
>> Cc: Baihan Li <libaihan@huawei.com>
>> Cc: Yongbang Shi <shiyongbang@huawei.com>
>> Cc: <stable@vger.kernel.org> # v4.10+
>> ---
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    | 46 ++++++-------------
>>   1 file changed, 14 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> index 89bed78f1466..8fa2a95bcdd1 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> @@ -55,46 +55,28 @@ static const struct hibmc_dislay_pll_config hibmc_pll_table[] = {
>>   static int hibmc_plane_atomic_check(struct drm_plane *plane,
>>   				    struct drm_atomic_state *state)
>>   {
>> -	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
>> -										 plane);
>> -	struct drm_framebuffer *fb = new_plane_state->fb;
>> -	struct drm_crtc *crtc = new_plane_state->crtc;
>> -	struct drm_crtc_state *crtc_state;
>> -	u32 src_w = new_plane_state->src_w >> 16;
>> -	u32 src_h = new_plane_state->src_h >> 16;
>> -
>> -	if (!crtc || !fb)
>> -		return 0;
>> +	struct drm_plane_state *new_plane_state =
>> +		drm_atomic_get_new_plane_state(state, plane);
>> +	struct drm_crtc_state *new_crtc_state = NULL;
>> +	int ret;
>>   
>> -	crtc_state = drm_atomic_get_crtc_state(state, crtc);
>> -	if (IS_ERR(crtc_state))
>> -		return PTR_ERR(crtc_state);
>> +	if (new_plane_state->crtc)
>> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
>>   
>> -	if (src_w != new_plane_state->crtc_w || src_h != new_plane_state->crtc_h) {
>> -		drm_dbg_atomic(plane->dev, "scale not support\n");
>> -		return -EINVAL;
>> -	}
>> -
>> -	if (new_plane_state->crtc_x < 0 || new_plane_state->crtc_y < 0) {
>> -		drm_dbg_atomic(plane->dev, "crtc_x/y of drm_plane state is invalid\n");
>> -		return -EINVAL;
>> -	}
>> -
>> -	if (!crtc_state->enable)
>> +	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> +						  DRM_PLANE_NO_SCALING,
>> +						  DRM_PLANE_NO_SCALING,
>> +						  false, true);
> The last parameter, "can_update_disabled", if set to true, causes the
> condition "if (!crtc_state->enable && !can_update_disabled)" in the
> function `drm_atomic_helper_check_plane_state` to always evaluate to
> false, meaning `crtc_state->enable` will not be checked. This differs
> from the behavior prior to the modification.
>
> before:
> 	- crtc_state->enable(true)  --> continue check
> 	- crtc_state->enable(false) --> return 0(atomic check success)
>
> after:
> 	- crtc_state->enable(true)   --> _helper_check_plane_ ->  continue check
> 	- crtc_state->enable(false)  --> _helper_check_plane_ ->  continue check

Isn't this what the hardware supports? The plane's hardware registers 
can be updated even if the plane's CRTC is off?

In the old case, atomic check returned success. If we set 
can_update_disable to false, it would return an error in such as case.  
Settings it ti true keeps the success for disabled CRTCs. The semantics 
of the returned value don't change. It's just that the helper fills a 
few more fields in drm_plane_state.



>
>
>> +	if (ret)
>> +		return ret;
>> +	else if (!new_plane_state->visible)
>>   		return 0;
>>   
>> -	if (new_plane_state->crtc_x + new_plane_state->crtc_w >
>> -	    crtc_state->adjusted_mode.hdisplay ||
>> -	    new_plane_state->crtc_y + new_plane_state->crtc_h >
>> -	    crtc_state->adjusted_mode.vdisplay) {
>> -		drm_dbg_atomic(plane->dev, "visible portion of plane is invalid\n");
>> -		return -EINVAL;
>> -	}
>> -
> The purpose of this check is to ensure that the right and bottom
> boundaries of the plane do not extend beyond the crtc. In
> `drm_atomic_helper_check_plane_state`, `drm_mode_get_hv_timing` is
> called to retrieve the crtc boundaries, and `drm_rect_clip_scaled` is
> used to clip the plane, any portions extending beyond the right and
> bottom boundaries are discarded.
>
> I'd like to confirm that my understanding is correct? previously, the
> check failed if the plane exceeded the boundaries, but now, after
> `drm_atomic_helper_check_plane_state` is called, the plane is clipped to
> fit within the boundaries.

Yes. This sets plane_state->dst, which is clipped to the size of the 
display mode. But it also tests that the primary plane covers the whole 
display.

>
> in function drm_rect_clip_scaled:
>
> diff = dst->x2 - clip->x2;
> if (diff > 0) {
> 	...
> 	dst->x2 -= diff;
> }
> diff = dst->y2 - clip->y2;
> if (diff > 0) {
> 	...
> 	dst->y2 -= diff;
> }

I agree, the logic in drm_atomic_helper_check_plane_state() is hard to 
understand. It sets the clip rectangle to the size of the display mode 
(or zero if the CRTC is off) at [1].  Then is clips the source and 
destination coordinates against the clipping rectangle at [2].

Because we set can_position to false, it tests if the destination and 
clipping rectangles are equal at [3]. This is similar to the that is 
being replaced, but with plane state correctly adjusted. If both 
rectangles are equal, it returns success. If the destination is too 
small, it fails with an errno code and a warning.

If the plane is not visible, the helper already returned at [4].

[1] 
https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/drm_atomic_helper.c#L943
[2] 
https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/drm_atomic_helper.c#L945
[3] 
https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/drm_atomic_helper.c#L959
[4] 
https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/drm_atomic_helper.c#L949

It is now possible to have a primary plane that is larger than the 
display mode. This is a feature of the DRM API.

Best regards
Thomas


>
>
> Thanks.
>
> Best regards
> Yongbang.
>
>
>>   	if (new_plane_state->fb->pitches[0] % 128 != 0) {
>>   		drm_dbg_atomic(plane->dev, "wrong stride with 128-byte aligned\n");
>>   		return -EINVAL;
>>   	}
>> +
>>   	return 0;
>>   }
>>   

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH 1/4] drm/hibmc: Use drm_atomic_helper_check_plane_state()

[Yongbang Shi]

> Hi
> 
> Am 16.04.26 um 08:53 schrieb Yongbang Shi:
>>> Call drm_atomic_helper_check_plane_state() from the primary plane's
>>> atomic-check helper and replace the custom implementation.
>>>
>>> All plane's implementations of atomic_check should call the shared
>>> _check_plane_state() helper first. It adjusts the plane state for
>>> correct positioning, rotation and scaling of the plane. If the plane
>>> is not visible, it clears the corresponding flag in the plane state.
>>>
>>> On errors or if the plane is not visible, the atomic-check helper can
>>> return early. Implement all this in hibmc and drop the custom code
>>> that does some of it.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Fixes: da52605eea8f ("drm/hisilicon/hibmc: Add support for display
>>> engine")
>>> Cc: Rongrong Zou <zourongrong@gmail.com>
>>> Cc: Sean Paul <seanpaul@chromium.org>
>>> Cc: Xinliang Liu <xinliang.liu@linaro.org>
>>> Cc: Dmitry Baryshkov <lumag@kernel.org>
>>> Cc: Baihan Li <libaihan@huawei.com>
>>> Cc: Yongbang Shi <shiyongbang@huawei.com>
>>> Cc: <stable@vger.kernel.org> # v4.10+
>>> ---
>>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    | 46 ++++++-------------
>>>   1 file changed, 14 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/
>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> index 89bed78f1466..8fa2a95bcdd1 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> @@ -55,46 +55,28 @@ static const struct hibmc_dislay_pll_config
>>> hibmc_pll_table[] = {
>>>   static int hibmc_plane_atomic_check(struct drm_plane *plane,
>>>                       struct drm_atomic_state *state)
>>>   {
>>> -    struct drm_plane_state *new_plane_state =
>>> drm_atomic_get_new_plane_state(state,
>>> -                                         plane);
>>> -    struct drm_framebuffer *fb = new_plane_state->fb;
>>> -    struct drm_crtc *crtc = new_plane_state->crtc;
>>> -    struct drm_crtc_state *crtc_state;
>>> -    u32 src_w = new_plane_state->src_w >> 16;
>>> -    u32 src_h = new_plane_state->src_h >> 16;
>>> -
>>> -    if (!crtc || !fb)
>>> -        return 0;
>>> +    struct drm_plane_state *new_plane_state =
>>> +        drm_atomic_get_new_plane_state(state, plane);
>>> +    struct drm_crtc_state *new_crtc_state = NULL;
>>> +    int ret;
>>>   -    crtc_state = drm_atomic_get_crtc_state(state, crtc);
>>> -    if (IS_ERR(crtc_state))
>>> -        return PTR_ERR(crtc_state);
>>> +    if (new_plane_state->crtc)
>>> +        new_crtc_state = drm_atomic_get_new_crtc_state(state,
>>> new_plane_state->crtc);
>>>   -    if (src_w != new_plane_state->crtc_w || src_h !=
>>> new_plane_state->crtc_h) {
>>> -        drm_dbg_atomic(plane->dev, "scale not support\n");
>>> -        return -EINVAL;
>>> -    }
>>> -
>>> -    if (new_plane_state->crtc_x < 0 || new_plane_state->crtc_y < 0) {
>>> -        drm_dbg_atomic(plane->dev, "crtc_x/y of drm_plane state is
>>> invalid\n");
>>> -        return -EINVAL;
>>> -    }
>>> -
>>> -    if (!crtc_state->enable)
>>> +    ret = drm_atomic_helper_check_plane_state(new_plane_state,
>>> new_crtc_state,
>>> +                          DRM_PLANE_NO_SCALING,
>>> +                          DRM_PLANE_NO_SCALING,
>>> +                          false, true);
>> The last parameter, "can_update_disabled", if set to true, causes the
>> condition "if (!crtc_state->enable && !can_update_disabled)" in the
>> function `drm_atomic_helper_check_plane_state` to always evaluate to
>> false, meaning `crtc_state->enable` will not be checked. This differs
>> from the behavior prior to the modification.
>>
>> before:
>>     - crtc_state->enable(true)  --> continue check
>>     - crtc_state->enable(false) --> return 0(atomic check success)
>>
>> after:
>>     - crtc_state->enable(true)   --> _helper_check_plane_ ->  continue
>> check
>>     - crtc_state->enable(false)  --> _helper_check_plane_ ->  continue
>> check
> 
> Isn't this what the hardware supports? The plane's hardware registers
> can be updated even if the plane's CRTC is off?
> 

Sure, The plane's hardware registers could be update after disable
plane's crtc.

> In the old case, atomic check returned success. If we set
> can_update_disable to false, it would return an error in such as case. 
> Settings it ti true keeps the success for disabled CRTCs. The semantics
> of the returned value don't change. It's just that the helper fills a
> few more fields in drm_plane_state.
> 

Yes, can_update_disable couldn't be false.

I just wanted to confirm this change. There is a slight difference
between the original code and the modified version. In the original
code, at location [1], it returns "check success" directly without
checking the subsequent "new_plane_state->fb->pitches[0]" at [2],
whereas the modified version continues to perform the full check.

However, I believe the modified version is the better implementation,
the plane check should be complete even if the CRTC is not enabled.

[1]
https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c#L84
[2]
https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c#L94


> 
> 
>>
>>
>>> +    if (ret)
>>> +        return ret;
>>> +    else if (!new_plane_state->visible)
>>>           return 0;
>>>   -    if (new_plane_state->crtc_x + new_plane_state->crtc_w >
>>> -        crtc_state->adjusted_mode.hdisplay ||
>>> -        new_plane_state->crtc_y + new_plane_state->crtc_h >
>>> -        crtc_state->adjusted_mode.vdisplay) {
>>> -        drm_dbg_atomic(plane->dev, "visible portion of plane is
>>> invalid\n");
>>> -        return -EINVAL;
>>> -    }
>>> -
>> The purpose of this check is to ensure that the right and bottom
>> boundaries of the plane do not extend beyond the crtc. In
>> `drm_atomic_helper_check_plane_state`, `drm_mode_get_hv_timing` is
>> called to retrieve the crtc boundaries, and `drm_rect_clip_scaled` is
>> used to clip the plane, any portions extending beyond the right and
>> bottom boundaries are discarded.
>>
>> I'd like to confirm that my understanding is correct? previously, the
>> check failed if the plane exceeded the boundaries, but now, after
>> `drm_atomic_helper_check_plane_state` is called, the plane is clipped to
>> fit within the boundaries.
> 
> Yes. This sets plane_state->dst, which is clipped to the size of the
> display mode. But it also tests that the primary plane covers the whole
> display.
> 
>>
>> in function drm_rect_clip_scaled:
>>
>> diff = dst->x2 - clip->x2;
>> if (diff > 0) {
>>     ...
>>     dst->x2 -= diff;
>> }
>> diff = dst->y2 - clip->y2;
>> if (diff > 0) {
>>     ...
>>     dst->y2 -= diff;
>> }
> 
> I agree, the logic in drm_atomic_helper_check_plane_state() is hard to
> understand. It sets the clip rectangle to the size of the display mode
> (or zero if the CRTC is off) at [1].  Then is clips the source and
> destination coordinates against the clipping rectangle at [2].
> 
> Because we set can_position to false, it tests if the destination and
> clipping rectangles are equal at [3]. This is similar to the that is
> being replaced, but with plane state correctly adjusted. If both
> rectangles are equal, it returns success. If the destination is too
> small, it fails with an errno code and a warning.
> 
> If the plane is not visible, the helper already returned at [4].
> 
> [1] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
> drm_atomic_helper.c#L943
> [2] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
> drm_atomic_helper.c#L945
> [3] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
> drm_atomic_helper.c#L959
> [4] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
> drm_atomic_helper.c#L949
> 
> It is now possible to have a primary plane that is larger than the
> display mode. This is a feature of the DRM API.
> 

Thank you so much for the explanation. It's much clearer to understand
drm_atomic_helper_check_plane_state.

By the way, we ran some basic tests on this set of patches using the
latest BMC chip, and everything worked fine.


Thanks.

Best regards
Yongbang.


> Best regards
> Thomas
> 
> 
>>
>>
>> Thanks.
>>
>> Best regards
>> Yongbang.
>>
>>
>>>       if (new_plane_state->fb->pitches[0] % 128 != 0) {
>>>           drm_dbg_atomic(plane->dev, "wrong stride with 128-byte
>>> aligned\n");
>>>           return -EINVAL;
>>>       }
>>> +
>>>       return 0;
>>>   }
>>>   
> 

Reviewed-by: Yongbang Shi <shiyongbang@huawei.com>

Re: [PATCH 1/4] drm/hibmc: Use drm_atomic_helper_check_plane_state()

[Thomas Zimmermann]

Hi

Am 16.04.26 um 15:22 schrieb Yongbang Shi:
[...]
> However, I believe the modified version is the better implementation,
> the plane check should be complete even if the CRTC is not enabled.

Great.

I agree that the logic in the helper is non-intuitive. I'll send you an 
updated series with an improved commit message next week.

Best regards
Thomas

>
> [1]
> https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c#L84
> [2]
> https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c#L94
>
>
>>
>>>
>>>> +    if (ret)
>>>> +        return ret;
>>>> +    else if (!new_plane_state->visible)
>>>>            return 0;
>>>>    -    if (new_plane_state->crtc_x + new_plane_state->crtc_w >
>>>> -        crtc_state->adjusted_mode.hdisplay ||
>>>> -        new_plane_state->crtc_y + new_plane_state->crtc_h >
>>>> -        crtc_state->adjusted_mode.vdisplay) {
>>>> -        drm_dbg_atomic(plane->dev, "visible portion of plane is
>>>> invalid\n");
>>>> -        return -EINVAL;
>>>> -    }
>>>> -
>>> The purpose of this check is to ensure that the right and bottom
>>> boundaries of the plane do not extend beyond the crtc. In
>>> `drm_atomic_helper_check_plane_state`, `drm_mode_get_hv_timing` is
>>> called to retrieve the crtc boundaries, and `drm_rect_clip_scaled` is
>>> used to clip the plane, any portions extending beyond the right and
>>> bottom boundaries are discarded.
>>>
>>> I'd like to confirm that my understanding is correct? previously, the
>>> check failed if the plane exceeded the boundaries, but now, after
>>> `drm_atomic_helper_check_plane_state` is called, the plane is clipped to
>>> fit within the boundaries.
>> Yes. This sets plane_state->dst, which is clipped to the size of the
>> display mode. But it also tests that the primary plane covers the whole
>> display.
>>
>>> in function drm_rect_clip_scaled:
>>>
>>> diff = dst->x2 - clip->x2;
>>> if (diff > 0) {
>>>      ...
>>>      dst->x2 -= diff;
>>> }
>>> diff = dst->y2 - clip->y2;
>>> if (diff > 0) {
>>>      ...
>>>      dst->y2 -= diff;
>>> }
>> I agree, the logic in drm_atomic_helper_check_plane_state() is hard to
>> understand. It sets the clip rectangle to the size of the display mode
>> (or zero if the CRTC is off) at [1].  Then is clips the source and
>> destination coordinates against the clipping rectangle at [2].
>>
>> Because we set can_position to false, it tests if the destination and
>> clipping rectangles are equal at [3]. This is similar to the that is
>> being replaced, but with plane state correctly adjusted. If both
>> rectangles are equal, it returns success. If the destination is too
>> small, it fails with an errno code and a warning.
>>
>> If the plane is not visible, the helper already returned at [4].
>>
>> [1] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
>> drm_atomic_helper.c#L943
>> [2] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
>> drm_atomic_helper.c#L945
>> [3] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
>> drm_atomic_helper.c#L959
>> [4] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
>> drm_atomic_helper.c#L949
>>
>> It is now possible to have a primary plane that is larger than the
>> display mode. This is a feature of the DRM API.
>>
> Thank you so much for the explanation. It's much clearer to understand
> drm_atomic_helper_check_plane_state.
>
> By the way, we ran some basic tests on this set of patches using the
> latest BMC chip, and everything worked fine.
>
>
> Thanks.
>
> Best regards
> Yongbang.
>
>
>> Best regards
>> Thomas
>>
>>
>>>
>>> Thanks.
>>>
>>> Best regards
>>> Yongbang.
>>>
>>>
>>>>        if (new_plane_state->fb->pitches[0] % 128 != 0) {
>>>>            drm_dbg_atomic(plane->dev, "wrong stride with 128-byte
>>>> aligned\n");
>>>>            return -EINVAL;
>>>>        }
>>>> +
>>>>        return 0;
>>>>    }
>>>>    
> Reviewed-by: Yongbang Shi <shiyongbang@huawei.com>
>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH 1/4] drm/hibmc: Use drm_atomic_helper_check_plane_state()

[Yongbang Shi]

> Hi
> 
> Am 16.04.26 um 15:22 schrieb Yongbang Shi:
> [...]
>> However, I believe the modified version is the better implementation,
>> the plane check should be complete even if the CRTC is not enabled.
> 
> Great.
> 
> I agree that the logic in the helper is non-intuitive. I'll send you an
> updated series with an improved commit message next week.
> 

Looking forward to the updated series next week, and I appreciate the
improved commit message as well.

Thanks,
Yongbang.

> Best regards
> Thomas
> 
>>
>> [1]
>> https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
>> hisilicon/hibmc/hibmc_drm_de.c#L84
>> [2]
>> https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
>> hisilicon/hibmc/hibmc_drm_de.c#L94
>>
>>
>>>
>>>>
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +    else if (!new_plane_state->visible)
>>>>>            return 0;
>>>>>    -    if (new_plane_state->crtc_x + new_plane_state->crtc_w >
>>>>> -        crtc_state->adjusted_mode.hdisplay ||
>>>>> -        new_plane_state->crtc_y + new_plane_state->crtc_h >
>>>>> -        crtc_state->adjusted_mode.vdisplay) {
>>>>> -        drm_dbg_atomic(plane->dev, "visible portion of plane is
>>>>> invalid\n");
>>>>> -        return -EINVAL;
>>>>> -    }
>>>>> -
>>>> The purpose of this check is to ensure that the right and bottom
>>>> boundaries of the plane do not extend beyond the crtc. In
>>>> `drm_atomic_helper_check_plane_state`, `drm_mode_get_hv_timing` is
>>>> called to retrieve the crtc boundaries, and `drm_rect_clip_scaled` is
>>>> used to clip the plane, any portions extending beyond the right and
>>>> bottom boundaries are discarded.
>>>>
>>>> I'd like to confirm that my understanding is correct? previously, the
>>>> check failed if the plane exceeded the boundaries, but now, after
>>>> `drm_atomic_helper_check_plane_state` is called, the plane is
>>>> clipped to
>>>> fit within the boundaries.
>>> Yes. This sets plane_state->dst, which is clipped to the size of the
>>> display mode. But it also tests that the primary plane covers the whole
>>> display.
>>>
>>>> in function drm_rect_clip_scaled:
>>>>
>>>> diff = dst->x2 - clip->x2;
>>>> if (diff > 0) {
>>>>      ...
>>>>      dst->x2 -= diff;
>>>> }
>>>> diff = dst->y2 - clip->y2;
>>>> if (diff > 0) {
>>>>      ...
>>>>      dst->y2 -= diff;
>>>> }
>>> I agree, the logic in drm_atomic_helper_check_plane_state() is hard to
>>> understand. It sets the clip rectangle to the size of the display mode
>>> (or zero if the CRTC is off) at [1].  Then is clips the source and
>>> destination coordinates against the clipping rectangle at [2].
>>>
>>> Because we set can_position to false, it tests if the destination and
>>> clipping rectangles are equal at [3]. This is similar to the that is
>>> being replaced, but with plane state correctly adjusted. If both
>>> rectangles are equal, it returns success. If the destination is too
>>> small, it fails with an errno code and a warning.
>>>
>>> If the plane is not visible, the helper already returned at [4].
>>>
>>> [1] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
>>> drm_atomic_helper.c#L943
>>> [2] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
>>> drm_atomic_helper.c#L945
>>> [3] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
>>> drm_atomic_helper.c#L959
>>> [4] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/
>>> drm_atomic_helper.c#L949
>>>
>>> It is now possible to have a primary plane that is larger than the
>>> display mode. This is a feature of the DRM API.
>>>
>> Thank you so much for the explanation. It's much clearer to understand
>> drm_atomic_helper_check_plane_state.
>>
>> By the way, we ran some basic tests on this set of patches using the
>> latest BMC chip, and everything worked fine.
>>
>>
>> Thanks.
>>
>> Best regards
>> Yongbang.
>>
>>
>>> Best regards
>>> Thomas
>>>
>>>
>>>>
>>>> Thanks.
>>>>
>>>> Best regards
>>>> Yongbang.
>>>>
>>>>
>>>>>        if (new_plane_state->fb->pitches[0] % 128 != 0) {
>>>>>            drm_dbg_atomic(plane->dev, "wrong stride with 128-byte
>>>>> aligned\n");
>>>>>            return -EINVAL;
>>>>>        }
>>>>> +
>>>>>        return 0;
>>>>>    }
>>>>>    
>> Reviewed-by: Yongbang Shi <shiyongbang@huawei.com>
>>
>>
>