Hi Justin, On Wed, 12 Oct 2022 at 20:12, Justin Green <greenjus...@chromium.org> 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