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.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?
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?
>
> 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
