Hi Jason,
On 14 June 2017 at 17:59, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Wed, Jun 14, 2017 at 1:06 AM, Daniel Stone <dan...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
