[Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

[Rogovin, Kevin]

Hi, 

>A couple of us chatted about this, and we all agreed that it's probably not useful to 
>enable ARB_framebuffer_no_attachments on pre-Gen7.  We don't expose atomics, SSBOs, 
> or image load/store on those platforms (and probably won't), so the 
> only way fragment shaders can output any data is via framebuffer writes.

> So I'd be inclined to just not touch the pre-Gen7 code at all.

There are a number of issues lurking. 

Firstly, there are atoms from Gen4 (for example brw_drawing_rect) 
which are used all the way to Gen8.  Therefore, Gen4 code must be 
changed. At this point then one can advocate to only change those 
atoms that are active in Gen7 and Gen8. If that is done, then there 
is an style inconsistency where some atoms , for Gen4,5 and 6 use 
the new functions and some do not. That is just icky in my opinion,
as it  leads to the awkward question "why? is there something 
delicate here?" Compounding the style issue is that the code
should -convey- what it being done to the reader what is going on.
Using the functions makes the convey more trivial for the reader
to know.

Secondly, not using those functions for Gen4,5  and 6 leaves the code 
with a trap for  later. Namely that trap if someone says "you know 
although the extension is silly for Gens 4,5 and 6, it still is trivial to 
enable". I hate leaving traps behind for others.

Thirdly, there is the real meat of reviewing patch 6: a good review 
of that patch will make sure that any remaining references to 
gl_framebuffer::Width, Height, and so on are correct (i.e. the code 
requires the dimension of the backing store and not the geometry).
That checking is made easier, more mechanical if -all- of i965 is made 
consistent in this fashion, for otherwise the checker has more to check 
(i.e. instead of is this for setting rasterizer it is setting rasterizer and 
active for Gen7 and 8). 

When I was making these changes to i965 I was also tempted
to add a functions of the sort "_mesa_buffer_width", that
returned gl_fraembufffer::Width regardless of the value
of gl_framebuffer::_HasAttachments. The reason why I was
tempted was to help (via lovely grep) to make patch 6 and 
the reviewing of it more mechanical and easier. but I chose 
not to since the meaning of the fields from gl_framebuffer
became the exact meaning of those fields.  That third 
reason is the one reason that patch 6 should make people
itch (in my opinion): "where all references hit?" Checking
that require more than just checking the diff, that requires
knowing all the references to gl_framebuffer::Width and
friends, knowing the use there and then checking if the
patch does or DOES NOT change that code block 
appropriately. 

-Kevin