[Mesa-dev] [PATCH 59/59] i965/fs: fix MOV_INDIRECT exec_size for doubles

[Jason Ekstrand]

On May 2, 2016 12:31 AM, "Iago Toral" <itoral at igalia.com> wrote:
>
> On Sun, 2016-05-01 at 20:04 -0700, Jordan Justen wrote:
> > On 2016-04-29 04:29:56, Samuel Iglesias Gonsálvez wrote:
> > > In that case, the writes need two times the size of a 32-bit value.
> > > We need to adjust the exec_size, so it is not breaking any hardware
> > > rule.
> > >
> > > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_fs.cpp | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > > index 059edc6..85d430c 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> > > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > > @@ -4655,7 +4655,13 @@ get_lowered_simd_width(const struct
brw_device_info *devinfo,
> > >
> > >     case SHADER_OPCODE_MOV_INDIRECT:
> > >        /* Prior to Broadwell, we only have 8 address subregisters */
> > > -      return devinfo->gen < 8 ? 8 : MIN2(inst->exec_size, 16);
> > > +      if (devinfo->gen < 8)
> > > +         return 8;
> > > +
> > > +      if (inst->exec_size < 16)
> > > +         return inst->exec_size;
> > > +      else
> > > +         return MIN2(inst->exec_size / (type_sz(inst->dst.type) /
4), 16);
> >
> > Can't type_sz return < 4, which could cause a divide by 0? If that
> > can't happen along this code path, would an assert be a good idea?
>
> Looking at the places where the driver inserts MOV INDIRECT opcodes it
> seems that it always uses region sizes that are multiples of 4 and dst
> types are usually UD (except in load uniform where I don't think we can
> see any types that are not at least 4 bytes, since we implement bool as
> UD too), so it seems to me that we shouldn't be seeing types with sizes
> < 4 here.

At stone point in the future, it may be used for half-float types.  I'm not
to worried about that case at the moment though.

> That said, I am adding Jason in the CC just in case I am missing
> something.
>
> In any case, adding the assert for extra safety sounds good to me.
>
> > -Jordan
> >
> > >
> > >     default:
> > >        return inst->exec_size;
> > > --
> > > 2.5.0
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160502/fe57ceb1/attachment-0001.html>