<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 12, 2017 at 11:05 AM, Chad Versace <span dir="ltr"><<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu 29 Jun 2017, Jason Ekstrand wrote:<br>
> From: Ben Widawsky <<a href="mailto:ben@bwidawsk.net">ben@bwidawsk.net</a>><br>
><br>
> This code will disable actually creating these buffers for the scanout,<br>
> but it puts the allocation in place.<br>
><br>
> Primarily this patch is split out for review, it can be squashed in<br>
> later if preferred.<br>
><br>
> v2:<br>
> assert(mt->offset == 0) in ccs creation (as requested by Topi)<br>
> Remove bogus is_scanout check in miptree_release<br>
><br>
> v3:<br>
> Remove is_scanout assert in intel_miptree_create. It doesn't work with<br>
> latest codebase - not sure it ever should have worked.<br>
><br>
> v4:<br>
> assert(mt->last_level == 0) and assert(mt->first_level == 0) in ccs setup<br>
> (Topi)<br>
><br>
> v5 (Jason Ekstrand):<br>
> - Base the decision to allocate a CCS on the image modifier<br>
><br>
> Signed-off-by: Ben Widawsky <<a href="mailto:ben@bwidawsk.net">ben@bwidawsk.net</a>><br>
> Acked-by: Daniel Stone <<a href="mailto:daniels@collabora.com">daniels@collabora.com</a>><br>
> Reviewed-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>><br>
> ---<br>
> src/mesa/drivers/dri/i965/<wbr>intel_mipmap_tree.c | 123 +++++++++++++++++++++++---<br>
> 1 file changed, 113 insertions(+), 10 deletions(-)<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/<wbr>intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/<wbr>intel_mipmap_tree.c<br>
> index 575f04f..7a22cbf 100644<br>
> --- a/src/mesa/drivers/dri/i965/<wbr>intel_mipmap_tree.c<br>
> +++ b/src/mesa/drivers/dri/i965/<wbr>intel_mipmap_tree.c<br>
> @@ -59,6 +59,11 @@ intel_miptree_alloc_mcs(struct brw_context *brw,<br>
> struct intel_mipmap_tree *mt,<br>
> GLuint num_samples);<br>
><br>
> +static void<br>
> +intel_miptree_init_mcs(struct brw_context *brw,<br>
> + struct intel_mipmap_tree *mt,<br>
> + int init_value);<br>
> +<br>
> /**<br>
> * Determine which MSAA layout should be used by the MSAA surface being<br>
> * created, based on the chip generation and the surface type.<br>
> @@ -1026,6 +1031,67 @@ miptree_create_for_planar_<wbr>image(struct brw_context *brw,<br>
> return planar_mt;<br>
> }<br>
><br>
> +static bool<br>
> +create_ccs_buf_for_image(<wbr>struct brw_context *brw,<br>
> + __DRIimage *image,<br>
> + struct intel_mipmap_tree *mt,<br>
> + enum isl_aux_state initial_state)<br>
> +{<br>
> + struct isl_surf temp_main_surf, temp_ccs_surf;<br>
> +<br>
> + /* There isn't anything specifically wrong with there being an offset, in<br>
> + * which case, the CCS miptree's offset should be mt->offset +<br>
> + * image->aux_offset. However, the code today only will have an offset when<br>
> + * this miptree is pointing to a slice from another miptree, and in that case<br>
> + * we'd need to offset within the AUX CCS buffer properly. It's questionable<br>
> + * whether our code handles that case properly, and since it can never happen<br>
> + * for scanout, just use the assertion to prevent it.<br>
> + */<br>
> + assert(mt->offset == 0);<br>
> +<br>
> + /* CCS is only supported for very simple miptrees */<br>
> + assert(image->aux_offset && image->aux_pitch);<br>
<br>
</div></div>Why require that aux_offset > 0? Why reject images where the aux surface<br>
precedes the primary surface? This rejection seems arbitrary.</blockquote><div><br></div><div>All throughout these patches image->aux_offset == 0 is used for "no aux". As you pointed out earlier, that's a bit on the bogus side but also always true. For scanout, the hardware requires the aux buffer to be placed after the main surface so it's not *that* bogus.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Please<br>
add a little explanatory comment.<br>
<br>
Also, please use `!= 0` above, to be consistent with the assertions<br>
above and below this one.<br>
<span class=""><br>
> + assert(image->tile_x == 0 && image->tile_y == 0);<br>
> + assert(mt->num_samples <= 1);<br>
> + assert(mt->first_level == 0);<br>
> + assert(mt->last_level == 0);<br>
> + assert(mt->logical_depth0 == 1);<br>
> +<br>
> + /* We shouldn't already have a CCS */<br>
> + assert(!mt->mcs_buf);<br>
> +<br>
> + intel_miptree_get_isl_surf(<wbr>brw, mt, &temp_main_surf);<br>
> + if (!isl_surf_get_ccs_surf(&brw-><wbr>isl_dev, &temp_main_surf, &temp_ccs_surf))<br>
> + return false;<br>
> +<br>
> + assert(temp_ccs_surf.size <= image->bo->size - image->aux_offset);<br>
<br>
</span>This assertion appears invalid, vulnerable to underflow when the<br>
incoming aux_offset is wrong and too large. It needs to be preceded<br>
by assert(image->aux_offset < image->bo->size).<span class=""><br></span></blockquote><div><br></div><div>Sure. I'm happy to add that.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> + assert(temp_ccs_surf.row_pitch <= image->aux_pitch);<br>
> +<br>
> + mt->mcs_buf = calloc(sizeof(*mt->mcs_buf), 1);<br>
> + if (mt->mcs_buf == NULL)<br>
> + return false;<br>
> +<br>
> + mt->aux_state = create_aux_state_map(mt, initial_state);<br>
> + if (!mt->aux_state) {<br>
> + free(mt->mcs_buf);<br>
> + mt->mcs_buf = NULL;<br>
> + return false;<br>
> + }<br>
> +<br>
> + mt->mcs_buf->bo = image->bo;<br>
> + brw_bo_reference(image->bo);<br>
> +<br>
> + mt->mcs_buf->offset = image->aux_offset;<br>
> + mt->mcs_buf->size = image->bo->size - image->aux_offset;<br>
> + mt->mcs_buf->pitch = image->aux_pitch;<br>
> + mt->mcs_buf->qpitch = 0;<br>
> +<br>
> + intel_miptree_init_mcs(brw, mt, 0);<br>
<br>
</span>Ack! This may be an imported image, and therefore the aux surface content is<br>
significant. But this patch clobbers it! What am I misunderstanding?<span class=""><br></span></blockquote><div><br></div><div>Nothing. You're 100% correct. I believe the correct answer is that the original creator of an image with aux needs to do the init. I'll get that fixed.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> + mt->msaa_layout = INTEL_MSAA_LAYOUT_CMS;<br>
<br>
</span>I flinched when I saw mt->msaa_layout = INTEL_MSAA_LAYOUT_CMS. And the comments<br>
below make me sad. Anyway, fixing that is a task for another day.<br></blockquote><div><br></div><div>It makes me very sad too. I've been trying, as I refactor, to move us away from that but it hasn't quite died yet.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
bool<br>
intel_miptree_is_lossless_<wbr>compressed(const struct brw_context *brw,<br>
const struct intel_mipmap_tree *mt)<br>
{<br>
...<br>
<br>
/* Single sample compression is represented re-using msaa compression<br>
* layout type: "Compressed Multisampled Surfaces".<br>
*/<br>
if (mt->msaa_layout != INTEL_MSAA_LAYOUT_CMS)<br>
return false;<br>
<br>
/* And finally distinguish between msaa and single sample case. */<br>
return mt->num_samples <= 1;<br>
<div><div class="h5"> }<br>
<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> struct intel_mipmap_tree *<br>
> intel_miptree_create_for_dri_<wbr>image(struct brw_context *brw,<br>
> __DRIimage *image, GLenum target,<br>
> @@ -1072,15 +1138,26 @@ intel_miptree_create_for_dri_<wbr>image(struct brw_context *brw,<br>
> if (!brw->ctx.<wbr>TextureFormatSupported[format]<wbr>)<br>
> return NULL;<br>
><br>
> + const struct isl_drm_modifier_info *mod_info =<br>
> + isl_drm_modifier_get_info(<wbr>image->modifier);<br>
> +<br>
> + uint32_t mt_layout_flags = 0;<br>
> +<br>
> + /* If this image comes in from a window system, then it may get promoted to<br>
> + * scanout at any time so we need to set the flag accordingly.<br>
> + */<br>
> + if (is_winsys_image)<br>
> + mt_layout_flags |= MIPTREE_LAYOUT_FOR_SCANOUT;<br>
> +<br>
> /* If this image comes in from a window system, we have different<br>
> * requirements than if it comes in via an EGL import operation. Window<br>
> * system images can use any form of auxiliary compression we wish because<br>
> * they get "flushed" before being handed off to the window system and we<br>
> - * have the opportunity to do resolves. Window system buffers also may be<br>
> - * used for scanout so we need to flag that appropriately.<br>
> + * have the opportunity to do resolves.<br>
> */<br>
> - const uint32_t mt_layout_flags =<br>
> - is_winsys_image ? MIPTREE_LAYOUT_FOR_SCANOUT : MIPTREE_LAYOUT_DISABLE_AUX;<br>
> + if (!is_winsys_image &&<br>
> + (!mod_info || mod_info->aux_usage == ISL_AUX_USAGE_NONE))<br>
> + mt_layout_flags |= MIPTREE_LAYOUT_DISABLE_AUX;<br>
><br>
> /* Disable creation of the texture's aux buffers because the driver exposes<br>
> * no EGL API to manage them. That is, there is no API for resolving the aux<br>
> @@ -1117,15 +1194,41 @@ intel_miptree_create_for_dri_<wbr>image(struct brw_context *brw,<br>
> }<br>
> }<br>
><br>
> - /* Since CCS_E can compress more than just clear color, we create the CCS<br>
> - * for it up-front. For CCS_D which only compresses clears, we create the<br>
> - * CCS on-demand when a clear occurs that wants one.<br>
> - */<br>
> - if (mt->aux_usage == ISL_AUX_USAGE_CCS_E) {<br>
> - if (!intel_miptree_alloc_ccs(brw, mt)) {<br>
> + if (mod_info && mod_info->aux_usage != ISL_AUX_USAGE_NONE) {<br>
> + assert(mod_info->aux_usage == ISL_AUX_USAGE_CCS_E);<br>
> +<br>
> + mt->aux_usage = mod_info->aux_usage;<br>
<br>
</div></div>Convince me that it's safe to clobber mt->aux_usage here. I will be easily convinced.<br><div><div class="h5"></div></div></blockquote><div><br></div><div>No aux buffer has been created yet. I'm happy to assert that.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> + /* If we are a window system buffer, then we can support fast-clears<br>
> + * even if the modifier doesn't support them by doing a partial resolve<br>
> + * as part of the flush operation.<br>
> + */<br>
> + mt->supports_fast_clear =<br>
> + is_winsys_image || mod_info->supports_clear_<wbr>color;<br>
> +<br>
> + /* We don't know the actual state of the surface when we get it but we<br>
> + * can make a pretty good guess based on the modifier. What we do know<br>
> + * for sure is that it isn't in the AUX_INVALID state, so we just assume<br>
> + * a worst case of compression.<br>
> + */<br>
> + enum isl_aux_state initial_state =<br>
> + mod_info->supports_clear_color ? ISL_AUX_STATE_COMPRESSED_CLEAR :<br>
> + ISL_AUX_STATE_COMPRESSED_NO_<wbr>CLEAR;<br>
> +<br>
> + if (!create_ccs_buf_for_image(<wbr>brw, image, mt, initial_state)) {<br>
> intel_miptree_release(&mt);<br>
> return NULL;<br>
> }<br>
> + } else if (mt->aux_usage != ISL_AUX_USAGE_NONE) {<br>
> + /* Since CCS_E can compress more than just clear color, we create the<br>
> + * CCS for it up-front. For CCS_D which only compresses clears, we<br>
> + * create the CCS on-demand when a clear occurs that wants one.<br>
> + */<br>
> + if (mt->aux_usage == ISL_AUX_USAGE_CCS_E) {<br>
<br>
</div></div>The else-if-if chain here is ugly.</blockquote><div><br></div><div>Yes it is. But I did it because it made more logical sense to me. "if we have a modifier, follow it for aux else follow aux_usage." However, the only aux_usage that will ever show up here is CCS_E.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The inner if's condition should replace the<br>
outer else-if's condition.<br></blockquote><div><br></div><div>Here's a better idea. How about I write an intel_miptree_alloc_aux helper function and call that for aux_usage != NONE? It can handle CCS, MCS, and HiZ so that's all together in one place.<br></div></div><br></div><div class="gmail_extra">--Jason<br></div></div>