Re: [PATCH v3] drm/mediatek: Add AFBC support to Mediatek DRM driver

[Daniel Stone <daniel@xxxxxxxxxxxxx>]

Hi Justin,

On Wed, 12 Oct 2022 at 20:12, Justin Green <greenjustin@xxxxxxxxxxxx> wrote:

@@ -226,6 +249,32 @@ int mtk_ovl_layer_check(struct device *dev, unsigned int idx,
        if (state->fb->format->is_yuv && rotation != 0)
                return -EINVAL;

+       if (state->fb->modifier) {

Please spell this out explicitly as DRM_FORMAT_MOD_LINEAR. For some reason, we specify that 0 is explicitly block-linear, whilst INVALID ('unknown layout, figure it out by magic') is a non-zero value. So != 0 isn't a check for 'was a modifier explicitly specified', even if != 0 is almost always 'is this buffer non-linear'.

+               unsigned long long modifier;
+               unsigned int fourcc;

u64, u32, but ...

 

+               if (!ovl->data->supports_afbc)
+                       return -EINVAL;
+
+               modifier = state->fb->modifier;
+
+               if (modifier != DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
+                                                       AFBC_FORMAT_MOD_SPLIT |
+                                                       AFBC_FORMAT_MOD_SPARSE))
+                       return -EINVAL;
+
+               fourcc = state->fb->format->format;
+               if (fourcc != DRM_FORMAT_BGRA8888 &&
+                   fourcc != DRM_FORMAT_ABGR8888 &&
+                   fourcc != DRM_FORMAT_ARGB8888 &&
+                   fourcc != DRM_FORMAT_XRGB8888 &&
+                   fourcc != DRM_FORMAT_XBGR8888 &&
+                   fourcc != DRM_FORMAT_RGB888 &&
+                   fourcc != DRM_FORMAT_BGR888)
+                       return -EINVAL;

The core shouldn't allow a framebuffer to be created with a format/modifier pair which wasn't advertised, so these checks can be dropped. Except that it's never advertised.

mtk_plane_init() passes a set of formats and modifiers to drm_universal_plane_init(); the AFBC modifier needs to be added here, as well as LINEAR and INVALID. Then you'll need to set the format_mod_supported() hook on the plane to filter out format/modifier pairs. After that, you should see (e.g. with drm_info) that you get an IN_FORMATS blob which advertises DRM_FORMAT_MOD_ARM_AFBC(...) as well as linear for DRM_FORMAT_XRGB8888, but only linear for DRM_FORMAT_RGB565.

After doing that, you should see framebuffer creation fail for unsupported pairings, e.g. RGB565 + AFBC.

 

 +       bool is_afbc = pending->modifier;

... != DRM_FORMAT_MOD_LINEAR

 

@@ -52,6 +53,7 @@ static void mtk_plane_reset(struct drm_plane *plane)

        state->base.plane = plane;
        state->pending.format = DRM_FORMAT_RGB565;
+       state->pending.modifier = 0;

= DRM_FORMAT_MOD_LINEAR

 

@@ -120,21 +122,52 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
        struct drm_gem_object *gem;
        struct mtk_drm_gem_obj *mtk_gem;
        unsigned int pitch, format;
+       unsigned long long modifier;

u64

 

+       if (!modifier) {

modifier == DRM_FORMAT_MOD_LINEAR

 

Cheers,

Daniel