On 17-03-20 15:36:37, Jason Ekstrand wrote:
On Mon, Mar 20, 2017 at 3:25 PM, Ben Widawsky <b...@bwidawsk.net> wrote:
On 17-03-20 12:00:44, Jason Ekstrand wrote:
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.
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@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
