[Beignet] [PATCH 1/2] enable scratch memory allocation and read/write

[zhigang gong]

On Wed, Aug 7, 2013 at 1:44 PM, Song, Ruiling <ruiling.song at intel.com>wrote:

>  ** **
>
> ** **
>
> *From:* zhigang gong [mailto:zhigang.gong at gmail.com]
> *Sent:* Wednesday, August 07, 2013 1:04 PM
> *To:* Song, Ruiling
> *Cc:* beignet at lists.freedesktop.org
> *Subject:* Re: [Beignet] [PATCH 1/2] enable scratch memory allocation and
> read/write****
>
> ** **
>
> My suggestion here is to seprate the scratch OBlock read/write out. And
> submit the****
>
> Dword Read/Write part firstly, as OBlock read/write is not used right now,
> Right?****
>
> [Ruiling] In fact, I first use OBlock Read/Write for spill implementation.
> As I met some failure, so, I also implement scratch HBlock. I call it
> HBlock as it always read multiples of HWord. There are two modes of scratch
> RW, but I only enable DWORD
>
  I think you should use Dword to keep align with the spec.  It has reason
that although it read data start from Hword alignment,
 but it only use the part of the data according to the data type. For
example, if the data type is Dword then it only put the first 8 Dword
 of data to the destination registers.

> mode. That is the HBlock in the patch. If you have concern about the oword
> block rw, I can remove OBlock RW logic.
>
+  void GenEncoder::SCRATCH_READ_HWORD(GenRegister dst, GenRegister src,
> uint32_t offset, uint32_t size, uint32_t dst_num)
>
> +  {
> +     assert(dst_num == 1 || dst_num ==2);
> +     uint32_t block_size = dst_num == 1 ? GEN_SCRATCH_BLOCK_SIZE_1 :
> GEN_SCRATCH_BLOCK_SIZE_2;
> +     GenInstruction *insn = this->next(GEN_OPCODE_SEND);
> +     this->setHeader(insn);
> +     this->setDst(insn, dst);
> +     this->setSrc0(insn, src);
> +     this->setSrc1(insn, GenRegister::immud(0));
> +      // here dst_num is the register that will be write-back: in terms
> of 32byte register
> +     setScratchMessage(this, insn, offset, block_size,
> GEN_SCRATCH_DATA_DWORD, GEN_SCRATCH_READ, 1, dst_num);
> +  }
> +
> +  void GenEncoder::SCRATCH_WRITE_OWORD(GenRegister msg, uint32_t size,
> uint32_t src_num)
> +  {
> +     GenInstruction *insn = this->next(GEN_OPCODE_SEND);
> +     this->setHeader(insn);
> +     this->setDst(insn, GenRegister::retype(GenRegister::null(),
> GEN_TYPE_UD));
> +     this->setSrc0(insn, msg);
> +     this->setSrc1(insn, GenRegister::immud(0));
> +     // here src_num means registers that will be write out: in terms of
> 32byte register number
> +     setOBlockRW(this, insn, 255, size/16, GEN_OBLOCK_WRITE, src_num+1,
> 0);
> +  }****
>
>    I'm confused here, you prepared a message header for  Scratch OWord
>  read/write, but then you send a OWord Block Read****
>
>  message. I don't think it's going to work as desired. You can check
> the  "OWord Block Read/Write" in the spec, you can find****
>
>  a restriction there:****
>
>    the only surface type allowed is SURFTYPE_BUFFER.****
>
>  And here, the surface is a scratch buffer.****
>
> [Ruiling] scratch buffer is stateless buffer, i.e. SURFTYPE_BUFFER. It is
> more preferred to call “OWord block read/write” instead of “scratch OWord
> block read/write”. This is not what written as oword mode under scratch
> memory read/write.
>

 You are right, the scratch buffer is one type of SURFTYPE_BUFFER and can
be accessed by OWord block read/write.
 What confused me is you are using SCRATCH_WRITE_OWORD as the function
name. You know there are real
 Scratch Block Read/Write for OWord which is quite different from this
OWord Bloc read. Right? They have different
 message descriptor and different payload/writeback message.

 Ben had implemented the OBlock read/write extension before. And for some
reason he removed those implementations.
 If you really want to use OBlock read/write here, it may worth to find
what Ben already implemented before and see what we
 can reuse here.  And maybe you can get some hint that why you failed to
use the OBlock read/write to implement the spill/unspill.

 Anyway, use scratch_write/read_oword is quite misleading here, and should
be corrected. What do you think?

>
>
  I'm thinking of is there a way to test the scratch read/write directly?
>  Maybe it's not so straightforward as it's only****
>
>  used when spill/unspill occurs.  And if a kernel triggers spill/unspill,
> it means the kernel is not very simple and can't****
>
>  be a proper unit test case for scratch read/write. Any good idea?****
>
> [Ruiling] this is also a problem I met. At last I only can manually call
> spillReg() to select one virtual register I want, so, I don’t have to face
> a complex kernel. But I don’t have good way of verifying scratch RW.****
>
>  - Zhigang****
>
> --
> 1.7.9.5
>
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet****
>
>  ** **
>
> ** **
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/beignet/attachments/20130807/662672a2/attachment.html>