[Mesa-dev] [PATCH 05/15] i965: Prepare image validation for isl based miptrees

[Pohjolainen, Topi]

On Thu, Jun 15, 2017 at 09:57:03PM -0700, Jason Ekstrand wrote:
> On Thu, Jun 15, 2017 at 9:49 PM, Pohjolainen, Topi <
> topi.pohjolainen at gmail.com> wrote:
> 
> > On Thu, Jun 15, 2017 at 05:21:42PM -0700, Nanley Chery wrote:
> > > On Tue, Jun 13, 2017 at 05:50:03PM +0300, Topi Pohjolainen wrote:
> > > > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > > ---
> > > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > >
> > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > index 061860cdf6..f44bac988f 100644
> > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > @@ -1087,6 +1087,21 @@ intel_miptree_match_image(struct
> > intel_mipmap_tree *mt,
> > > >     if (mt->target == GL_TEXTURE_CUBE_MAP)
> > > >        depth = 6;
> > > >
> > > > +   if (mt->surf.size > 0) {
> > > > +      if (level >= mt->surf.levels)
> > > > +         return false;
> > > > +
> > > > +      const unsigned level_depth =
> > > > +         mt->surf.dim == ISL_SURF_DIM_3D ?
> > > > +            minify(mt->surf.logical_level0_px.depth, level) :
> > >
> > > We should be looking up the physical depth shouldn't we? I haven't yet
> > > looked into what width or height should be. I'm done for today, so I
> > > will revisit this series tomorrow.
> >
> > Hmm, you are correct when comparing to the existing code - it uses the
> > table
> > which contains physical values. The given depth, however, comes from
> > gl_texture_image::Depth which is core concept and deals with logical
> > values.
> >
> > The confusing bit here is (which I should have written into the commit
> > message) is that number of layers can't be altered, only levels. The
> > existing
> > code checks for depth because it uses that to catch missing levels. The
> > table
> > is initialised to zeroes and finding depth of zero is taken as meaning that
> > the desired level doesn't exist.
> >
> 
> It's also worth pointing out that there's no difference between logical and
> physical with 3D since 3D can't be multisampled.  For 3D, I think logical
> is the only one that makes sense.
> 
> 
> > >
> > > > +            mt->surf.logical_level0_px.array_len;
> > > > +
> > > > +      return width == minify(mt->surf.logical_level0_px.width,
> > level) &&
> > > > +             height == minify(mt->surf.logical_level0_px.height,
> > level) &&
> > > > +             depth == level_depth &&
> >
> > I should probably assert this instead of failing - as I said I don't see
> > how
> > number of layers could change. Ian, Ken, Jason, how do you see this?
> >
> 
> No, you want to return.  The whole point of this function is to determine
> if this gl_texture_image matches the given level of the miptree.  This is
> used, for instence, when someone does glTexImage2D and changes the size of
> one LOD.  In that case, we can't upload to the miptree and we have to split
> that LOD out into it's own.  Later on, assuming the texture is complete, we
> recombine them into a single miptree.  It's aweful, but welcome to OpenGL.

Right, sorry, should have been clearer. I meant:

         /* Only number of levels, width and height can be altered. */
         assert(depth == level_depth);

         return width == minify(mt->surf.logical_level0_px.width, level) &&
                height == minify(mt->surf.logical_level0_px.height, level);

> 
> --Jason
> 
> 
> > > > +             MAX2(image->NumSamples, 1) == mt->surf.samples;
> > > > +   }
> > > > +
> > > >     int level_depth = mt->level[level].depth;
> > > >     if (mt->num_samples > 1) {
> > > >        switch (mt->msaa_layout) {
> > > > --
> > > > 2.11.0
> > > >
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >