On 17-01-31 12:38:44, Jason Ekstrand wrote:
On Mon, Jan 23, 2017 at 10:21 PM, Ben Widawsky <b...@bwidawsk.net> wrote:
Replace the naive, 'save all the modifiers' with a proper query for just
the modifier that was selected. To accomplish this, two new query tokens
are added to the extension:
__DRI_IMAGE_ATTRIB_MODIFIER_UPPER
__DRI_IMAGE_ATTRIB_MODIFIER_LOWER
The query extension only supported 32b queries, and modifiers are 64b,
so we needed two of them.
Yes>> NOTE: The extension version is still set to 12, so none of this will
actually be called.
v2: Use stored modifiers from create instead of queryImage
v3: Make sure not to query modifiers for dumb buffers (Daniel)
Fixed comments in functions.
Cc: Daniel Stone <dan...@fooishbar.org>
Signed-off-by: Ben Widawsky <b...@bwidawsk.net>
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>
Acked-by: Daniel Stone <dani...@collabora.com>
---
src/gbm/backends/dri/gbm_dri.c | 37
++++++++++++++++++++++++--------
src/gbm/gbm-symbols-check | 1 +
src/gbm/main/gbm.c | 19 ++++++++++++++++
src/gbm/main/gbm.h | 3 +++
src/gbm/main/gbmint.h | 5 +----
src/mesa/drivers/dri/i965/intel_screen.c | 6 ++++++
6 files changed, 58 insertions(+), 13 deletions(-)
diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri
.c
index a777f1a984..d5b458aa38 100644
--- a/src/gbm/backends/dri/gbm_dri.c
+++ b/src/gbm/backends/dri/gbm_dri.c
@@ -38,6 +38,7 @@
#include <unistd.h>
#include <dlfcn.h>
#include <xf86drm.h>
+#include <drm_fourcc.h>
#include <GL/gl.h> /* dri_interface needs GL types */
#include <GL/internal/dri_interface.h>
@@ -732,6 +733,32 @@ gbm_dri_bo_get_offset(struct gbm_bo *_bo, int plane)
return (uint32_t)offset;
}
+static uint64_t
+gbm_dri_bo_get_modifier(struct gbm_bo *_bo)
+{
+ struct gbm_dri_device *dri = gbm_dri_device(_bo->gbm);
+ struct gbm_dri_bo *bo = gbm_dri_bo(_bo);
+
+ if (!dri->image || dri->image->base.version < 14) {
+ errno = ENOSYS;
+ return 0;
Do we want to return the invalid modifier in the error case? I thought 0
was "linear"
Introducing the LINEAR modifier (which happened after v2 of this series) did
make things complex because it's possible in some horrific future that a image
doesn't support linear. As a result, you are correct. I think for this case, the
client can handle it pretty easily, and returning INVALID is the right thing to
do.
Daniel, are you okay with changing this to return DRM_FORMAT_MOD_INVALID?
+ }
+
+ /* Dumb buffers have no modifiers */
+ if (!bo->image)
+ return 0;
Same here. I'm not really sure that this is an error, but saying it's
linear might be a lie. I guess this is a static function so maybe it
doesn't matter?
Yeah, this is also a lie but way trickier than the above. Again before this rev
of the series, 0 meant DRM_FORMAT_MOD_NONE, and that was actually legit,
however, now it does mean LINEAR. I believe it's safe to assume that all dumb
BOs are linear, but it should probably be baked in somewhere better. One option
would be to create a proper DRIimage for a dumb BO, but I think the best bet is
to just replace 0 with DRM_FORMAT_MOD_LINEAR.
+
+ uint64_t ret = 0;
+ int mod;
+ dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_MODIFIER_UPPER,
&mod);
+ ret = (uint64_t)mod << 32;
+
+ dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_MODIFIER_LOWER,
&mod);
+ ret |= mod;
+
+ return ret;
+}
+
static void
gbm_dri_bo_destroy(struct gbm_bo *_bo)
{
@@ -1074,15 +1101,6 @@ gbm_dri_bo_create(struct gbm_device *gbm,
if (bo->image == NULL)
goto failed;
- bo->base.base.modifiers = calloc(count, sizeof(*modifiers));
- if (count && !bo->base.base.modifiers) {
- dri->image->destroyImage(bo->image);
- goto failed;
- }
-
- bo->base.base.count = count;
- memcpy(bo->base.base.modifiers, modifiers, count *
sizeof(*modifiers));
-
What's going on here? Is this in the right patch?
Yes, but no. Originally all the modifiers were saved/stored at creation and I
did something with the list at query time. Over time this changed and the
modifier is chosen at creation time (at this point in the series,
__intel_create_image()). As a result, the original code which saves all the
modifiers is bogus and can be deleted. The first logically place to remove this
code is when we return the new modifier, here, but it's all bogus until now
anyway. Essentially, this hunk needs to be squashed into where it was
introduced:
gbm: Introduce modifiers into surface/bo creation
GBM Surface creation still needs this however since there is a more complex
deferred BO allocation there (the surface create API basically does nothing).
dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_HANDLE,
&bo->base.base.handle.s32);
dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_STRIDE,
@@ -1230,6 +1248,7 @@ dri_device_create(int fd)
dri->base.base.bo_get_handle = gbm_dri_bo_get_handle_for_plane;
dri->base.base.bo_get_stride = gbm_dri_bo_get_stride;
dri->base.base.bo_get_offset = gbm_dri_bo_get_offset;
+ dri->base.base.bo_get_modifier = gbm_dri_bo_get_modifier;
dri->base.base.bo_destroy = gbm_dri_bo_destroy;
dri->base.base.destroy = dri_destroy;
dri->base.base.surface_create = gbm_dri_surface_create;
diff --git a/src/gbm/gbm-symbols-check b/src/gbm/gbm-symbols-check
index c137c6cd93..c72fb66b03 100755
--- a/src/gbm/gbm-symbols-check
+++ b/src/gbm/gbm-symbols-check
@@ -23,6 +23,7 @@ gbm_bo_get_handle
gbm_bo_get_fd
gbm_bo_get_plane_count
gbm_bo_get_handle_for_plane
+gbm_bo_get_modifier
gbm_bo_write
gbm_bo_set_user_data
gbm_bo_get_user_data
diff --git a/src/gbm/main/gbm.c b/src/gbm/main/gbm.c
index bfdd009bef..8780b41914 100644
--- a/src/gbm/main/gbm.c
+++ b/src/gbm/main/gbm.c
@@ -280,6 +280,25 @@ gbm_bo_get_handle_for_plane(struct gbm_bo *bo, int
plane)
return bo->gbm->bo_get_handle(bo, plane);
}
+/**
+ * Get the chosen modifier for the buffer object
+ *
+ * This function returns the modifier that was chosen for the object.
These
+ * properties may be generic, or platform/implementation dependent.
+ *
+ * \param bo The buffer object
+ * \return Returns the selected modifier (chosen by the implementation)
for the
+ * BO.
+ * \sa gbm_bo_create_with_modifiers() where possible modifiers are set
+ * \sa gbm_surface_create_with_modifiers() where possible modifiers are
set
+ * \sa define DRM_FORMAT_MOD_* in drm_fourcc.h for possible modifiers
+ */
+GBM_EXPORT uint64_t
+gbm_bo_get_modifier(struct gbm_bo *bo)
+{
+ return bo->gbm->bo_get_modifier(bo);
+}
+
/** Write data into the buffer object
*
* If the buffer object was created with the GBM_BO_USE_WRITE flag,
diff --git a/src/gbm/main/gbm.h b/src/gbm/main/gbm.h
index 39fb9d2d29..b52137ed01 100644
--- a/src/gbm/main/gbm.h
+++ b/src/gbm/main/gbm.h
@@ -327,6 +327,9 @@ gbm_bo_get_handle(struct gbm_bo *bo);
int
gbm_bo_get_fd(struct gbm_bo *bo);
+uint64_t
+gbm_bo_get_modifier(struct gbm_bo *bo);
+
int
gbm_bo_get_plane_count(struct gbm_bo *bo);
diff --git a/src/gbm/main/gbmint.h b/src/gbm/main/gbmint.h
index cd437df021..c27a7a560a 100644
--- a/src/gbm/main/gbmint.h
+++ b/src/gbm/main/gbmint.h
@@ -82,6 +82,7 @@ struct gbm_device {
union gbm_bo_handle (*bo_get_handle)(struct gbm_bo *bo, int plane);
uint32_t (*bo_get_stride)(struct gbm_bo *bo, int plane);
uint32_t (*bo_get_offset)(struct gbm_bo *bo, int plane);
+ uint64_t (*bo_get_modifier)(struct gbm_bo *bo);
void (*bo_destroy)(struct gbm_bo *bo);
struct gbm_surface *(*surface_create)(struct gbm_device *gbm,
@@ -107,10 +108,6 @@ struct gbm_bo {
uint32_t height;
uint32_t stride;
uint32_t format;
- struct {
- uint64_t *modifiers;
- unsigned int count;
- };
Same here.
union gbm_bo_handle handle;
void *user_data;
void (*destroy_user_data)(struct gbm_bo *, void *);
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
b/src/mesa/drivers/dri/i965/intel_screen.c
index eb4f3d7e6b..91b89f0a93 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -743,6 +743,12 @@ intel_query_image(__DRIimage *image, int attrib, int
*value)
case __DRI_IMAGE_ATTRIB_OFFSET:
*value = image->offset;
return true;
+ case __DRI_IMAGE_ATTRIB_MODIFIER_LOWER:
+ *value = (image->modifier & 0xffffffff);
+ return true;
+ case __DRI_IMAGE_ATTRIB_MODIFIER_UPPER:
+ *value = ((image->modifier >> 32) & 0xffffffff);
+ return true;
default:
return false;
D
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
