[Mesa-dev] [PATCH 3/8] i965: Faking the ETC2 compression on Gen < 8 GPUs using two miptrees.

[Nanley Chery]

On Tue, Jan 22, 2019 at 01:15:25PM +0200, Eleni Maria Stea wrote:
> On 1/22/19 12:46 PM, Eleni Maria Stea wrote:
> >>> +   /**
> >>> +    * \brief Indicates that we fake the ETC2 compression support
> >>> +    *
> >>> +    * GPUs Gen < 8 don't support sampling and rendering of ETC2
> >>> formats so
> >>> +    * we need to fake it. This variable is set to true when we
> >>> fake it.
> >>> +    */
> >>> +   bool needs_fake_etc;
> >>> +  
> >>
> >> Let's make a function to detect needs_fake_etc instead of adding to
> >> the data structure. That'd be easier to follow.
> >>
> >> -Nanley
> > 
> > 
> > Hi Nanley,
> > 
> > I'd like a small clarification here if you don't mind: I wasn't very
> > sure about this last change you suggest.
> > 
> > The reasons I preferred to extend the data structure instead of adding
> > a function were:
> > 
> > 1- that I need to check if we fake ETC in several different places in
> > which I don't always have access to the information that helped me
> > decide if we need to fake the ETC or not, so I found it much easier to
> > keep this information in the miptree that can be accessed from
> > everywhere. (That was the main reason).
> 
> Actually, now I better thought of it, I only need the GPU version and if
> the format is compressed, so I can probably get this information in all
> places but we would still need to make many unnecessary calls...
> Couldn't we avoid them by just checking this once at the beginning?
> 

The performance difference should be negligible if the function is
declared static inline in the intel_mipmap_tree.h header. The compiler
should include the body of function (which should be small) and avoid
the overhead of a function call.

> Thanks again,
> Eleni
> 
> > The other reasons were that:
> > 2- I thought that it would be faster to check the miptree than call a
> > function.
> > 3- I was hoping that from the name of the variable it won't be
> > difficult to follow (but I could rename it to something better if you
> > prefer it).
> > 
> > Could you explain me why you'd like me to replace it? Is there an
> > advantage I hadn't thought of?
> > 

Firstly, it's not information that's generally useful for most
intel_mipmap_tree objects. Having too much of such state makes debugging
and reading the struct definition more difficult.

Secondly, it adds to the amount of state-dependent variables I have to
keep in mind when looking at the code. I have to start asking, when is
needs_fake_etc initialized? Is needs_fake_etc ever modified later? I'm
already familiar with the other variables needs_fake_etc can be computed
by: the gen, the miptree format, and the shadow_mt. I hope that helps.

-Nanley

> > Thank you in advance,
> > Eleni
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>