<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Sun, Feb 5, 2017 at 1:15 PM, Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 17-01-31 12:38:44, Jason Ekstrand wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Mon, Jan 23, 2017 at 10:21 PM, Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote></blockquote> Yes>> NOTE: The extension version is still set to 12, so none of this will<div><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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 <<a href="mailto:daniel@fooishbar.org" target="_blank">daniel@fooishbar.org</a>> Signed-off-by: Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>> Reviewed-by: Eric Engestrom <<a href="mailto:eric.engestrom@imgtec.com" target="_blank">eric.engestrom@imgtec.com</a>> Acked-by: Daniel Stone <<a href="mailto:daniels@collabora.com" target="_blank">daniels@collabora.com</a>> --- 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; </blockquote> Do we want to return the invalid modifier in the error case? I thought 0 was "linear" </blockquote> </div></div> 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? <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + } + + /* Dumb buffers have no modifiers */ + if (!bo->image) + return 0; </blockquote> 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? </blockquote> 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. </blockquote><div> </div><div>That sounds fairly reasonable to me. I guess someone could create a BO with GBM and then call the kernel ioctl to set the tiling mode to X-tiled and then ask what it has. However, short of calling into the driver and having it query the kernel, I don't see a good way to get around that. I think I'd be ok with just returning LINEAR and saying "don't do that". Daniel? </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + 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)); - </blockquote> What's going on here? Is this in the right patch? </blockquote> </div></div> 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).<div><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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; - }; </blockquote> Same here. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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; </blockquote></blockquote> </div></div> D </blockquote></div> </div></div>