On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone <dan...@fooishbar.org> wrote:
> Hi Jason,
>
> On 9 February 2018 at 23:43, Jason Ekstrand <ja...@jlekstrand.net> 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
