diff mbox series

[v4] drm/rockchip: Disable AFBC for res >2560 on rk3399

Message ID 20250417065759.5948-1-mail@etehtsea.me (mailing list archive)
State New
Headers show
Series [v4] drm/rockchip: Disable AFBC for res >2560 on rk3399 | expand

Commit Message

Konstantin Shabanov April 17, 2025, 6:57 a.m. UTC 
As it isn't supported by hardware. At least, RK3399 doesn't support
it. From the datasheet[1]
("1.2.10 Video IN/OUT", "Display Interface", p. 17):

  Support AFBC function co-operation with GPU
    * support 2560x1600 UI

Manually tested on RockPro64 (rk3399):
- ARM_AFBC modifier is used for 1920x1080
- DRM_FORMAT_MOD_LINEAR modifier us used for 3840x2160
- No noise on the screen when sway is running in 4k
- Dynamic resolution switching works correctly in sway

Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>
Cc: Daniel Stone <daniel@fooishbar.org>
Cc: Andy Yan <andyshrk@163.com>
Reported-by: Dan Callaghan <djc@djc.id.au>
Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7968

[1]: https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf
---
V3 -> V4: Correct redundant header inclusion
V2 -> V3: Run check only on rk3399
V1 -> V2: Move the check to the fb_create callback

 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)


base-commit: 4890d68db651562ea80250f2c93205a5c0327a6a
--
2.48.1

Comments

Andy Yan April 18, 2025, 12:16 a.m. UTC | #1 
Hi Konstantin,

At 2025-04-17 14:57:58, "Konstantin Shabanov" <mail@etehtsea.me> wrote:
>As it isn't supported by hardware. At least, RK3399 doesn't support
>it. From the datasheet[1]
>("1.2.10 Video IN/OUT", "Display Interface", p. 17):
>
>  Support AFBC function co-operation with GPU
>    * support 2560x1600 UI
>
>Manually tested on RockPro64 (rk3399):
>- ARM_AFBC modifier is used for 1920x1080
>- DRM_FORMAT_MOD_LINEAR modifier us used for 3840x2160
>- No noise on the screen when sway is running in 4k
>- Dynamic resolution switching works correctly in sway
>
>Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>
>Cc: Daniel Stone <daniel@fooishbar.org>
>Cc: Andy Yan <andyshrk@163.com>
>Reported-by: Dan Callaghan <djc@djc.id.au>
>Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7968
>
>[1]: https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf
>---
>V3 -> V4: Correct redundant header inclusion
>V2 -> V3: Run check only on rk3399
>V1 -> V2: Move the check to the fb_create callback
>
> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>index dcc1f07632c3..45e1619b5c97 100644
>--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>@@ -5,6 +5,7 @@
>  */
>
> #include <linux/kernel.h>
>+#include <linux/of.h>
>
> #include <drm/drm.h>
> #include <drm/drm_atomic.h>
>@@ -18,6 +19,8 @@
> #include "rockchip_drm_fb.h"
> #include "rockchip_drm_gem.h"
>
>+#define RK3399_AFBC_MAX_WIDTH		2560
>+
> static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
> 	.destroy       = drm_gem_fb_destroy,
> 	.create_handle = drm_gem_fb_create_handle,
>@@ -52,6 +55,15 @@ rockchip_fb_create(struct drm_device *dev, struct drm_file *file,
> 	}
>
> 	if (drm_is_afbc(mode_cmd->modifier[0])) {
>+		if (of_machine_is_compatible("rockchip,rk3399")) {
>+			if (mode_cmd->width > RK3399_AFBC_MAX_WIDTH) {
>+				DRM_DEBUG_KMS("AFBC is not supported for the width %d (max %d)\n",
>+					      mode_cmd->width,
>+					      RK3399_AFBC_MAX_WIDTH);
>+				return ERR_PTR(-EINVAL);
>+			};
>+		}
>+


I prefer the V1 version PATCH[0]. This is because we do not deal with hardware-related
differences at this level.  It involves a VOP-related restriction and we always  handle
limitiation like this  within the VOP driver . 

[0] https://lore.kernel.org/dri-devel/20250402125320.21836-1-mail@etehtsea.me/


> 		int ret, i;
>
> 		ret = drm_gem_fb_afbc_init(dev, mode_cmd, afbc_fb);
>
>base-commit: 4890d68db651562ea80250f2c93205a5c0327a6a
>--
>2.48.1
Daniel Stone April 18, 2025, 9:43 a.m. UTC | #2 
Hi Andy,

On Fri, 18 Apr 2025 at 01:16, Andy Yan <andyshrk@163.com> wrote:
> I prefer the V1 version PATCH[0]. This is because we do not deal with hardware-related
> differences at this level.  It involves a VOP-related restriction and we always  handle
> limitiation like this  within the VOP driver .

As said in the review for v1, this is not enough for generic userspace.

If drmModeAddFB2WithModifiers() fails, userspace knows that the buffer
can never work in that configuration, and it should fall back. If
drmModeAtomicCommit(DRM_MODE_ATOMIC_TEST_ONLY) fails, userspace does
not know that the buffer can _never_ work, because the failure may be
transient or due to a combination of things.

Returning the more localised error saves userspace a lot of work and
enables a more optimal system.

Cheers,
Daniel
Andy Yan April 18, 2025, 9:50 a.m. UTC | #3 
Hi 
At 2025-04-18 17:43:19, "Daniel Stone" <daniel@fooishbar.org> wrote:
>Hi Andy,
>
>On Fri, 18 Apr 2025 at 01:16, Andy Yan <andyshrk@163.com> wrote:
>> I prefer the V1 version PATCH[0]. This is because we do not deal with hardware-related
>> differences at this level.  It involves a VOP-related restriction and we always  handle
>> limitiation like this  within the VOP driver .
>
>As said in the review for v1, this is not enough for generic userspace.
>
>If drmModeAddFB2WithModifiers() fails, userspace knows that the buffer
>can never work in that configuration, and it should fall back. If
>drmModeAtomicCommit(DRM_MODE_ATOMIC_TEST_ONLY) fails, userspace does
>not know that the buffer can _never_ work, because the failure may be
>transient or due to a combination of things.
>
>Returning the more localised error saves userspace a lot of work and
>enables a more optimal system.

Okay,  fine, ACK.

>
>Cheers,
>Daniel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index dcc1f07632c3..45e1619b5c97 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -5,6 +5,7 @@ 
  */

 #include <linux/kernel.h>
+#include <linux/of.h>

 #include <drm/drm.h>
 #include <drm/drm_atomic.h>
@@ -18,6 +19,8 @@ 
 #include "rockchip_drm_fb.h"
 #include "rockchip_drm_gem.h"

+#define RK3399_AFBC_MAX_WIDTH		2560
+
 static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
 	.destroy       = drm_gem_fb_destroy,
 	.create_handle = drm_gem_fb_create_handle,
@@ -52,6 +55,15 @@  rockchip_fb_create(struct drm_device *dev, struct drm_file *file,
 	}

 	if (drm_is_afbc(mode_cmd->modifier[0])) {
+		if (of_machine_is_compatible("rockchip,rk3399")) {
+			if (mode_cmd->width > RK3399_AFBC_MAX_WIDTH) {
+				DRM_DEBUG_KMS("AFBC is not supported for the width %d (max %d)\n",
+					      mode_cmd->width,
+					      RK3399_AFBC_MAX_WIDTH);
+				return ERR_PTR(-EINVAL);
+			};
+		}
+
 		int ret, i;

 		ret = drm_gem_fb_afbc_init(dev, mode_cmd, afbc_fb);