[Mesa-dev] [PATCH v2 22/27] i965/miptree: Allocate mcs_buf for an image's CCS
Chad Versace
chadversary at chromium.org
Wed Jul 12 18:05:27 UTC 2017
On Thu 29 Jun 2017, Jason Ekstrand wrote:
> From: Ben Widawsky <ben at bwidawsk.net>
>
> This code will disable actually creating these buffers for the scanout,
> but it puts the allocation in place.
>
> Primarily this patch is split out for review, it can be squashed in
> later if preferred.
>
> v2:
> assert(mt->offset == 0) in ccs creation (as requested by Topi)
> Remove bogus is_scanout check in miptree_release
>
> v3:
> Remove is_scanout assert in intel_miptree_create. It doesn't work with
> latest codebase - not sure it ever should have worked.
>
> v4:
> assert(mt->last_level == 0) and assert(mt->first_level == 0) in ccs setup
> (Topi)
>
> v5 (Jason Ekstrand):
> - Base the decision to allocate a CCS on the image modifier
>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> Acked-by: Daniel Stone <daniels at collabora.com>
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 123 +++++++++++++++++++++++---
> 1 file changed, 113 insertions(+), 10 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 575f04f..7a22cbf 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -59,6 +59,11 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
> struct intel_mipmap_tree *mt,
> GLuint num_samples);
>
> +static void
> +intel_miptree_init_mcs(struct brw_context *brw,
> + struct intel_mipmap_tree *mt,
> + int init_value);
> +
> /**
> * Determine which MSAA layout should be used by the MSAA surface being
> * created, based on the chip generation and the surface type.
> @@ -1026,6 +1031,67 @@ miptree_create_for_planar_image(struct brw_context *brw,
> return planar_mt;
> }
>
> +static bool
> +create_ccs_buf_for_image(struct brw_context *brw,
> + __DRIimage *image,
> + struct intel_mipmap_tree *mt,
> + enum isl_aux_state initial_state)
> +{
> + struct isl_surf temp_main_surf, temp_ccs_surf;
> +
> + /* There isn't anything specifically wrong with there being an offset, in
> + * which case, the CCS miptree's offset should be mt->offset +
> + * image->aux_offset. However, the code today only will have an offset when
> + * this miptree is pointing to a slice from another miptree, and in that case
> + * we'd need to offset within the AUX CCS buffer properly. It's questionable
> + * whether our code handles that case properly, and since it can never happen
> + * for scanout, just use the assertion to prevent it.
> + */
> + assert(mt->offset == 0);
> +
> + /* CCS is only supported for very simple miptrees */
> + assert(image->aux_offset && image->aux_pitch);
Why require that aux_offset > 0? Why reject images where the aux surface
precedes the primary surface? This rejection seems arbitrary. Please
add a little explanatory comment.
Also, please use `!= 0` above, to be consistent with the assertions
above and below this one.
> + assert(image->tile_x == 0 && image->tile_y == 0);
> + assert(mt->num_samples <= 1);
> + assert(mt->first_level == 0);
> + assert(mt->last_level == 0);
> + assert(mt->logical_depth0 == 1);
> +
> + /* We shouldn't already have a CCS */
> + assert(!mt->mcs_buf);
> +
> + intel_miptree_get_isl_surf(brw, mt, &temp_main_surf);
> + if (!isl_surf_get_ccs_surf(&brw->isl_dev, &temp_main_surf, &temp_ccs_surf))
> + return false;
> +
> + assert(temp_ccs_surf.size <= image->bo->size - image->aux_offset);
This assertion appears invalid, vulnerable to underflow when the
incoming aux_offset is wrong and too large. It needs to be preceded
by assert(image->aux_offset < image->bo->size).
> + assert(temp_ccs_surf.row_pitch <= image->aux_pitch);
> +
> + mt->mcs_buf = calloc(sizeof(*mt->mcs_buf), 1);
> + if (mt->mcs_buf == NULL)
> + return false;
> +
> + mt->aux_state = create_aux_state_map(mt, initial_state);
> + if (!mt->aux_state) {
> + free(mt->mcs_buf);
> + mt->mcs_buf = NULL;
> + return false;
> + }
> +
> + mt->mcs_buf->bo = image->bo;
> + brw_bo_reference(image->bo);
> +
> + mt->mcs_buf->offset = image->aux_offset;
> + mt->mcs_buf->size = image->bo->size - image->aux_offset;
> + mt->mcs_buf->pitch = image->aux_pitch;
> + mt->mcs_buf->qpitch = 0;
> +
> + intel_miptree_init_mcs(brw, mt, 0);
Ack! This may be an imported image, and therefore the aux surface content is
significant. But this patch clobbers it! What am I misunderstanding?
> + mt->msaa_layout = INTEL_MSAA_LAYOUT_CMS;
I flinched when I saw mt->msaa_layout = INTEL_MSAA_LAYOUT_CMS. And the comments
below make me sad. Anyway, fixing that is a task for another day.
bool
intel_miptree_is_lossless_compressed(const struct brw_context *brw,
const struct intel_mipmap_tree *mt)
{
...
/* Single sample compression is represented re-using msaa compression
* layout type: "Compressed Multisampled Surfaces".
*/
if (mt->msaa_layout != INTEL_MSAA_LAYOUT_CMS)
return false;
/* And finally distinguish between msaa and single sample case. */
return mt->num_samples <= 1;
}
> +
> + return true;
> +}
> +
> struct intel_mipmap_tree *
> intel_miptree_create_for_dri_image(struct brw_context *brw,
> __DRIimage *image, GLenum target,
> @@ -1072,15 +1138,26 @@ intel_miptree_create_for_dri_image(struct brw_context *brw,
> if (!brw->ctx.TextureFormatSupported[format])
> return NULL;
>
> + const struct isl_drm_modifier_info *mod_info =
> + isl_drm_modifier_get_info(image->modifier);
> +
> + uint32_t mt_layout_flags = 0;
> +
> + /* If this image comes in from a window system, then it may get promoted to
> + * scanout at any time so we need to set the flag accordingly.
> + */
> + if (is_winsys_image)
> + mt_layout_flags |= MIPTREE_LAYOUT_FOR_SCANOUT;
> +
> /* If this image comes in from a window system, we have different
> * requirements than if it comes in via an EGL import operation. Window
> * system images can use any form of auxiliary compression we wish because
> * they get "flushed" before being handed off to the window system and we
> - * have the opportunity to do resolves. Window system buffers also may be
> - * used for scanout so we need to flag that appropriately.
> + * have the opportunity to do resolves.
> */
> - const uint32_t mt_layout_flags =
> - is_winsys_image ? MIPTREE_LAYOUT_FOR_SCANOUT : MIPTREE_LAYOUT_DISABLE_AUX;
> + if (!is_winsys_image &&
> + (!mod_info || mod_info->aux_usage == ISL_AUX_USAGE_NONE))
> + mt_layout_flags |= MIPTREE_LAYOUT_DISABLE_AUX;
>
> /* Disable creation of the texture's aux buffers because the driver exposes
> * no EGL API to manage them. That is, there is no API for resolving the aux
> @@ -1117,15 +1194,41 @@ intel_miptree_create_for_dri_image(struct brw_context *brw,
> }
> }
>
> - /* Since CCS_E can compress more than just clear color, we create the CCS
> - * for it up-front. For CCS_D which only compresses clears, we create the
> - * CCS on-demand when a clear occurs that wants one.
> - */
> - if (mt->aux_usage == ISL_AUX_USAGE_CCS_E) {
> - if (!intel_miptree_alloc_ccs(brw, mt)) {
> + if (mod_info && mod_info->aux_usage != ISL_AUX_USAGE_NONE) {
> + assert(mod_info->aux_usage == ISL_AUX_USAGE_CCS_E);
> +
> + mt->aux_usage = mod_info->aux_usage;
Convince me that it's safe to clobber mt->aux_usage here. I will be easily convinced.
> + /* If we are a window system buffer, then we can support fast-clears
> + * even if the modifier doesn't support them by doing a partial resolve
> + * as part of the flush operation.
> + */
> + mt->supports_fast_clear =
> + is_winsys_image || mod_info->supports_clear_color;
> +
> + /* We don't know the actual state of the surface when we get it but we
> + * can make a pretty good guess based on the modifier. What we do know
> + * for sure is that it isn't in the AUX_INVALID state, so we just assume
> + * a worst case of compression.
> + */
> + enum isl_aux_state initial_state =
> + mod_info->supports_clear_color ? ISL_AUX_STATE_COMPRESSED_CLEAR :
> + ISL_AUX_STATE_COMPRESSED_NO_CLEAR;
> +
> + if (!create_ccs_buf_for_image(brw, image, mt, initial_state)) {
> intel_miptree_release(&mt);
> return NULL;
> }
> + } else if (mt->aux_usage != ISL_AUX_USAGE_NONE) {
> + /* Since CCS_E can compress more than just clear color, we create the
> + * CCS for it up-front. For CCS_D which only compresses clears, we
> + * create the CCS on-demand when a clear occurs that wants one.
> + */
> + if (mt->aux_usage == ISL_AUX_USAGE_CCS_E) {
The else-if-if chain here is ugly. The inner if's condition should replace the
outer else-if's condition.
> + if (!intel_miptree_alloc_ccs(brw, mt)) {
> + intel_miptree_release(&mt);
> + return NULL;
> + }
> + }
> }
>
> return mt;
> --
> 2.5.0.400.gff86faf
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list