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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
