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