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

[Ben Widawsky]

On 17-01-02 10:25:49, Topi Pohjolainen Topi Pohjolainen wrote:
>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.

Yeah, I think this isn't something we can actually hit today because the scanout
buffer is allocated separately and won't be part of another miptree (someone
please correct me if I am wrong). Now, the code was mostly trying to be
"correct" in that, if there is an offset, we should obey it. I suppose instead I
can go with your assertion that offset == 0. It doesn't matter much to me as
long as it passes the tests.

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

Yes. It was the latter case. I don't think we do it, but I wanted the code to
handle it if we decide to. As stated above, I'm fine guarding against the case.

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

Roger :-)