[Mesa-dev] MapTextureImage patch series for review

[Brian Paul]

On 08/17/2011 04:51 PM, Eric Anholt wrote:
> On Tue, 16 Aug 2011 18:53:00 -0600, Brian Paul<brianp at vmware.com>  wrote:
>> On 08/15/2011 12:53 PM, Eric Anholt wrote:
>>> Here's my "mti-tested" branch of the map-texture-image-v4 work.  It's
>>> not regressing on intel or softpipe, and in fact fixes a couple of
>>> tests now on those.  It is not as complete a rework as
>>> map-texture-image-v4, but it's quite a bit of the change and it it
>>> should be bisectable (I've had to several times so far to work out the
>>> regressions in map-texture-image-v4).
>>
>> Hmmm, yeah, a lot of the core/swrast refactoring I did isn't there.  I
>> guess I can redo that on top of your patch series, but it'll take some
>> time (and I've had virtually no spare time lately).
>
> Note that for a bunch of the driver typing for the swrast base-classing,
> coccinelle was *very* useful for producing clean changes:

I've never heard of coccinelle so I'll have to read up on that first.


>
> @@
> @@
> struct intel_texture_image {
> - struct gl_texture_image base;
> + struct swrast_texture_image base;
>    ...
> };
>
> @@
> struct intel_texture_image *I;
> @@
> - I->base
> + I->base.Base
>
> (though I think something that noticed that there was a gl_texture_image
> that the intel_texture_image came from and dereffing that instead would
> be a lot nicer).
>
> With that, I found the rebasing of the rest of your branch on top a lot
> less painful when I was doing it.  The issue I saw was how swrast's
> private fields would get updated when a teximage is initialized, but
> that should be a matter of a small init function in swrast.
>
>>> However, despite the two driver issues, I'd still like to push the
>>> code.  I think it makes maintenance of classic drivers much easier.
>>> Before doing so, I'd like to get at last an ack from Brian and the
>>> other driver maintainers for the changes to their stuff.  If you don't
>>> want to review/ack the whole series, just mention it for the driver
>>> patch in question.
>>
>> I'd like to see this merged/pushed soon too.  However, we've got a
>> problem with the gallium drivers.  The non-scons build complains that
>> _mesa_meta_init() isn't defined because meta.c isn't included in the
>> libmesagallium.a library.  I had a patch for this, but it was pretty
>> ugly (and I think I lost it).
>>
>> The problem with meta.c is it uses swrast code (which we don't want in
>> gallium builds - though, that's what the SCons build currently does).
>
> Ouch.  Good point.  What I'm confused by is how I'm sure I managed to
> build and test softpipe despite this.
>
> I found that the gallium decompression produced regressions when it
> started getting used for mipmap generation, which was why I started
> using the meta there.

I need to do some testing of that yet, but I have a patch that I'll
post very soon that refactors things a bit and fixes the
gallium/meta/swrast dependency mess.

Which piglit tests were showing regressions?


>> But the only thing in meta.c that gallium uses (indirectly) is the new
>> texture decompression code.
>
> I've been thinking about trying to pull the glGetTexImage meta
> decompression out and get the current set of changes in minus that.  I
> don't think it's quite baked yet -- it still has issues with snorm
> compressed textures, and I fear it doesn't support all the format/type
> combos that would be valid for glGetTexImage. I really want the
> generatemipmaps cleanup, though, so I'm not sure how to handle getting
> the fetch functions what they need if we were to go that way for now.
>
> However, if we can confirm that the format/types stuff for glGetTexImage
> of compressed is good (we don't have tests for it), I'd be up for making
> snorm decompression rely on snorm RTT and go ahead with the metaops.  We
> don't support that snorm RTT in i965 right now because of span functions
> despair.  I'd like to see a renderbuffer mapping hook like this and
> avoid spans functions before flailing around more with that.  If
> everything works out, we're hoping to sort that out before the next
> release.

I was planning on tackling the renderbuffer/span mess next.


>> One fix would be to refactor meta.c into several files to separate out
>> the meta functions that use swrast.
>
> I typed out basically that sentence before coming across this
> paragraph.  I agree!
>
>> Another approach is to add more functions like
>> _mesa_meta_check_generate_mipmap_fallback() to query if the meta
>> function can do its job, then remove all the swrast calls and leave it
>> up to the caller to call the meta or swrast functions.
>
> I've been wanting less of that.  At least for mipmaps generation, the
> driver hooks all ended up being
>
> if (_mesa_meta_check_generate_mipmap_fallback())
>     _mesa_generate_mipmaps();
> else
>     _mesa_meta_generate_mipmaps().

Take a look at the patch coming next...

-Brian