Re: [PATCH 5/8] drm/i915/guc: Use GuC FW size and HW restrictions to determine WOPCM partition

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Mar 23, 2018 at 01:00:35PM -0700, Yaodong Li wrote:
> On 03/23/2018 05:34 AM, Michał Winiarski wrote:
> > We need GuC to load HuC, but it's also possible for GuC to operate on
> > its own. We don't know what the size of HuC FW may be, so, not wanting
> > to make any platform-specific hardcoded guesses, we assume that its size
> > is 0... Which is a very bad approximation.
> > This has a very unfortunate consequence - once we've booted with GuC and
> > no HuC, we'll never be able to load HuC (unless we reboot, since the
> > registers are locked).
> > Rather than using unknown values in our computations - let's partition
> > based on GuC size.
> we can do this based on known GuC and HuC sizes without
> any assumption on FW sizes :)
> please also refer to: https://patchwork.freedesktop.org/series/40541/

You need to have HuC FW present in the filesystem do do that though.
(IIUC we still get 0 if it's not there)

> > We have one HW restriction where we're using HuC size (GuC size needs to
> > be roughly similar to HuC size - which may be unknown at this point),
> > luckily, another HW restriction makes it very unlikely to run in any
> > sort of issues in this case.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Jackie Li <yaodong.li@xxxxxxxxx>
> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/intel_wopcm.c | 60 +++++++++++++++++++++-----------------
> >   1 file changed, 34 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
> > index 52841d340002..295d302e97b9 100644
> > --- a/drivers/gpu/drm/i915/intel_wopcm.c
> > +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> > @@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd)
> >   	return 0;
> >   }
> > -static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
> > +static inline void
> > +__guc_region_grow(struct intel_wopcm *wopcm, u32 size)
> > +{
> > +	/*
> > +	 * We're growing guc region in the direction of lower addresses.
> > +	 * We need to use multiples of base alignment, because it has more
> > +	 * strict alignment rules.
> > +	 */
> > +	size = DIV_ROUND_UP(size, 2);
> A bit confused here. Don't we just want to push the base downward for
> *size* bytes?

Starting from the following:

    +--------------+           
    |--------------|           
    ||            ||           
    ||            ||           
    ||  GuC       ||           
    ||  region    ||           
    ||            ||           
    ||            ||           
    ||            ||           
    ||            ||           
    +--------------+           
    |              |           
    |              |           
    |              |           
    |              |           
    |              |           
    |              |           
    |              |           
    |              |           
    |              |           
    +--------------+           

We want to grow the whole region by size bytes. Pushing the base downward gives
us this:

    +--------------+        
    |  Empty       |        
    |  space       |        
    +--------------+        
    ||            ||        
    ||            ||        
    ||  GuC       ||        
    ||  region    ||        
    ||            ||        
    ||            ||        
    ||            ||        
    ||            ||        
    +--------------+        
    |              |        
    |              |        
    |              |        
    |              |        
    |              |        
    |              |        
    |              |        
    +--------------+        

Which leaves less space for HuC (and we're also leaving a bunch of unused space
in our partitioning).

If we modify both base and size to end up with this:

    +--------------+
    |--------------|
    ||            ||
    ||            ||
    ||  GuC       ||
    ||  region    ||
    ||            ||
    ||            ||
    ||            ||
    ||            ||
    ||            ||
    +--------------+
    |              |
    |              |
    |              |
    |              |
    |              |
    |              |
    |              |
    |              |
    +--------------+

We're still satisfying the HW restriciton, but we're not wasting any space from
the top (and also we're leaving more space for HuC).

> > +	size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT);
> Hmm, I think we only need align it to 4K boundary.

No - because we're modifying both base (16K alignment) and size (4K
alignment). 

> > +
> > +	wopcm->guc.base -= size;
> > +	wopcm->guc.size += size;
> > +}
> > +
> > +static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm)
> >   {
> >   	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> >   	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
> > @@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
> >   		size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
> >   							   wopcm->guc.size);
> >   		if (size)
> > -			goto err;
> > +			__guc_region_grow(wopcm, size);
> >   		size = gen9_size_for_huc_restriction(wopcm->guc.size,
> >   						     huc_fw_size);
> Note that huc_fw_size might be zero in enable_guc=1 (GuC only) case.
> Once the registers were locked, enable_guc=3 may still fail since
> huc_fw_size
> may still large enough to break the restriction we want to enforce that:
> hw_fw_size + 16KB > guc_fw_size.

Well - as mentioned in the commit message, since we also have a "dword gap"
restriction, we're left with pretty large GuC size, which makes this problem
dissaperar for real-world HuC FW sizes.

But yeah - I agree, we're not able to guarantee this.

> >   		if (size)
> > -			goto err;
> > -	}
> > -
> > -	return 0;
> > +			__guc_region_grow(wopcm, size);
> > -err:
> > -	DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
> > -		  wopcm->guc.size / 1024,
> > -		  size / 1024);
> > -
> > -	return -E2BIG;
> > +		GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base,
> > +							       wopcm->guc.size));
> > +		GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size,
> > +							 huc_fw_size));
> > +	}
> >   }
> >   static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
> > @@ -218,21 +229,16 @@ static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
> >   	return 0;
> >   }
> > -static int wopcm_guc_init(struct intel_wopcm *wopcm)
> > +static int wopcm_guc_region_init(struct intel_wopcm *wopcm)
> >   {
> >   	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
> > -	u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw);
> > +	u32 guc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->guc.fw);
> >   	u32 ctx_rsvd = context_reserved_size(dev_priv);
> > -	wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size),
> > -				     GUC_WOPCM_OFFSET_ALIGNMENT);
> > +	wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size);
> > -	wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd,
> > -				PAGE_SIZE);
> > -
> > -	DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n",
> > -			 wopcm->guc.base / 1024,
> > -			 (wopcm->guc.base + wopcm->guc.size) / 1024);
> > +	wopcm->guc.base = ALIGN_DOWN(wopcm->size - ctx_rsvd - wopcm->guc.size,
> > +				     GUC_WOPCM_OFFSET_ALIGNMENT);
> guc.size + ctx_rsvd > wopcm->size check?

It's being done in check_ctx_rsvd_fits later on (in wopcm_check_components_fit).

> >   	return 0;
> >   }
> > @@ -255,18 +261,20 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
> >   	GEM_BUG_ON(!wopcm->size);
> > -	err = wopcm_guc_init(wopcm);
> > +	err = wopcm_guc_region_init(wopcm);
> Again, wopcm will contain invalid data on failure. But maybe we can
> go with it now :)

I could go back to passing around a bunch of locals, but since we're only
accessing wopcm here I think passing around struct intel_wopcm makes the code a
bit cleaner. And we're failing the driver load on error anyways.

-Michał

> 
> Regards,
> -Jackie
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux