[Mesa-dev] [PATCH v2 5/5] i965/gen6: Initial implementation of MSAA.

[Ian Romanick]

On 05/17/2012 03:43 PM, Paul Berry wrote:
> On 17 May 2012 11:21, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
>     On 05/14/2012 06:24 PM, Paul Berry wrote:
>
>         On 14 May 2012 16:50, Ian Romanick <idr at freedesktop.org
>         <mailto:idr at freedesktop.org>
>         <mailto:idr at freedesktop.org <mailto:idr at freedesktop.org>>> wrote:
>
>             On 05/14/2012 02:13 PM, Paul Berry wrote:
>
>                 Just to update the list: I had an in-person meeting with
>         Eric,
>                 Ian, and
>                 Ken today to discuss the design of the patch series and
>         decide
>                 what to
>                 do about it.  We agreed to go ahead and push the patches to
>                 master as
>                 is, and to make further improvements as time goes on.
>
>                 Specific improvements that were discussed:
>
>                 - I seem to be the only person who thinks it makes sense
>         for the
>                 brw_blorp_params class to contain an exec() function.
>           In the
>                 interest
>                 of avoiding a rebasing nightmare, I will go ahead with the
>                 patches as
>                 they are, and submitt a follow up patch which replaces
>                 brw_blorp_params::exec() with a global function
>         brw_blorp_exec(const
>                 brw_blorp_params *).
>
>
>             Part of the issue was that "exec" isn't a terrific name, and
>             exec'ing a set of parameters doesn't have implicit meaning.  My
>             preference was for a function, similar to what you mention,
>         called
>             brw_blorp_do_blit or similar.
>
>
>         I'm not going to try to change your mind, but in the spirit of
>         clarifying the method that was behind my madness:
>
>         I did not have the clarity of mind during our discussion today
>         to point
>         out that in creating the brw_blorp_params class, I was in fact
>         trying to
>         follow a standard design pattern, the Gang of Four "command" pattern
>         (http://en.wikipedia.org/wiki/__Command_pattern
>         <http://en.wikipedia.org/wiki/Command_pattern>).  The idea of that
>
>
>     There are some other places, mostly in the compiler, where various
>     patterns are employed.  In those cases, I've explicitly put in a
>     comment like, "This is pattern XYZ.  Refer to http://foo/ for the
>     back story." The comment block in
>     src/glsl/ir_hierarchical___visitor.h is an example.
>
>
>         pattern is to encapsulate all of the information necessary to
>         perform an
>         action into a class; this allows the action to be treated as a first
>         class object.  The usual reasons why it might be useful for the
>         action
>         to be a first class object are so that it can be stored in an undo
>         history, so that its execution can be deferred, restarted, or
>         repeated,
>         or so that some other part of the program can take the
>         opportunity to
>         tweak the parameters before the action is performed.  In this
>         particular
>         case the benefit is that the various phases of the action can be
>         performed by different functions without an explosion of parameter
>         passing, and so that actions that share some, but not all, of their
>         implementation (HiZ ops and blits, in this case) can be
>         implemented by
>         using a base class with virtual functions.  It's also possible
>         that in
>         the future, the blorp engine might be able to record the last
>         command it
>         executed, so that it can compare that to the command it's being
>         asked to
>         execute and avoid some redundant pipeline setup.  In the command
>         pattern, it is conventional to make the function that performs the
>         action a member of the command class and to call it "exec" or
>         "execute".  So from my point of view, this function name
>         actually *does*
>         have an implicit meaning.  I'm aware, of course, that most of my
>         fellow
>         Mesa developers don't share my respect for Gang of Four-style
>         design :)
>
>         (Note that in a strict implementation of the "command" pattern
>         there are
>         other auxiliary classes involved, but I thought that would be
>         way too
>         heavyweight for this purpose).
>
>
>     I believe that it is also typical in the command pattern for the
>     classes to have "command" in the name instead of "parameters."  At
>     least the examples in http://en.wikipedia.org/wiki/__Command_pattern
>     <http://en.wikipedia.org/wiki/Command_pattern> and my "Patterns in
>     Java" book suggest this.
>
>     For me, the advantage of patterns is rarely technical.  For me, the
>     advantage of patterns is in communication with other humans.  If a
>     piece of code says, "I'm an implementation of the factory pattern"
>     people will have a set of expectations about what's coming.  In some
>     sense they'll be familiar with the code before they even see it.
>
>     There was, by necessity, a lot of moving parts in MSAA patch series.
>     That combined with a slight deviation from the pattern (the
>     previously mentioned parameters vs. command naming) caused me to not
>     see the pattern.  For me, that defeated the improved communication
>     of pattern use.
>
>     Communication isn't just with other humans.  Perhaps if the class
>     had been called brw_blorp_blit_command you would have remembered
>     that you were using the command pattern while we were having the
>     review meeting. :)
>
>
> Yes, you're right.  That would have helped.
>
>
>
>             If I see foo->exec() or exec(foo) in a pipe of code, I have
>         to go
>             look at the declaration of foo to know what's going on.  If
>         I see
>             do_some_action(foo) it's a bit more obvious.
>
>
>         In this particular case (and in most uses of the command
>         pattern, IMHO),
>         the declaration is right next to the exec() call, so this isn't a
>         problem.  E.g.:
>
>             /* Do the blit */
>             brw_blorp_blit_params params(brw_context(ctx), src_mt, dst_mt,
>                                          srcX0, srcY0, dstX0, dstY0,
>         dstX1, dstY1,
>                                          mirror_x, mirror_y);
>             params.exec(intel);
>
>         I agree that it looks kind of hokey, though.  Other alternatives I
>         considered included:
>
>         - Make intel_context a class, and make executing a blorp action
>         a method
>         on that class, so that last line would have been something like
>         "intel->do_blorp_op(params);".  Incidentally, I think we would
>         get a lot
>         of benefits out of making brw_context, intel_context, and
>         gl_context a
>         class hierarchy; for example, macros like OUT_BATCH() would become
>         ordinary member functions, and we wouldn't have to do anywhere
>         near as
>         many explicit conversions between the three context types.
>           Also, the
>         virtual function tables that we currently code up explicitly
>         would be
>         automatically created by the compiler, so we would have much
>         less risk
>         of problems like forgetting to dispatch through the virtual function
>         table, forgetting to initialize a function pointer properly, or
>         initializing a function pointer at the wrong time.  Of course, this
>         would have been *way* too big a change to try to slip into this
>         patch
>         series.  By orders of magnitude.
>
>         - Make a base class to represent the Blorp engine, with a
>         derived class
>         for Gen6 and a derived class for Gen7.  The appropriate object
>         would be
>         created at context creation time.  Then, instead of calling
>         "params.exec(intel);", you would do "brw->blorp->exec(params);".  I
>         largely dismissed this option because I had already done so much
>         refactoring of the HiZ code that it seemed like it was time to
>         settle
>         down and actually implement some new functionality.  But I would
>         still
>         be open to it if people like the idea.
>
>
>     Wouldn't a more usual way to handle this kind of partitioning be
>     with an abstract factory?
>
> Can you elaborate?  IIRC, an abstract factory is for when component A
> needs to do object creation, but component B knows how to do object
> creation, so you create an abstract factory class with a pure virtual
> "create" method.  Component B instantiates a concrete factory and hands
> it off to component A, and then component A can use the factory to
> create the objects without needing to know details about how they are
> created.  It's not obvious to me how to map this pattern onto the
> situation we have with brw_blorp_params.

I was thinking you'd have an blorp_factory class with pure virtual 
create_blit_command and create_hiz_resolve_command methods.  The 
gen6_blorp_factory and gen7_blorp_factory classes would derive from 
blorp_factory.  The intelCreateContext would instantiate either a 
gen6_blorp_factory or a gen7_blorp_factory at context creation.  The 
rest of the architecture would remain pretty much the way you have it now.