[Mesa-dev] [PATCH 14/20] radeonsi: reserve a parameter slot for FMASK resources in shaders

[Michel Dänzer]

On Fre, 2013-08-09 at 08:30 -0700, Tom Stellard wrote:
> On Fri, Aug 09, 2013 at 07:35:02AM -0700, Tom Stellard wrote:
> > On Fri, Aug 09, 2013 at 07:54:11AM +0200, Michel Dänzer wrote:
> > > On Don, 2013-08-08 at 11:32 -0700, Tom Stellard wrote:
> > > > On Thu, Aug 08, 2013 at 05:36:09PM +0200, Michel Dänzer wrote:
> > > > > On Don, 2013-08-08 at 08:00 -0700, Tom Stellard wrote:
> > > > > > On Thu, Aug 08, 2013 at 02:20:54AM +0200, Marek Olšák wrote:
> > > > > > > 
> > > > > > > diff --git a/src/gallium/drivers/radeonsi/si_state_draw.c b/src/gallium/drivers/radeonsi/si_state_draw.c
> > > > > > > index 746ace6..4208fa7 100644
> > > > > > > --- a/src/gallium/drivers/radeonsi/si_state_draw.c
> > > > > > > +++ b/src/gallium/drivers/radeonsi/si_state_draw.c
> > > > > > > @@ -241,6 +241,7 @@ static void si_pipe_shader_ps(struct pipe_context *ctx, struct si_pipe_shader *s
> > > > > > >  		/* Last 2 reserved SGPRs are used for VCC */
> > > > > > >  		num_sgprs = num_user_sgprs + 2;
> > > > > > >  	}
> > > > > > > +	num_sgprs += 1; /* XXX this fixes VM faults */
> > > > > > 
> > > > > > One problem is that the compiler is under reporting the number of SGPRs,
> > > > > > when there are unused USER_SGPRs in the shader.  It should always be
> > > > > > reporting a number greater than or equal to the number of USER_SGPRs.
> > > > > 
> > > > > That itself shouldn't be a problem, as the radeonsi code ensures this.
> > > > >
> > > > 
> > > > The way radeonsi checks for this on pixel shaders won't work in all
> > > > cases.  For example, if there are 8 USER_SGPR and the shader uses VCC,
> > > > then the backend should report 10 SGPRs used.  This value is eventually
> > > > rounded up to the next multiple of 8, so what is actually stored in
> > > > shader->num_sgprs is 16.
> > > > 
> > > > Now lets say that the shader doesn't see 2 of the USER_SGPRs, it will
> > > > report 8 SGPRs (6 USER_SGPRs + 2 for VCC).  This value is rounded up to
> > > > the nearest multiple of 8, which is still 8, and stored in shader->num_sgprs.
> > > > 
> > > > Since the number of SGPRs reported matches the number of USER_SGPRs,
> > > > the code in si_pipe_shader_ps() will accepted the reported number of
> > > > SGPRs as valid, even though it isn't.
> > > 
> > > Why isn't it valid? In your example, the shader doesn't use more than 8
> > > SGPRs, and doesn't use SGPR6 and SGPR7 for anything but VCC, right?
> > > 
> > 
> > My understanding is that USER_SGPRs count towards the total number of
> > SGPRs even if they are not used, so VCC would go into SGPR8 and SGPR9.
> > 
> 
> I think I am wrong about this.  From the docs, it appears that VCC
> always uses the highest allocated SGPRs, so if you allocate 8, it
> will use  SGPR6, SGPR7, and if you allocate 16, it will use
> SGPR14,SGPR15.
> 
> The real problem with this patch is that the shaders are using 8
> USER_SGPRs + 1 SGPR {prim_mask, lds_offset} which is always loaded for pixel
> shaders, so we should be allocating at least 16 SGPRs.

Right, I totally forgot about that additional SGPR pre-loaded in the
pixel shader. :( Thanks for the reminder.

Marek, does the patch below work instead of the line above?


> > The checks as they are now don't actually check anything, since num_sgprs
> > is always greater than or equal to num_user_sgprs,.

Not true if the shader doesn't use the last user SGPRs, and the pixel
shader doesn't interpolate any parameters.

> > I think the best think to do is move all this logic into the
> > compiler backend.

That would require the LLVM backend to always know which SGPRs are
pre-loaded, which I'm not sure makes sense, as it may depend on 3D
hardware state. The current separation of responsibilities between the
LLVM backend (reporting how many SGPRs are actually used by the shader
code) and the driver (ensuring any pre-loaded SGPRs are allocated as
well) kind of makes sense to me.


diff --git a/src/gallium/drivers/radeonsi/si_state_draw.c b/src/gallium/drivers/radeonsi/si_state_draw.c
index e8c7cd5..84789af 100644
--- a/src/gallium/drivers/radeonsi/si_state_draw.c
+++ b/src/gallium/drivers/radeonsi/si_state_draw.c
@@ -231,9 +231,10 @@ static void si_pipe_shader_ps(struct pipe_context *ctx, struct si_pipe_shader *s

        num_user_sgprs = SI_PS_NUM_USER_SGPR;
        num_sgprs = shader->num_sgprs;
-       if (num_user_sgprs > num_sgprs) {
+       /* One SGPR after user SGPRs is pre-loaded with {prim_mask, lds_offset} */
+       if ((num_user_sgprs + 1) > num_sgprs) {
                /* Last 2 reserved SGPRs are used for VCC */
-               num_sgprs = num_user_sgprs + 2;
+               num_sgprs = num_user_sgprs + 1 + 2;
        }
        assert(num_sgprs <= 104);



-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer