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

[Marek Olšák]

On Fri, Aug 9, 2013 at 6:00 PM, Michel Dänzer <michel at daenzer.net> wrote:
> 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?

Yes, it fixes the VM faults too. Feel free to commit it.

Marek