Re: [Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout
On Wed, Feb 14, 2018 at 9:42 AM, Ville Syrjälä < ville.syrj...@linux.intel.com> wrote: > On Wed, Feb 14, 2018 at 04:43:15PM +, Daniel Stone wrote: > > On 14 February 2018 at 16:27, Jason Ekstrand > wrote: > > > On Wed, Feb 14, 2018 at 3:39 AM, Daniel Stone > wrote: > > >> > On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone < > dan...@fooishbar.org> wrote: > > >> >> For actual scanout, the kernel very specifically demands that if > the > > >> >> BO is X-tiled, then set_tiling be called with TILING_X. This > applies > > >> >> even if we explicitly allocate with MOD_X_TILED and pass that in > via > > >> >> drmModeAddFB2WithModifiers: there is an explicit check for > > >> >> !!(bo_tiling == TILING_X) == !!(modifier == MOD_X_TILED). > > >> > > >> You missed this bit. Suggested fixup: https://hastebin.com/xikopobiza > > > > > > I was really hoping I'd read wrong. I'm going with "that's a kernel > bug". > > Old kernels required TILING_X with MOD_X. How many kernels is that? > I changed that at some > point to allow TILING_NONE with any modifier, but otherwise we > require the BO tiling to match the modifier. Ie. you still can't > mix TILING_Y + MOD_X for example. > Right. That's perfectly reasonable. It should be either TILING_NONE or match. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout
On Wed, Feb 14, 2018 at 04:43:15PM +, Daniel Stone wrote: > On 14 February 2018 at 16:27, Jason Ekstrand wrote: > > On Wed, Feb 14, 2018 at 3:39 AM, Daniel Stone wrote: > >> > On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone > >> > wrote: > >> >> For actual scanout, the kernel very specifically demands that if the > >> >> BO is X-tiled, then set_tiling be called with TILING_X. This applies > >> >> even if we explicitly allocate with MOD_X_TILED and pass that in via > >> >> drmModeAddFB2WithModifiers: there is an explicit check for > >> >> !!(bo_tiling == TILING_X) == !!(modifier == MOD_X_TILED). > >> > >> You missed this bit. Suggested fixup: https://hastebin.com/xikopobiza > > > > I was really hoping I'd read wrong. I'm going with "that's a kernel bug". Old kernels required TILING_X with MOD_X. I changed that at some point to allow TILING_NONE with any modifier, but otherwise we require the BO tiling to match the modifier. Ie. you still can't mix TILING_Y + MOD_X for example. -- Ville Syrjälä Intel OTC ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout
On 14 February 2018 at 17:23, Jason Ekstrand wrote: > On Wed, Feb 14, 2018 at 8:43 AM, Daniel Stone wrote: >> Linear is unchanged as it's implicit. X tiling has to take the >> workaround, in case anyone wants to display it. > > I'm not sure I buy that. From a userspace perspective, you shouldn't be > using modifiers unless everything supports them. Why? Because no userspace > that isn't an Intel driver should look at I915_FORMAT_MOD_X_TILED and say "I > can hand that off without modifiers just fine". Take a sprite plane which only advertises LINEAR + X_TILED at the moment (watermark allocation bugs seem to prevent Y_TILED). Hand that set of two modifiers to Vulkan for allocation, render into the resulting image. Call AddFB on it with I915_FORMAT_MOD_X_TILED. That will get rejected because set_tiling hasn't been called, even though you _have_ been explicit with modifiers everywhere. >> I can kind of see why that ABI makes sense though. Having userspace >> explicitly call set_tiling(X) had the kernel magically imply that the >> FB created from that BO should be X-tiled. Given that ABI still >> unfortunately exists, enforcing coherency between old and new ways to >> declare an FB should be tiled, doesn't seem unreasonable. > > Eh. The whole point of modifiers is as an alternate mechanism. Insisting > that they "match" is, to me, equivalent to insisting that you use both which > is silly. Of course, anyone is free to argue in the opposite direction. *shrug*. Looked at another way, it's just about having your implicit and explicit mechanisms match exactly. Given the implicit mechanism will stay around forever, keeping them consistent makes sure that no-one slip up. Either way, we're stuck with it forever, and if we ever want to display X_TILED then we need it, so ... Cheers, Daniel ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout
On Wed, Feb 14, 2018 at 8:43 AM, Daniel Stone wrote: > On 14 February 2018 at 16:27, Jason Ekstrand wrote: > > On Wed, Feb 14, 2018 at 3:39 AM, Daniel Stone > wrote: > >> > On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone > wrote: > >> >> For actual scanout, the kernel very specifically demands that if the > >> >> BO is X-tiled, then set_tiling be called with TILING_X. This applies > >> >> even if we explicitly allocate with MOD_X_TILED and pass that in via > >> >> drmModeAddFB2WithModifiers: there is an explicit check for > >> >> !!(bo_tiling == TILING_X) == !!(modifier == MOD_X_TILED). > >> > >> You missed this bit. Suggested fixup: https://hastebin.com/xikopobiza > > > > I was really hoping I'd read wrong. I'm going with "that's a kernel > bug". > > Modifiers is supposed to separate tiling from the BO. Tying them back > > together in drmModeAddFB2WithModifiers is wrong. This is especially true > > since SET_TILING is an inherently racy operation and we really need to > stop > > using it entirely. > > > > If what you wrote above really is immutably true, then this patch is > dead. > > It is immutably true, but I think this patch (with ugly fixup) still > makes sense. > That's unfortunate. Yeah, I agree that we should only set_tiling in the minimum case. > Linear is unchanged as it's implicit. X tiling has to take the > workaround, in case anyone wants to display it. I'm not sure I buy that. From a userspace perspective, you shouldn't be using modifiers unless everything supports them. Why? Because no userspace that isn't an Intel driver should look at I915_FORMAT_MOD_X_TILED and say "I can hand that off without modifiers just fine". > Y tiling can skip it > though, and future Yf/etc tiling modes don't need to either extend the > I915_TILING_* enum (eww), or be explicitly set to linear when they're > not. I think there's value in having the code clarify, rather than it > doing set_tiling everywhere, so people assuming it's still required. > Agreed. We should set the minimum amount of tiling. > I can kind of see why that ABI makes sense though. Having userspace > explicitly call set_tiling(X) had the kernel magically imply that the > FB created from that BO should be X-tiled. Given that ABI still > unfortunately exists, enforcing coherency between old and new ways to > declare an FB should be tiled, doesn't seem unreasonable. > Eh. The whole point of modifiers is as an alternate mechanism. Insisting that they "match" is, to me, equivalent to insisting that you use both which is silly. Of course, anyone is free to argue in the opposite direction. > > What's the failure mode here? We just don't flip? Or the compositor > wigs > > out and crashes? > > AddFB never succeeds, so we can never flip. > Ok. That helps. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout
On 14 February 2018 at 16:27, Jason Ekstrand wrote: > On Wed, Feb 14, 2018 at 3:39 AM, Daniel Stone wrote: >> > On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone >> > wrote: >> >> For actual scanout, the kernel very specifically demands that if the >> >> BO is X-tiled, then set_tiling be called with TILING_X. This applies >> >> even if we explicitly allocate with MOD_X_TILED and pass that in via >> >> drmModeAddFB2WithModifiers: there is an explicit check for >> >> !!(bo_tiling == TILING_X) == !!(modifier == MOD_X_TILED). >> >> You missed this bit. Suggested fixup: https://hastebin.com/xikopobiza > > I was really hoping I'd read wrong. I'm going with "that's a kernel bug". > Modifiers is supposed to separate tiling from the BO. Tying them back > together in drmModeAddFB2WithModifiers is wrong. This is especially true > since SET_TILING is an inherently racy operation and we really need to stop > using it entirely. > > If what you wrote above really is immutably true, then this patch is dead. It is immutably true, but I think this patch (with ugly fixup) still makes sense. Linear is unchanged as it's implicit. X tiling has to take the workaround, in case anyone wants to display it. Y tiling can skip it though, and future Yf/etc tiling modes don't need to either extend the I915_TILING_* enum (eww), or be explicitly set to linear when they're not. I think there's value in having the code clarify, rather than it doing set_tiling everywhere, so people assuming it's still required. I can kind of see why that ABI makes sense though. Having userspace explicitly call set_tiling(X) had the kernel magically imply that the FB created from that BO should be X-tiled. Given that ABI still unfortunately exists, enforcing coherency between old and new ways to declare an FB should be tiled, doesn't seem unreasonable. > What's the failure mode here? We just don't flip? Or the compositor wigs > out and crashes? AddFB never succeeds, so we can never flip. Cheers, Daniel ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout
On Wed, Feb 14, 2018 at 3:39 AM, Daniel Stone wrote: > Hi, > > On 13 February 2018 at 22:15, Jason Ekstrand wrote: > > On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone > wrote: > >> On 9 February 2018 at 23:43, Jason Ekstrand > wrote: > >> For actual scanout, the kernel very specifically demands that if the > >> BO is X-tiled, then set_tiling be called with TILING_X. This applies > >> even if we explicitly allocate with MOD_X_TILED and pass that in via > >> drmModeAddFB2WithModifiers: there is an explicit check for > >> !!(bo_tiling == TILING_X) == !!(modifier == MOD_X_TILED). > > You missed this bit. Suggested fixup: https://hastebin.com/xikopobiza > I was really hoping I'd read wrong. I'm going with "that's a kernel bug". Modifiers is supposed to separate tiling from the BO. Tying them back together in drmModeAddFB2WithModifiers is wrong. This is especially true since SET_TILING is an inherently racy operation and we really need to stop using it entirely. If what you wrote above really is immutably true, then this patch is dead. What's the failure mode here? We just don't flip? Or the compositor wigs out and crashes? --Jason ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout
On 14 February 2018 at 11:39, Daniel Stone wrote: > On 13 February 2018 at 22:15, Jason Ekstrand wrote: >> On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone wrote: >>> On 9 February 2018 at 23:43, Jason Ekstrand wrote: >>> For actual scanout, the kernel very specifically demands that if the >>> BO is X-tiled, then set_tiling be called with TILING_X. This applies >>> even if we explicitly allocate with MOD_X_TILED and pass that in via >>> drmModeAddFB2WithModifiers: there is an explicit check for >>> !!(bo_tiling == TILING_X) == !!(modifier == MOD_X_TILED). > > You missed this bit. Suggested fixup: https://hastebin.com/xikopobiza Er, except it should be (1 << b) everywhere, not b. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout
Hi, On 13 February 2018 at 22:15, Jason Ekstrand wrote: > On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone wrote: >> On 9 February 2018 at 23:43, Jason Ekstrand wrote: >> For actual scanout, the kernel very specifically demands that if the >> BO is X-tiled, then set_tiling be called with TILING_X. This applies >> even if we explicitly allocate with MOD_X_TILED and pass that in via >> drmModeAddFB2WithModifiers: there is an explicit check for >> !!(bo_tiling == TILING_X) == !!(modifier == MOD_X_TILED). You missed this bit. Suggested fixup: https://hastebin.com/xikopobiza Cheers, Daniel ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout
On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone wrote: > Hi Jason, > > On 9 February 2018 at 23:43, Jason Ekstrand wrote: > > - /* For images using modifiers, we require a dedicated > allocation > > - * and we set the BO tiling to match the tiling of the > underlying > > - * modifier. This is a bit unfortunate as this is completely > > - * pointless for Vulkan. However, GL needs to be able to map > things > > - * so it needs the tiling to be set. The only way to do this > in a > > - * non-racy way is to set the tiling in the creator of the BO > so that > > - * makes it our job. > > - * > > - * One of these days, once the GL driver learns to not map > things > > - * through the GTT in random places, we can drop this and start > > - * allowing multiple modified images in the same BO. > > + /* For legacy scanout images, no tiling information is passed > along > > + * directly. Instead, consumers pull the tiling from BO. > >*/ > > - if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) { > > -assert(isl_drm_modifier_get_info(image->drm_format_mod)->tiling > == > > - image->planes[0].surface.isl.tiling); > > + if (image->legacy_scanout) { > > Hm. There's a couple of things here. There's no-modifier protocols > like DRI2, current DRI3 (see patches to improve this), and wl_drm > which don't carry modifier information. For those, we set the tiling > so the importer can know what we've done. This is either linear or > X-tiled, and is more legacy-winsys than legacy-scanout. > > For actual scanout, the kernel very specifically demands that if the > BO is X-tiled, then set_tiling be called with TILING_X. This applies > even if we explicitly allocate with MOD_X_TILED and pass that in via > drmModeAddFB2WithModifiers: there is an explicit check for > !!(bo_tiling == TILING_X) == !!(modifier == MOD_X_TILED). > > I think this would be better called image->needs_set_tiling or > similar, which enforced dedicated allocation and a set_tiling call, > and was called if we have no modifier information, _or_ if the buffer > is X-tiled. > Yeah, image->needs_set_tiling seems reasonable to me. I'll make the change. > (Is there a port of vkcube to the new modifier bits somewhere?) > > > @@ -2217,7 +2206,7 @@ void anv_GetImageMemoryRequirements2KHR( > >switch (ext->sType) { > >case VK_STRUCTURE_TYPE_MEMORY_DEDICATED_REQUIREMENTS_KHR: { > > VkMemoryDedicatedRequirementsKHR *requirements = (void *)ext; > > - if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) { > > + if (image->legacy_scanout) { > > /* Require a dedicated allocation for images with modifiers. > > Also this comment is stale. > Yup. > > + /** True if this is a legacy scanout image */ > > + bool legacy_scanout; > > The comment in the header could also perhaps go a brief way to > explaining the bear trap outlined above. > I've written the following: /** True if this is needs to be bound to an appropriately tiled BO. * * When not using modifiers, consumers such as X11, Wayland, and KMS need * the tiling passed via I915_GEM_SET_TILING. When exporting these buffers * we require a dedicated allocation so that we can know to allocate a * tiled buffer. */ ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout
Hi Jason, On 9 February 2018 at 23:43, Jason Ekstrand wrote: > - /* For images using modifiers, we require a dedicated allocation > - * and we set the BO tiling to match the tiling of the underlying > - * modifier. This is a bit unfortunate as this is completely > - * pointless for Vulkan. However, GL needs to be able to map things > - * so it needs the tiling to be set. The only way to do this in a > - * non-racy way is to set the tiling in the creator of the BO so > that > - * makes it our job. > - * > - * One of these days, once the GL driver learns to not map things > - * through the GTT in random places, we can drop this and start > - * allowing multiple modified images in the same BO. > + /* For legacy scanout images, no tiling information is passed along > + * directly. Instead, consumers pull the tiling from BO. >*/ > - if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) { > -assert(isl_drm_modifier_get_info(image->drm_format_mod)->tiling > == > - image->planes[0].surface.isl.tiling); > + if (image->legacy_scanout) { Hm. There's a couple of things here. There's no-modifier protocols like DRI2, current DRI3 (see patches to improve this), and wl_drm which don't carry modifier information. For those, we set the tiling so the importer can know what we've done. This is either linear or X-tiled, and is more legacy-winsys than legacy-scanout. For actual scanout, the kernel very specifically demands that if the BO is X-tiled, then set_tiling be called with TILING_X. This applies even if we explicitly allocate with MOD_X_TILED and pass that in via drmModeAddFB2WithModifiers: there is an explicit check for !!(bo_tiling == TILING_X) == !!(modifier == MOD_X_TILED). I think this would be better called image->needs_set_tiling or similar, which enforced dedicated allocation and a set_tiling call, and was called if we have no modifier information, _or_ if the buffer is X-tiled. (Is there a port of vkcube to the new modifier bits somewhere?) > @@ -2217,7 +2206,7 @@ void anv_GetImageMemoryRequirements2KHR( >switch (ext->sType) { >case VK_STRUCTURE_TYPE_MEMORY_DEDICATED_REQUIREMENTS_KHR: { > VkMemoryDedicatedRequirementsKHR *requirements = (void *)ext; > - if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) { > + if (image->legacy_scanout) { > /* Require a dedicated allocation for images with modifiers. Also this comment is stale. > + /** True if this is a legacy scanout image */ > + bool legacy_scanout; The comment in the header could also perhaps go a brief way to explaining the bear trap outlined above. Rest looks good though. Cheers, Daniel ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout
For a bit there, we had a bug in i965 where it ignored the tiling of the modifier and used the one from the BO instead. At one point, we though this was best fixed by setting a tiling from Vulkan. However, we've decided that i965 was just doing the wrong thing and have fixed it as of 50485723523d2948a44570ba110f02f726f86a54. The old assumptions also affected the solution we used for legacy scanout in Vulkan. Instead of treating it specially, we just treated it like a modifier like we do in GL. This commit goes back to making it it's own thing so that it's clear in the driver when we're using modifiers and when we're using legacy paths. --- src/intel/vulkan/anv_device.c | 19 --- src/intel/vulkan/anv_image.c | 27 --- src/intel/vulkan/anv_private.h | 6 ++ 3 files changed, 14 insertions(+), 38 deletions(-) diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c index 86c1bdc..30f4159 100644 --- a/src/intel/vulkan/anv_device.c +++ b/src/intel/vulkan/anv_device.c @@ -1835,21 +1835,10 @@ VkResult anv_AllocateMemory( if (dedicated_info && dedicated_info->image != VK_NULL_HANDLE) { ANV_FROM_HANDLE(anv_image, image, dedicated_info->image); - /* For images using modifiers, we require a dedicated allocation - * and we set the BO tiling to match the tiling of the underlying - * modifier. This is a bit unfortunate as this is completely - * pointless for Vulkan. However, GL needs to be able to map things - * so it needs the tiling to be set. The only way to do this in a - * non-racy way is to set the tiling in the creator of the BO so that - * makes it our job. - * - * One of these days, once the GL driver learns to not map things - * through the GTT in random places, we can drop this and start - * allowing multiple modified images in the same BO. + /* For legacy scanout images, no tiling information is passed along + * directly. Instead, consumers pull the tiling from BO. */ - if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) { -assert(isl_drm_modifier_get_info(image->drm_format_mod)->tiling == - image->planes[0].surface.isl.tiling); + if (image->legacy_scanout) { const uint32_t i915_tiling = isl_tiling_to_i915_tiling(image->planes[0].surface.isl.tiling); int ret = anv_gem_set_tiling(device, mem->bo->gem_handle, @@ -2217,7 +2206,7 @@ void anv_GetImageMemoryRequirements2KHR( switch (ext->sType) { case VK_STRUCTURE_TYPE_MEMORY_DEDICATED_REQUIREMENTS_KHR: { VkMemoryDedicatedRequirementsKHR *requirements = (void *)ext; - if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) { + if (image->legacy_scanout) { /* Require a dedicated allocation for images with modifiers. * * See also anv_AllocateMemory. diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c index a297cc4..355aff0 100644 --- a/src/intel/vulkan/anv_image.c +++ b/src/intel/vulkan/anv_image.c @@ -93,7 +93,8 @@ choose_isl_surf_usage(VkImageCreateFlags vk_create_flags, static isl_tiling_flags_t choose_isl_tiling_flags(const struct anv_image_create_info *anv_info, -const struct isl_drm_modifier_info *isl_mod_info) +const struct isl_drm_modifier_info *isl_mod_info, +bool legacy_scanout) { const VkImageCreateInfo *base_info = anv_info->vk_info; isl_tiling_flags_t flags = 0; @@ -112,6 +113,9 @@ choose_isl_tiling_flags(const struct anv_image_create_info *anv_info, if (anv_info->isl_tiling_flags) flags &= anv_info->isl_tiling_flags; + if (legacy_scanout) + flags &= ISL_TILING_LINEAR_BIT | ISL_TILING_X_BIT; + if (isl_mod_info) flags &= 1 << isl_mod_info->tiling; @@ -504,19 +508,6 @@ make_surface(const struct anv_device *dev, return VK_SUCCESS; } -static const struct isl_drm_modifier_info * -get_legacy_scanout_drm_format_mod(VkImageTiling tiling) -{ - switch (tiling) { - case VK_IMAGE_TILING_OPTIMAL: - return isl_drm_modifier_get_info(I915_FORMAT_MOD_X_TILED); - case VK_IMAGE_TILING_LINEAR: - return isl_drm_modifier_get_info(DRM_FORMAT_MOD_LINEAR); - default: - unreachable("bad VkImageTiling"); - } -} - VkResult anv_image_create(VkDevice _device, const struct anv_image_create_info *create_info, @@ -533,8 +524,6 @@ anv_image_create(VkDevice _device, const struct wsi_image_create_info *wsi_info = vk_find_struct_const(pCreateInfo->pNext, WSI_IMAGE_CREATE_INFO_MESA); - if (wsi_info && wsi_info->scanout) - isl_mod_info = get_legacy_scanout_drm_format_mod(pCreateInfo->tiling); anv_assert(pCreateInfo->mipLevels > 0); anv_assert(pCreateInfo->arrayLayers > 0); @@ -559,14 +548,14 @