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

Paul Berry stereotype441 at gmail.com
Mon May 14 18:24:05 PDT 2012


On 14 May 2012 16:50, Ian Romanick <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 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).


>
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120514/bf6f85d5/attachment-0001.html>


More information about the mesa-dev mailing list