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

[Pohjolainen, Topi]

On Tue, Apr 28, 2015 at 10:28:17AM +0300, Rogovin, Kevin wrote:
> 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. 
> 

I read the patch again and I'm still in the opinion that the changes
to the pure pre-gen7 logic (i.e., logic that is not re-used for later gens)
are not needed.

The shared logic between pre-gen7 and later, namely setup for renderbuffers,
drawing rectangle and fragment shader compilation key are safe to do as
they only introduce new logic that is conditional to no-attachments being
used.
Your concern about the readers getting confused could be also addressed
with assert(brw->gen >= 7) and a comment saying that the no-attachment
specific path is not applicable for older gens.

And when it comes to the pure pre-gen7 logic, I, in fact, have just the
opposite opinion on making it to go through the no-attachment-aware path.
As the extension is not possible for older gens, I find it clearer that logic
explicitly by-passes such paths that even consider it.