On Tue, Apr 28, 2015 at 3:37 PM, Rogovin, Kevin <kevin.rogo...@intel.com> wrote: > >> 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. > > As I have tried and apparently failed to communicate, it is -better- and more > consistent. Need > is a far stronger word. Without a doubt, if the extension is never enabled > for those older > Gens, then it does not matter in terms of produced output. However, I stated > that it leaves > a trap and an inconsistency which I find quite bothering.
You have very clearly communicated that you *think* it's better to change it everywhere. Others have chosen to disagree for a variety of reasons. Defending your choice to the death isn't aiding in the discussion. >> 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. > > And that is exactly for the case for that code that is not shared. Indeed, if > the shared code is safe > for pre-Gen7, then so is the non-shared code. No, because the non-shared code is (by your own admission) untested and/or dead code. Untested code is broken code. I would personally be ok with a lot of the changes that just replace fb->Width with _mesa_geometric_width(fb) since it's effectively just replacing a direct access with a getter. However, almost half of the patch is updating the upload_sf_vp function which is only used for gen <= 5. A comment or assert there would be sufficient rather than reworking it. >> 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. > > There is only one occurrence of "no-attachment specific code paths" in these > i965 patches > and that is associated to scissoring. The rest is existing code is changed > from accessing Width, > Height of gl_framebuffer to "getting" those values from a function. There is > no proper place > to insert an assert(brw->gen >=7 ), since, with the exception of the > scissoring (and it is just > one if block) there is no such "no attachment code path". I had thought the > diffs of the series > made that quite clear. > >> 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. > > Um, I am pretty sure than pre Gen7 hardware can do the extension. The crux is > that the extension > is pointless for such hardware because pre Gen7 hardware does not (AFAIK) > have a feature that > allows for a fragment shader to have a side effect. Even that statement is > not totally true. Indeed, > one can argue performance queries and occlusion queries with > framebuffer_no_attachments > make some form of sense (it would give an application a count of sorts). That's a contingency I think we can ignore for the moment. If someone really wants to do occlusion queries with no framebuffer on ILK, we can add it then. Until that unlikely event happens, let's concentrate on HW that at least exposes atomics. --Jason _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
