On Fri, Mar 17, 2017 at 5:34 PM, Ben Widawsky <b...@bwidawsk.net> wrote:
> This patch begins introducing how we'll actually handle the potentially
> many modifiers coming in from the API, how we'll store them, and the
> structure in the code to support it.
>
> Prior to this patch, the Y-tiled modifier would be entirely ignored. It
> shouldn't actually be used until this point because we've not bumped the
> DRIimage extension version (which is a requirement to use modifiers).
>
> With X-tiling:
> Writes: 6,583.58 MiB
> Reads: 6,540.93 MiB
>
> With Y-tiling:
> Writes: 5,361.78 MiB
> Reads 6,052.45 MiB
>
> Savings per frame
> Writes: 2 MiB
> Reads: .8 MiB
>
> Similar functionality was introduced and then reverted here:
>
> commit 6a0d036483caf87d43ebe2edd1905873446c9589
> Author: Ben Widawsky <b...@bwidawsk.net>
> Date: Thu Apr 21 20:14:58 2016 -0700
>
> i965: Always use Y-tiled buffers on SKL+
>
> v2: Use last set bit instead of first set bit in modifiers to address
> bug found by Daniel Stone.
>
> Signed-off-by: Ben Widawsky <benjamin.widaw...@intel.com>
> Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>
> Acked-by: Daniel Stone <dani...@collabora.com>
> ---
> src/mesa/drivers/dri/i965/intel_screen.c | 55
> ++++++++++++++++++++++++++++----
> 1 file changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
> b/src/mesa/drivers/dri/i965/intel_screen.c
> index 22ab3a30b6..1954757d1e 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -23,6 +23,7 @@
> * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> */
>
> +#include <drm_fourcc.h>
> #include <errno.h>
> #include <time.h>
> #include <unistd.h>
> @@ -520,16 +521,35 @@ select_best_modifier(struct gen_device_info *devinfo,
> const uint64_t *modifiers,
> const unsigned count)
> {
> - uint64_t modifier = DRM_FORMAT_MOD_INVALID;
> +#define YTILE (1 << 1)
> +#define LINEAR (1 << 0)
> +
> + const uint64_t prio_modifiers[] = { I915_FORMAT_MOD_Y_TILED,
> DRM_FORMAT_MOD_LINEAR };
> + uint32_t modifier_bitmask = 0; /* API only allows 32 */
>
The bitfield thing is still confusing to me. Here's an idea on how we
could maybe make it better.
enum modifier_priority {
MODIFIER_PRIORITY_LINEAR,
MODIFIER_PRIORITY_X,
MODIFIER_PRIORITY_Y,
MODIFIER_PRIORITY_Y_CCS,
};
uint32_t priority_to_modifier[] = {
[MODIFIER_PRIORITY_LINEAR] = DRM_FORMAT_MOD_LINEAR,
[MODIFIER_PRIORITY_X] = I915_FORMAT_MOD_X_TILED,
[MODIFIER_PRIORITY_Y] = I915_FORMAT_MOD_Y_TILED,
[MODIFIER_PRIORITY_Y_CCS] = I915_FORMAT_MOD_Y_TILED_CCS,
}
enum modier_priority prio = 0;
for (int i = 0; i < count; i++) {
switch (modifiers[i]) {
case DRM_FORMAT_MOD_LINEAR:
prio = MAX2(prio, MODIFIER_PRIORITY_LINEAR);
break;
case DRM_FORMAT_MOD_X_TILED:
prio = MAX2(prio, MODIFIER_PRIORITY_X);
break;
case DRM_FORMAT_MOD_Y_TILED:
prio = MAX2(prio, MODIFIER_PRIORITY_Y);
break;
case DRM_FORMAT_MOD_Y_TILIED_CCS:
prio = MAX2(prio, MODIFIER_PRIORITY_Y_CCS);
break;
}
return priority_to_modifier[prio];
How does this strike your fancy? I'm ok with the bit set approach if you
really prefer it but I find it hard to reason about.
>
> for (int i = 0; i < count; i++) {
> switch (modifiers[i]) {
> case DRM_FORMAT_MOD_LINEAR:
> - return modifiers[i];
> + modifier_bitmask |= LINEAR;
> + break;
> + case I915_FORMAT_MOD_Y_TILED:
> + if (devinfo->gen < 9) {
> + _mesa_warning(NULL, "Invalid Y-tiling parameter\n");
> + continue;
>
This isn't invalid. It's just invalid for scanout. If you wanted to
create an image to share between two GL implementations, Y-tiling works
fine on everything.
> + }
> +
> + modifier_bitmask |= YTILE;
> + break;
> }
> }
>
> - return modifier;
> + if (modifier_bitmask)
> + return prio_modifiers[util_last_bit64(modifier_bitmask)-1];
> +
> + return DRM_FORMAT_MOD_INVALID;
> +
> +#undef LINEAR
> +#undef YTILE
> }
>
> static __DRIimage *
> @@ -560,6 +580,9 @@ intel_create_image_common(__DRIscreen *dri_screen,
> case DRM_FORMAT_MOD_LINEAR:
> tiling = I915_TILING_NONE;
> break;
> + case I915_FORMAT_MOD_Y_TILED:
> + tiling = I915_TILING_Y;
> + break;
> case DRM_FORMAT_MOD_INVALID:
> default:
> break;
> @@ -611,8 +634,26 @@ intel_create_image_with_modifiers(__DRIscreen
> *dri_screen,
> const unsigned count,
> void *loaderPrivate)
> {
> - return intel_create_image_common(dri_screen, width, height, format,
> 0, NULL,
> - 0, loaderPrivate);
> + uint64_t local_mods[count];
> + int local_count = 0;
> +
> + /* This compacts the actual modifiers to the ones we know how to
> handle */
> + for (int i = 0; i < count; i++) {
> + switch (modifiers[i]) {
> + case I915_FORMAT_MOD_Y_TILED:
> + case DRM_FORMAT_MOD_LINEAR:
> + local_mods[local_count++] = modifiers[i];
> + break;
> + case DRM_FORMAT_MOD_INVALID:
> + unreachable("Invalid modifiers specified\n");
>
I'm not sure this is truely unreachable. What prevents someone from
calling create_image_with_modifiers with INVALID? Do we have a check
somewhere higher up that that prevents it? If this is something that's
actually triggerable from client code, then I think we probably want to
just let it fall through.
> + default:
> + /* Modifiers from other vendors would land here. */
> + break;
> + }
> + }
> +
> + return intel_create_image_common(dri_screen, width, height, format, 0,
> + local_mods, local_count,
> loaderPrivate);
> }
>
> static GLboolean
> @@ -1867,7 +1908,9 @@ intelAllocateBuffer(__DRIscreen *dri_screen,
> if (intelBuffer == NULL)
> return NULL;
>
> - /* The front and back buffers are color buffers, which are X tiled. */
> + /* The front and back buffers are color buffers, which are X tiled.
> GEN9+
> + * supports Y tiled and compressed buffers, but there is no way to
> plumb that
> + * through to here. */
> uint32_t tiling = I915_TILING_X;
> unsigned long pitch;
> int cpp = format / 8;
> --
> 2.12.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
