[Mesa-dev] [PATCH 12/34] i965: Handle Y-tile modifier
Ben Widawsky
ben at bwidawsk.net
Mon Feb 6 04:51:13 UTC 2017
On 17-01-31 12:10:11, Jason Ekstrand wrote:
>On Mon, Jan 23, 2017 at 10:21 PM, Ben Widawsky <ben at 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 <ben at bwidawsk.net>
>> Date: Thu Apr 21 20:14:58 2016 -0700
>>
>> i965: Always use Y-tiled buffers on SKL+
>>
>> Signed-off-by: Ben Widawsky <benjamin.widawsky at intel.com>
>> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
>> Acked-by: Daniel Stone <daniels at 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 ebfa74a8ff..6eaf146181 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>
>> @@ -559,16 +560,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 */
>>
>
>Is 32 an API limitation? Only looking locally, it seems to just be a
>limitation of the algorithm used here for picking modifiers. Given that
>the algorithm can be easily extended if needed, I see no reason why we need
>to make it an API limitation.
>
It was a limitation before at the request of Rob, but that's since been removed
and this has been changed locally.
>
>>
>> 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");
>>
>
>Why? We can support Y tiling on all our hardware, you just can't scan it
>out prior to Sky Lake. However, if you were doing mesa <-> mesa for
>compositing, Y-tiling is perfectly fine all the way back.
>
>
The intended usage of modifiers is the client will query the per plane
modifiers via GET_PLANE2, and only use the modifiers provided. As such, we
should never have a case where this is being called on gen < 9. The modifier is
an "FB modifier" and so really only applies to a framebuffer, and therefore gen
< 9 makes sense.
In thinking about this further though, as of now, you can get to this point
without necessarily creating a framebuffer. I think it's reasonable to assume
that if you're using a modifier, the buffer can be scanned out from, and perhaps
I just need to embellish this warning.
>> + continue;
>> + }
>> +
>> + modifier_bitmask |= YTILE;
>> + break;
>> }
>> }
>>
>> - return modifier;
>> + if (modifier_bitmask)
>> + return prio_modifiers[ffsll(modifier_bitmask)-1];
>> +
>> + return DRM_FORMAT_MOD_INVALID;
>> +
>> +#undef LINEAR
>> +#undef YTILE
>> }
>>
>> static __DRIimage *
>> @@ -599,6 +619,9 @@ __intel_create_image(__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;
>> @@ -650,8 +673,26 @@ intel_create_image_with_modifiers(__DRIscreen
>> *dri_screen,
>> const unsigned count,
>> void *loaderPrivate)
>> {
>> - return __intel_create_image(dri_screen, width, height, format, 0,
>> NULL, 0,
>> - loaderPrivate);
>> + uint64_t local_mods[count];
>>
>
>Should we really be stack allocating this? It comes in from the user and
>could be arbitrarily large. It's probably not a problem, but this patch
>got me thinking about modifier counts.
>
>
Fine for now IMO. The number of modifiers is going to stay relatively low in the
foreseeable future, and we can change it when we have a problem. Unlike the
kernel, these kinds of problems are easy to catch.
>> + 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");
>> + default:
>> + /* Modifiers from other vendors would land here. */
>> + break;
>> + }
>> + }
>> +
>> + return __intel_create_image(dri_screen, width, height, format, 0,
>> + local_mods, local_count, loaderPrivate);
>> }
>>
>> static GLboolean
>> @@ -1912,7 +1953,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.11.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
More information about the mesa-dev
mailing list