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