On Sun, Feb 5, 2017 at 1:15 PM, Ben Widawsky <b...@bwidawsk.net> wrote:
> 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. > 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? --Jason > >> + >>> + 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