[Mesa-dev] [PATCH 1/6] st/mesa: remove a weird msaa hack

[Roland Scheidegger]

Am 10.12.2012 21:22, schrieb Marek Olšák:
> On Mon, Dec 10, 2012 at 5:28 PM, Brian Paul <brianp at vmware.com> wrote:
>> On 12/08/2012 07:40 AM, Marek Olšák wrote:
>>>
>>> It doesn't work and it's not clear how it's supposed to work.
>>> ---
>>>   src/mesa/state_tracker/st_atom_rasterizer.c |    3 +--
>>>   src/mesa/state_tracker/st_context.c         |   17 -----------------
>>>   src/mesa/state_tracker/st_context.h         |    4 ----
>>>   src/mesa/state_tracker/st_manager.c         |    7 +------
>>>   4 files changed, 2 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/src/mesa/state_tracker/st_atom_rasterizer.c
>>> b/src/mesa/state_tracker/st_atom_rasterizer.c
>>> index d9e9d21..f20df9e 100644
>>> --- a/src/mesa/state_tracker/st_atom_rasterizer.c
>>> +++ b/src/mesa/state_tracker/st_atom_rasterizer.c
>>> @@ -230,8 +230,7 @@ static void update_raster_state( struct st_context *st
>>> )
>>>      raster->line_stipple_factor = ctx->Line.StippleFactor - 1;
>>>
>>>      /* _NEW_MULTISAMPLE */
>>> -   if (ctx->Multisample._Enabled || st->force_msaa)
>>> -      raster->multisample = 1;
>>> +   raster->multisample = ctx->Multisample._Enabled;
>>>
>>>      /* _NEW_SCISSOR */
>>>      if (ctx->Scissor.Enabled)
>>> diff --git a/src/mesa/state_tracker/st_context.c
>>> b/src/mesa/state_tracker/st_context.c
>>> index 69bd503..efac9ee 100644
>>> --- a/src/mesa/state_tracker/st_context.c
>>> +++ b/src/mesa/state_tracker/st_context.c
>>> @@ -97,22 +97,6 @@ void st_invalidate_state(struct gl_context * ctx,
>>> GLuint new_state)
>>>      _vbo_InvalidateState(ctx, new_state);
>>>   }
>>>
>>> -
>>> -/**
>>> - * Check for multisample env var override.
>>> - */
>>> -int
>>> -st_get_msaa(void)
>>> -{
>>> -   const char *msaa = _mesa_getenv("__GL_FSAA_MODE");
>>> -   if (msaa)
>>> -      return atoi(msaa);
>>> -   return 0;
>>> -}
>>> -
>>> -
>>> -
>>> -
>>>   static struct st_context *
>>>   st_create_context_priv( struct gl_context *ctx, struct pipe_context
>>> *pipe,
>>>                 const struct st_config_options *options)
>>> @@ -193,7 +177,6 @@ st_create_context_priv( struct gl_context *ctx, struct
>>> pipe_context *pipe,
>>>
>>>      st->pixel_xfer.cache = _mesa_new_program_cache();
>>>
>>> -   st->force_msaa = st_get_msaa();
>>>      st->has_stencil_export =
>>>         screen->get_param(screen, PIPE_CAP_SHADER_STENCIL_EXPORT);
>>>
>>> diff --git a/src/mesa/state_tracker/st_context.h
>>> b/src/mesa/state_tracker/st_context.h
>>> index 2cc5277..8e6f28b 100644
>>> --- a/src/mesa/state_tracker/st_context.h
>>> +++ b/src/mesa/state_tracker/st_context.h
>>> @@ -185,7 +185,6 @@ struct st_context
>>>
>>>      struct cso_context *cso_context;
>>>
>>> -   int force_msaa;
>>>      void *winsys_drawable_handle;
>>>
>>>      /* The number of vertex buffers from the last call of
>>> validate_arrays. */
>>> @@ -265,9 +264,6 @@ st_fb_orientation(const struct gl_framebuffer *fb)
>>>   #define ST_CALLOC_STRUCT(T)   (struct T *) calloc(1, sizeof(struct T))
>>>
>>>
>>> -extern int
>>> -st_get_msaa(void);
>>> -
>>>   extern struct st_context *
>>>   st_create_context(gl_api api, struct pipe_context *pipe,
>>>                     const struct gl_config *visual,
>>> diff --git a/src/mesa/state_tracker/st_manager.c
>>> b/src/mesa/state_tracker/st_manager.c
>>> index b065db0..e97b3f3 100644
>>> --- a/src/mesa/state_tracker/st_manager.c
>>> +++ b/src/mesa/state_tracker/st_manager.c
>>> @@ -285,7 +285,6 @@ st_framebuffer_add_renderbuffer(struct st_framebuffer
>>> *stfb,
>>>   {
>>>      struct gl_renderbuffer *rb;
>>>      enum pipe_format format;
>>> -   int samples;
>>>      boolean sw;
>>>
>>>      if (!stfb->iface)
>>> @@ -313,11 +312,7 @@ st_framebuffer_add_renderbuffer(struct st_framebuffer
>>> *stfb,
>>>      if (format == PIPE_FORMAT_NONE)
>>>         return FALSE;
>>>
>>> -   samples = stfb->iface->visual->samples;
>>> -   if (!samples)
>>> -      samples = st_get_msaa();
>>> -
>>> -   rb = st_new_renderbuffer_fb(format, samples, sw);
>>> +   rb = st_new_renderbuffer_fb(format, stfb->iface->visual->samples, sw);
>>>      if (!rb)
>>>         return FALSE;
>>>
>>
>>
>> The idea was to mimic NVIDIA's __GL_FSAA_MODE env var which allows you to
>> force MSAA on (and specify the MSAA mode).
>>
>> That said, I don't know if this actually works anymore or if anyone needs
>> it.  I'm OK with removing it if it's broken or unused.
> 
> As a matter of fact, it could have never worked. The correct way to
> force MSAA on is to change the GLX visual and then resolve the
> resource in SwapBuffers. What the hack does is that it only forces
> gl_renderbuffer::NumSamples to be multi-sample, while the underlying
> resource is still single-sample. Luckily it doesn't break anything at
> least.
> 
> I'll send a patch that implements the behavior of __GL_FSAA_MODE properly.

I'm pretty sure it worked at some point for some (non public source)
driver, this was way before there were the util bits for dealing with
msaa resolves (which said driver didn't need). I think it needed some
more hacks (which are gone too) and certainly this code looks safe to go
(but yes some way to force it would be of course nice).

Roland