[Mesa-dev] About merging pipe-video to master

[Christian König]

Am Dienstag, den 12.07.2011, 15:44 +0200 schrieb Henri Verbeet:
> 2011/7/12 Christian König <deathsimple at vodafone.de>:
> > it works with my available hardware (no piglit regressions). The changes
> > to the winsys code is about making a bo optional, even when the reg
> > informations says it isn't. This is useful for registers where only a
> > subset of the bits needs to be informed about a bo relocation, and seems
> > to work pretty fine as long as you don't touch those bits.
> >
> Well, ok, but I'd expect to find that information in the commit log.
> As it is, 68cc6bc5d8b6986acc7f5780d705f4ae9be2a446 removes
> REG_FLAG_NEED_BO, then e602ecf9ef2f66289bcb159fdbdce2c76e3c07c1 adds
> it back without much of an explanation. Also, what subset is that?
> After this patch both places that touch the register pass NULL for the
> bo.
Sounds like you're right. Adding the flag again was followed by a
discussion with Alex on the mailing list
(http://lists.freedesktop.org/archives/mesa-dev/2011-April/007069.html),
and I should have documented that a bit more. Also the second place
shouldn't pass NULL for the bo, that is indeed a bug.

> > +		// TODO get BLEND_CLAMP state from rasterizer state
> Is this comment still accurate?
Yes it is, the very first generation of R600 chipsets need to know if
blend clamping is enabled, to enable an additionally optimisation for
the color export (EXPORT_NORM).

The problem is that I'm unsure how to get that state from the rasterizer
structure into r600_cb, reprogramming color_info in r600_draw_vbo just
like Vadim Girlin did for his patches, seems to be a bit to much
overhead to me.

> > +				color_info, ~S_0280A0_BLEND_CLAMP(1), NULL);
> Did you mean to write "~C_0280A0_BLEND_CLAMP" there?
Yes, it should write every bit except BLEND_CLAMP here. 

> > On top of that it implements "clamp_fragment_color" also for the blender
> > state, this is necessary because the blender will otherwise clamp the
> > colour to [0,1] for unsigned and [-1,1] for signed buffers.
> >
> > This is another piece needed to get arb_color_buffer_float working
> > correctly (without the need to recompile the shaders each time).
> >
> You should probably remove the existing code that does that in
> r600_shader_from_tgsi() then, at least for r600. 
It's a bit more complicated than this, BLEND_CLAMP can only be used for
normalized buffers, but not float buffers. So we still need the
workaround with the shader recompile for those.

Vadim Girlin patches are indeed quite right for float buffers, the
problem is that disabling fragment color clamping currently doesn't work
correctly under the following conditions (with the code in master
branch):
1) you got a unsigned normalized buffer
2) you use a blender with ADD or SUB operation
3) your fragment shader outputs negative values

It took me a week to figure out what's going wrong here and why the
pipeline doesn't did what I wanted. The downside with my patches is that
it disables the export optimisation on the early R600 generation
chipsets, but my overall feeling is that it's better to render right and
slow instead of fast and wrong.

> Either way, it sounds
> like this is a mostly independent change from the rest of pipe-video
> and should go to r600g through the regular way, probably through the
> mailing list first.
Akk, I will send these as separate patches then.

> > +        switch (res->usage) {
> > +        case PIPE_USAGE_STREAM:
> > +        case PIPE_USAGE_STAGING:
> > +        case PIPE_USAGE_STATIC:
> > +        case PIPE_USAGE_IMMUTABLE:
> > +                return FALSE;
> > +
> > +        default:
> > +                return TRUE;
> > +        }
> At the very least this has whitespace errors. Why do we want this?
> Like the other change, the commit log for this change
> (77217af40d67612d1f1089ca188393d27a8a038f) isn't very descriptive. If
> it wasn't for the commit not being a merge commit, it would even be
> ambiguous if "Merge fix" means merging a fix or fixing a merge.
It was "fixing a merge", as it turned out Mathias Fröhlich and I were
working on the same thing -> placing bo buffers in different domains
depending on the resource usage.

Additional to that, these lines (which were part of my patch, but not of
Mathias code) should ensure that the driver doesn't tries to copy things
with a hardware blit from the staging area to vram.

For PIPE_USAGE_STREAM it doesn't makes sense because the very first
thing done with it is copying the data to the right position inside
another (much larger) buffer anyway.

For PIPE_USAGE_STAGING it doesn't make sense because the resource is
already marked as a STAGING resource anyway.

PIPE_USAGE_STATIC resources shouldn't be accessed by the CPU anyway.

And at least my PIPE_USAGE_IMMUTABLE resources are small and change so
infrequently that setting up a hardware blit takes more time than
copying them with the CPU.

Sorry for the confusion about the history of those patches, but I've
used "git reset --hard" while merge to get ride of my patches (BIG
mistake I know that now), ending up with my original patch inside a
merge commit.

I will send them as separate patches, so we should have a good patch
history in master when this gets merged.

Regards,
Christian.