[Mesa-dev] [PATCH 19/32] i965/miptree: Allocate mcs_buf for an image's CCS_E
Ben Widawsky
ben at bwidawsk.net
Thu Jan 5 02:12:57 UTC 2017
On 17-01-04 10:34:17, Topi Pohjolainen Topi Pohjolainen wrote:
>On Wed, Jan 04, 2017 at 09:40:51AM +0200, Pohjolainen, Topi wrote:
>> 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.pohjolainen at intel.com>
>> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> > Acked-by: Daniel Stone <daniels at 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?
>
>You drop this change again in patch 31. Between here and patch 31,
>(*mt)->mcs_buf would always be NULL if (*mt)->is_scanout is true. Is this a
>leftover from some early work?
>
Yeah, I think this was a bug with my original work which got pulled in by
accident on rebase. This was like an interim fix. Originally I didn't hold two
references (the ccs data in the "mcs_buf" and the image's pixel's data are both
in the same BO), and so the objective of this hunk was to only release the
reference once - which seems broken anyway.
Anyway, like you noticed, this hunk is bogus and gone later in the series. I'll
get it out of this patch.
>>
>> Otherwise the patch looks good:
>>
>> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
>>
Thanks.
>> > drm_intel_bo_unreference((*mt)->mcs_buf->bo);
>> > free((*mt)->mcs_buf);
>> > }
>> > --
>> > 2.11.0
>> >
>> > _______________________________________________
>> > 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