[Mesa-dev] [RFC 02/22] intel/isl: Add ISL <-> DRM modifier conversion

[Daniel Stone]

Hi Jason,

On 14 June 2017 at 17:59, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Wed, Jun 14, 2017 at 1:06 AM, Daniel Stone <daniel at fooishbar.org> wrote:
>> Ah, missed this reply. I'll leave that in your hands then and wait for
>> your patches, as well as Chad's ones to update the new protocol in a
>> few days. Happy to review those when they arrive.
>
> For your reading pleasure:
>
> https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=wip/i965-ccs-mod-v3
>
> That contains the isl_drm_modifier_info struct I described above.  I'll be
> sending it to the list once I run kmscube through it to ensure that it
> actually does CCS as advertised.

Everything up to and including 'Use miptree_create_for_dri_image in
image_target_renderbuffer_storage' seems like it could land already.
You can take my R-b for those, assuming they don't do bad things to
CI.

'Add a helper to convert tilings fro ISL to i915': typo
notwithstanding, this is just isl_tiling_to_gem_tiling without an aux
usage. Looking at the KMS patches again though, I don't think
aux_usage is required here: Y_TILED_CCS still maps to I915_TILING_Y,
which is the case I was concerned about (it flip-flopped between KMS
patchset revisions IIRC).

'Add basic modifier introspection': similarly duplicates
isl_tiling_{to,from}_drm_format_mod. I'm not at all precious about
keeping Chad's/my implementation of these rather than yours, but we
already have more than enough duplication of format conversion tables.

'Drop get_tiled_height' should be squashed into the previous patch to
avoid a transient compiler warning.

'Allocate mcs_buf for an image's CCS': In theory, the (mt->offset ==
0) assert could fire with imported dmabuf images. I guess we could
probably only get that with non-dedicated allocations on images
exported from Vulkan? Can't think of any other way to land there which
isn't completely artificial.

'Add a new, simpler, bo_alloc_tiled': it would be nice if this was
refactored so allocations passed anything other than a i915/GEM tiling
mode. Either ISL or format+modifier. Otherwise there'll be no Yf.
Every time I see an i915 tiling flag I feel itchy.

'More conservatively resolve external images': the above comment
suggests moving the part of this commit which sets mt->drm_modifier to
an earlier commit. That could be landed independently, and would be
good to have anyway. As for the rest, as a drive-by comment, it'd be
nice if intel_miptree_prepare_access (and its CCS/HiZ/MCS helpers)
used enums rather than bools. I kept having to track back and check
which bool was which; seems error-prone.

The rest generally looks good to me, and I'm definitely happy to see
ISL, but on the other hand had hoped for more of the format
conversion/lookup code to disappear ... ? Oh well. Seems like a good
step forward anyway.

Cheers,
Daniel