On Mon, Jan 02, 2017 at 06:37:10PM -0800, Ben Widawsky wrote:
> 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.
>
> assert(mt->offset == 0) in ccs creation (as requested by Topi)
>
> Cc: Topi Pohjolainen <topi.pohjolai...@intel.com>
> Signed-off-by: Ben Widawsky <b...@bwidawsk.net>
> Acked-by: Daniel Stone <dani...@collabora.com>
> ---
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 97
> +++++++++++++++++++++++----
> 1 file changed, 85 insertions(+), 12 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index d79cc61ac4..dce8ce3350 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -58,6 +58,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.
> @@ -152,6 +157,9 @@ intel_miptree_supports_non_msrt_fast_clear(struct
> brw_context *brw,
> if (mt->aux_disable & INTEL_AUX_DISABLE_MCS)
> return false;
>
> + if (mt->is_scanout)
> + return false;
> +
> /* This function applies only to non-multisampled render targets. */
> if (mt->num_samples > 1)
> return false;
> @@ -744,6 +752,7 @@ intel_miptree_create(struct brw_context *brw,
> * resolves.
> */
> const bool lossless_compression_disabled = INTEL_DEBUG & DEBUG_NO_RBC;
> + assert(!mt->is_scanout);
> const bool is_lossless_compressed =
> unlikely(!lossless_compression_disabled) &&
> brw->gen >= 9 && !mt->is_scanout &&
> @@ -810,6 +819,45 @@ intel_miptree_create_for_bo(struct brw_context *brw,
> return mt;
> }
>
> +static bool
> +create_ccs_buf_for_image(struct brw_context *intel,
> + __DRIimage *image,
> + struct intel_mipmap_tree *mt)
> +{
> +
> + struct isl_surf temp_main_surf;
> + struct isl_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);
Thanks a lot for adding this, especially the comment!
> +
> + intel_miptree_get_isl_surf(intel, mt, &temp_main_surf);
> + if (!isl_surf_get_ccs_surf(&intel->isl_dev, &temp_main_surf,
> &temp_ccs_surf))
> + return false;
> +
> + mt->mcs_buf = calloc(1, sizeof(*mt->mcs_buf));
> + mt->mcs_buf->bo = image->bo;
> + drm_intel_bo_reference(image->bo);
> +
> + mt->mcs_buf->offset = image->aux_offset;
> + mt->mcs_buf->size = temp_ccs_surf.size;
> + mt->mcs_buf->pitch = temp_ccs_surf.row_pitch;
> + mt->mcs_buf->qpitch = isl_surf_get_array_pitch_sa_rows(&temp_ccs_surf);
> +
> + intel_miptree_init_mcs(intel, mt, 0);
> + mt->aux_disable &= ~INTEL_AUX_DISABLE_CCS;
> + mt->msaa_layout = INTEL_MSAA_LAYOUT_CMS;
> +
> + return true;
> +}
> +
> struct intel_mipmap_tree *
> intel_miptree_create_for_image(struct brw_context *intel,
> __DRIimage *image,
> @@ -820,17 +868,42 @@ intel_miptree_create_for_image(struct brw_context
> *intel,
> uint32_t pitch,
> uint32_t layout_flags)
> {
> - layout_flags = (layout_flags & MIPTREE_LAYOUT_FOR_SCANOUT) ?
> - MIPTREE_LAYOUT_FOR_SCANOUT : MIPTREE_LAYOUT_DISABLE_AUX;
> - return intel_miptree_create_for_bo(intel,
> - image->bo,
> - format,
> - offset,
> - width,
> - height,
> - 1,
> - pitch,
> - layout_flags);
> + struct intel_mipmap_tree *mt;
> +
> + /* Other flags will be ignored, so make sure the caller didn't pass any.
> */
> + assert((layout_flags & ~MIPTREE_LAYOUT_FOR_SCANOUT) == 0);
> +
> + if (!image->aux_offset)
> + layout_flags |= MIPTREE_LAYOUT_DISABLE_AUX;
> + else
> + layout_flags |= MIPTREE_LAYOUT_FORCE_HALIGN16;
> +
> + mt = intel_miptree_create_for_bo(intel,
> + image->bo,
> + format,
> + offset,
> + width,
> + height,
> + 1,
> + pitch,
> + layout_flags);
> +
> + if (!intel_tiling_supports_non_msrt_mcs(intel, mt->tiling)) {
> + assert(image->aux_offset == 0);
> + return mt;
> + }
> +
> + if (layout_flags & MIPTREE_LAYOUT_DISABLE_AUX)
> + return mt;
> +
> + assert(image->aux_offset);
> + assert(mt->num_samples <= 1);
> + assert(mt->last_level < 2);
> + assert(mt->logical_depth0 == 1);
> +
> + create_ccs_buf_for_image(intel, image, mt);
> +
> + return mt;
> }
>
> /**
> @@ -998,7 +1071,7 @@ intel_miptree_release(struct intel_mipmap_tree **mt)
> intel_miptree_release(&(*mt)->stencil_mt);
> intel_miptree_release(&(*mt)->r8stencil_mt);
> intel_miptree_hiz_buffer_free((*mt)->hiz_buf);
> - if ((*mt)->mcs_buf) {
> + if ((*mt)->mcs_buf && !(*mt)->is_scanout) {
Reference count gets incremented in create_ccs_buf_for_image(), so shouldn't
we decrease it here? Original image should hold reference as long as it needs
regardless the logic here. Or am I missing something?
Otherwise the patch looks good:
Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
> drm_intel_bo_unreference((*mt)->mcs_buf->bo);
> free((*mt)->mcs_buf);
> }
> --
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
