[Intel-gfx] [PATCH 3/3] drm/i915/guc: Increase size of CTB buffers
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Nov 21 12:25:05 UTC 2019
On Thu, 21 Nov 2019 00:56:04 +0100, <John.C.Harrison at intel.com> wrote:
> From: Matthew Brost <matthew.brost at intel.com>
>
> With the introduction of non-blocking CTBs more than one CTB can be in
> flight at a time. Increasing the size of the CTBs should reduce how
> often software hits the case where no space is available in the CTB
> buffer.
>
> Cc: John Harrison <john.c.harrison at intel.com>
> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 50 +++++++++++++++--------
> 1 file changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> index 4d8a4c6afd71..31c512e7ecc2 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -14,6 +14,10 @@
> #define CT_DEBUG_DRIVER(...) do { } while (0)
> #endif
> +#define CTB_DESC_SIZE (PAGE_SIZE / 2)
CTB descriptors is now 64B each, not sure why we want to waste
whole page for them. Maybe to better use space (and be ready for
upcoming changes) we can place the buffer right after descriptor:
* <------ DESCRIPTOR ------> <--- BUFFER ------------->
*
* +--------------------------+--------------------------+
* | addr | head | tail | ... | |
* +--------------------------+--------------------------+
* \ ^
* \____________________/
*
* <------------ 64B -------> <---- n * PAGE - 64B ---->
> +#define CTB_H2G_BUFFER_SIZE (PAGE_SIZE)
> +#define CTB_G2H_BUFFER_SIZE (CTB_H2G_BUFFER_SIZE * 2)
> +
> #define MAX_RETRY 0x1000000
> struct ct_request {
> @@ -143,30 +147,35 @@ static int ctch_init(struct intel_guc *guc,
> GEM_BUG_ON(ctch->vma);
> - /* We allocate 1 page to hold both descriptors and both buffers.
> + /* We allocate 3 pages to hold both descriptors and both buffers.
> * ___________.....................
> * |desc (SEND)| :
> - * |___________| PAGE/4
> + * |___________| PAGE/2
> * :___________....................:
> * |desc (RECV)| :
> - * |___________| PAGE/4
> + * |___________| PAGE/2
> * :_______________________________:
> * |cmds (SEND) |
> - * | PAGE/4
> + * | PAGE
> * |_______________________________|
> * |cmds (RECV) |
> - * | PAGE/4
> + * | PAGE * 2
> * |_______________________________|
> *
> * Each message can use a maximum of 32 dwords and we don't expect to
> - * have more than 1 in flight at any time, so we have enough space.
> - * Some logic further ahead will rely on the fact that there is only 1
> - * page and that it is always mapped, so if the size is changed the
> - * other code will need updating as well.
> + * have more than 1 in flight at any time, unless we are using the GuC
> + * submission. In that case each request requires a minimum 8 bytes
> + * which gives us a maximum 512 queue'd requests. Hopefully this enough
hmm, do we really expect to have 512 messages in flight ?
> + * space to avoid backpressure on the driver. We also double the size
> of
> + * the receive buffer (relative to the send) to ensure a g2h response
> + * CTB has a landing spot.
We do plan to send nob-blocking messages that might generate higher
traffic, but
do we expect matching increase in incoming traffic ? what kind of data
will be
there ? and do we expect that driver will be unable consume them in timely
manner?
> */
> /* allocate vma */
> - vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
> + vma = intel_guc_allocate_vma(guc, CTB_DESC_SIZE *
> + ARRAY_SIZE(ctch->ctbs) +
> + CTB_H2G_BUFFER_SIZE +
> + CTB_G2H_BUFFER_SIZE);
> if (IS_ERR(vma)) {
> err = PTR_ERR(vma);
> goto err_out;
> @@ -185,8 +194,9 @@ static int ctch_init(struct intel_guc *guc,
> /* store pointers to desc and cmds */
> for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) {
> GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));
> - ctch->ctbs[i].desc = blob + PAGE_SIZE/4 * i;
> - ctch->ctbs[i].cmds = blob + PAGE_SIZE/4 * i + PAGE_SIZE/2;
> + ctch->ctbs[i].desc = blob + CTB_DESC_SIZE * i;
> + ctch->ctbs[i].cmds = blob + CTB_H2G_BUFFER_SIZE * i +
> + CTB_DESC_SIZE * ARRAY_SIZE(ctch->ctbs);
> }
> return 0;
> @@ -210,7 +220,7 @@ static void ctch_fini(struct intel_guc *guc,
> static int ctch_enable(struct intel_guc *guc,
> struct intel_guc_ct_channel *ctch)
> {
> - u32 base;
> + u32 base, size;
> int err;
> int i;
> @@ -226,9 +236,12 @@ static int ctch_enable(struct intel_guc *guc,
> */
> for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) {
> GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));
> + size = (i == CTB_SEND) ? CTB_H2G_BUFFER_SIZE :
> + CTB_G2H_BUFFER_SIZE;
> guc_ct_buffer_desc_init(&ctch->ctbs[i],
> - base + PAGE_SIZE/4 * i + PAGE_SIZE/2,
> - PAGE_SIZE/4,
> + base + CTB_H2G_BUFFER_SIZE * i +
> + CTB_DESC_SIZE * ARRAY_SIZE(ctch->ctbs),
> + size,
> ctch->owner);
> }
> @@ -236,13 +249,13 @@ static int ctch_enable(struct intel_guc *guc,
> * descriptors are in first half of the blob
> */
> err = guc_action_register_ct_buffer(guc,
> - base + PAGE_SIZE/4 * CTB_RECV,
> + base + CTB_DESC_SIZE * CTB_RECV,
> INTEL_GUC_CT_BUFFER_TYPE_RECV);
> if (unlikely(err))
> goto err_out;
> err = guc_action_register_ct_buffer(guc,
> - base + PAGE_SIZE/4 * CTB_SEND,
> + base + CTB_DESC_SIZE * CTB_SEND,
> INTEL_GUC_CT_BUFFER_TYPE_SEND);
> if (unlikely(err))
> goto err_deregister;
> @@ -635,7 +648,8 @@ static int ctb_read(struct intel_guc_ct_buffer *ctb,
> u32 *data)
> /* beware of buffer wrap case */
> if (unlikely(available < 0))
> available += size;
> - CT_DEBUG_DRIVER("CT: available %d (%u:%u)\n", available, head, tail);
> + CT_DEBUG_DRIVER("CT: available %d (%u:%u:%d)\n", available, head, tail,
> + size);
size will not change, not sure if it is worth to repeat that in every log
> GEM_BUG_ON(available < 0);
> data[0] = cmds[head];
More information about the Intel-gfx
mailing list