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

[Daniel Stone]

Hi Tomeu,

On Thu, 4 Jul 2019 at 09:05, Tomeu Vizoso <tomeu.vizoso at collabora.com> wrote:
> @@ -362,14 +392,19 @@ panfrost_resource_create_bo(struct panfrost_screen *screen, struct panfrost_reso
>
>          /* Tiling textures is almost always faster, unless we only use it once */
>
> +        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);
> +        bool want_linear = drm_find_modifier(DRM_FORMAT_MOD_LINEAR, modifiers, count);
>
> [...]
> -        bool should_tile = !is_streaming && is_texture && is_2d && !is_scanout;
> +        bool should_tile = !is_streaming && is_texture && is_2d && !want_linear && !is_scanout;

I don't think this is right.

The list of modifiers we pass in is the total list of acceptable
modifiers. Clients don't make any judgement or discernment about them,
but just pass in the full list. For example, in Weston we use EGL to
query the list of all modifiers supported by the driver, and then give
those to the client as the list of acceptable modifiers; the EGL
Wayland winsys will then pass that list into DRIImage create.

This means that 'want_linear' will be true as long as Panfrost (or, on
GBM, the KMS driver) supports linear, which realistically is all the
time.

I think ultimately we want should_tile to be (in order of evaluation):
  * definitively false if we can't support it - GPU doesn't support
it, isn't a 2D texture/RT, etc
  * if the above passes, definitively true if the above conditions are
met and we have passed in a modifier set which includes AFBC
  * if we can do it (first test) but we have no modifiers,
definitively false if it's a shared/scanout buffer (i.e. we need to
export the buffer somewhere else but the client isn't modifier-aware),
or definitively true if it's just an internal buffer

That code seems pretty familiar from VC4 (which is in turn probably
from freedreno? everything else is). It could be semi-useful to look
at how VC4 has structured its resource creation decision tree for
modifiers, but on the other hand they also have an implicit fallback
mode; when modifiers aren't available, it will pass the tiling
information through a magic kernel side-channel, which we really don't
want to do here.

Cheers,
Daniel