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

[Ian Romanick]

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>> 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).  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 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. :)

>     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?