[Mesa-dev] [PATCH 16/27] i965/miptree: Allocate mcs_buf for an image's CCS_E

[Pohjolainen, Topi]

On Thu, Dec 29, 2016 at 05:41:49PM -0800, Ben Widawsky wrote:
> On 16-12-10 15:26:02, Pohjolainen, Topi wrote:
> > On Thu, Dec 01, 2016 at 02:09:57PM -0800, Ben Widawsky 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.
> > > 
> > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > > ---
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 89 +++++++++++++++++++++++----
> > >  1 file changed, 77 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 cfa2dc0..d002546 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->disable_aux_buffers)
> > >        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,36 @@ 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;
> > > +   uint32_t offset = mt->offset + image->aux_offset;
> > 
> > I need to ask about this as I'm not sure myself. We usually use offset to
> > move to a slice other than the first. Here the offset to the start of the
> > auxiliary is taken as relative to that. This seems awkward to me. Moreover,
> > if we move to another slice, don't we need to move equally within the auxiliary
> > also.
> > 
> 
> There are restrictions in the hardware that the x,y offset of the main buffer
> need to match that of the aux buffer - that is what this is trying to achieve.
> The scanout shouldn't ever have more than 1 slice (AFAIK), and even if it does,
> we'd do the allocations for all slices here. What do you mean by, "if we move to
> another slice"?

My understanding is that intel_mipmap_tree::offset differs from zero if and
only if the miptree represents an individual image of another miptree (which
is mipmapped or arrayed). So by "if we move to another slice" I mean that the
miptree points to inside a buffer object (that contains multiple slices). Here
you try to offset the auxiliary region as well which is something we haven't
ever done before.
This is something we discussed with Jason and Ken some time ago in different
context, and I recall us agreeing that we really don't want to to try to do
that.
So I guess my question becomes more generic. What use case brings us now in
situation where we need to start pointing to individual slices of compressed
miptrees?
Or is this for a use case where we point to a sub-region of single image? Even
in that case offsetting the auxiliary buffer sounds a little scary to me.


I'm working on i965 ISL at the moment, and one thing I did as preparation step
was to make the "miptree pointing to an individual slice of another miptree"
more explicit. This is part of the reason why I'm asking all these questions.