<div dir="ltr"><div>Hi Justin,</div><div dir="ltr"><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 12 Oct 2022 at 20:12, Justin Green <<a href="mailto:greenjustin@chromium.org">greenjustin@chromium.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">@@ -226,6 +249,32 @@ int mtk_ovl_layer_check(struct device *dev, unsigned int idx,<br>
        if (state->fb->format->is_yuv && rotation != 0)<br>
                return -EINVAL;<br>
<br>
+       if (state->fb->modifier) {<br></blockquote><div><br></div><div>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'.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+               unsigned long long modifier;<br>
+               unsigned int fourcc;<br></blockquote><div><br></div><div>u64, u32, but ...</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+               if (!ovl->data->supports_afbc)<br>
+                       return -EINVAL;<br>
+<br>
+               modifier = state->fb->modifier;<br>
+<br>
+               if (modifier != DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |<br>
+                                                       AFBC_FORMAT_MOD_SPLIT |<br>
+                                                       AFBC_FORMAT_MOD_SPARSE))<br>
+                       return -EINVAL;<br>
+<br>
+               fourcc = state->fb->format->format;<br>
+               if (fourcc != DRM_FORMAT_BGRA8888 &&<br>
+                   fourcc != DRM_FORMAT_ABGR8888 &&<br>
+                   fourcc != DRM_FORMAT_ARGB8888 &&<br>
+                   fourcc != DRM_FORMAT_XRGB8888 &&<br>
+                   fourcc != DRM_FORMAT_XBGR8888 &&<br>
+                   fourcc != DRM_FORMAT_RGB888 &&<br>
+                   fourcc != DRM_FORMAT_BGR888)<br>
+                       return -EINVAL;</blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>After doing that, you should see framebuffer creation fail for unsupported pairings, e.g. RGB565 + AFBC.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +       bool is_afbc = pending->modifier;</blockquote><div><br></div><div>... != DRM_FORMAT_MOD_LINEAR</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">@@ -52,6 +53,7 @@ static void mtk_plane_reset(struct drm_plane *plane)<br>
<br>
        state->base.plane = plane;<br>
        state->pending.format = DRM_FORMAT_RGB565;<br>
+       state->pending.modifier = 0;<br></blockquote><div><br></div><div>= DRM_FORMAT_MOD_LINEAR</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">@@ -120,21 +122,52 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,<br>
        struct drm_gem_object *gem;<br>
        struct mtk_drm_gem_obj *mtk_gem;<br>
        unsigned int pitch, format;<br>
+       unsigned long long modifier;<br></blockquote><div><br></div><div>u64</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+       if (!modifier) {<br></blockquote><div><br></div><div>modifier == DRM_FORMAT_MOD_LINEAR</div><div> </div><div>Cheers,</div><div>Daniel</div></div></div>