[Mesa-dev] [PATCH 3/3] panfrost: Add support for modifiers

[Alyssa Rosenzweig]

> +	switch(whandle->modifier) {
> +	case DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP):
> +		rsc->layout = PAN_AFBC;
> +		break;

Why ROCKCHIP in particular? AFBC fourcc codes are based on modifiers,
and there are a *lot* of them. We need to figure out which upstream
modifier PAN_AFBC actually enables and use that:

	AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | /* certain */
	AFBC_FORMAT_MOD_YTR | /* likely */
	AFBC_FORMAT_MOD_SPARSE | /* possible */

On other AFBC flags, I don't know. I'm also not sure if Arm would chime
in one way or the other.

---

Mainly the issue is that whandle->modifier could contain some other,
non-Rockchip modifier, and then the whole stack collapses; AFBC
modifiers are meant to be parsedd, not =='d. See the komeda driver in
the kernel for reference.

> +static uint64_t
> +panfrost_resource_modifier(struct panfrost_resource *rsrc)
> +{
> +        switch (rsrc->layout) {
> +        case PAN_AFBC:
> +                return DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP);

(Similar issue, although a little lessened)

> +        case PAN_TILED:
> +                return DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED;
> +        case PAN_LINEAR:
> +                return DRM_FORMAT_MOD_LINEAR;
> +        default:
> +                return DRM_FORMAT_MOD_INVALID;
> +        }
> +}
> +
>  static boolean
>  panfrost_resource_get_handle(struct pipe_screen *pscreen,
>                               struct pipe_context *ctx,
> @@ -117,7 +145,7 @@ panfrost_resource_get_handle(struct pipe_screen *pscreen,
>          struct panfrost_resource *rsrc = (struct panfrost_resource *) pt;
>          struct renderonly_scanout *scanout = rsrc->scanout;
>  
> -        handle->modifier = DRM_FORMAT_MOD_INVALID;
> +        handle->modifier = panfrost_resource_modifier(rsrc);
>  
>  	if (handle->type == WINSYS_HANDLE_TYPE_SHARED) {
>  		return FALSE;
> @@ -341,7 +369,9 @@ panfrost_setup_slices(struct panfrost_resource *pres, size_t *bo_size)
>  }
>  

+1

> +        bool can_afbc = panfrost_format_supports_afbc(res->format);
> +        bool want_afbc = drm_find_modifier(DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP), modifiers, count);

(Parse, don't find!)

> +        bool want_linear = drm_find_modifier(DRM_FORMAT_MOD_LINEAR, modifiers, count);

+1

> +	if (want_afbc || (is_renderable && can_afbc && !is_texture))

This doesn't seem quite correct. We preselect AFBC if:

	- AFBC is explicitly requested, OR
	- We're rendering to something that's not bound as a texture
	  that could be AFBC

In case one, what happens if AFBC is explicitly requested but we don't
support AFBC on that format? It should fallback to linear, or error out.

In case two, what if the modifier for linear or tiled is explicitly set?
Now we've given them AFBC when they've asked for something else;
switching it up is only safe for internal BOs.

The logic in this code block has only ever been for internal BOs, where
it's up to *us* to pick a format. Imports are logically distinct, since
it's now up to the modifiers. Two different problems, unless the
modifier is a "don't care".

> +	{"modifiers", PAN_DBG_MODIFIERS,"Enable modifiers support"},

I'm not sure I'm comfortable hiding the support.

> +const uint64_t supported_modifiers[] = {
> +   DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP),
> +   DRM_FORMAT_MOD_LINEAR,
> +};

What happened to u-interleaved?

> +/* TODO: Pick these two up from kernel header */
> +#define AFBC_FORMAT_MOD_ROCKCHIP (1ULL <<  20)

What does this modifier mean?

> +#define DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED \
> +       fourcc_mod_code(ARM, ((1ULL << 55) | 1))

Has this modifier landed? If not, it's not clear that it's format code
is stable; we generally try not to land stuff into mesa's drm_fourcc.h
ahead of the kernel copy.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190705/efb6989a/attachment.sig>