[Mesa-dev] [PATCH 4/5] i965: Handle Y-tiled modifier
Ben Widawsky
ben at bwidawsk.net
Mon Mar 20 22:50:33 UTC 2017
On 17-03-20 15:36:37, Jason Ekstrand wrote:
>On Mon, Mar 20, 2017 at 3:25 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
>
>> On 17-03-20 12:00:44, Jason Ekstrand wrote:
>>
>>> On Fri, Mar 17, 2017 at 5:34 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+
>>>>
>>>> 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.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 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.
>>>
>>>
>> I don't really prefer. This looks pretty good. Seems no less complex to
>> me, but
>> I wrote the first one, so perhaps I'm partial.
>>
>> Originally, I had some code in the equivalent function (before select_best
>> was
>> separate) which would try fallbacks, ie. if Y tiled allocation failed,
>> it'd go
>> down to the next modifier just walking down the bits, but logic is now
>> gone, so
>> there isn't really a point in the bitmask.
>>
>> Will respin with this and the fixes meant below.
>>
>>
>>>
>>>> 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.
>>>
>>>
>>>
>> As a general function to support modifiers, you are correct, however since
>> this
>> is only called for image creation, I believe the existing warning is
>> correct.
>>
>
>But what if I want to create an image to share between two userspace
>processes with no scanout involved? While the GBM portion of the API is
>mostly intended for scanout, the EGL extension will be something that can
>and will be used for GL <-> GL. I guess we can always flip it on when we
>add support for the EGL extension but I see no reason why it shouldn't work
>through GBM.
>
>Part of the problem may be that I really don't understand why GBM exists.
>It's like a linux-specific half-of-EGL thing. :-/
>
>
Yeah, long term I agree, and I've killed it locally. It also means that I
believe (still need to check) the first patch in the series can go away since I
have no use for devinfo anymore.
I'll leave it to someone smarter than me who has been doing this longer to
explain the merits or lack thereof for GBM.
>> + }
>>>> +
>>>> + 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.
>>>
>>>
>>>
>> Nothing. It was meant to be handled in the entry points, but I missed it
>> apparently.
>
>
>If you want to add code to the entrypoints to handle it, and keep the
>unreachable, that's fine with me.
>
>
Yeah, my preference is that INVALID modifier has no business in any actual
implementation. Over time, it's too easy to mess up and end up having a meaning
for INVALID when it should by invalid.
>>
>> + 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 at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>>
>>>>
More information about the mesa-dev
mailing list