| Message ID | 1513119394-28951-3-git-send-email-yaodong.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote: > Hardware may have specific restrictions on GuC WOPCM partition > size versus HuC firmware size. With static WOPCM partitioning, > there's no way to adjust the GuC WOPCM partition size based on > the actual HuC firmware size, so that GuC/HuC loading failure > would occur even if there was enough WOPCM space for both > GuC and HuC firmware. WOPCM being a shared feature of the hardware, it should not go under intel_guc_ prefix. There should be a clear division of what is specific to GuC feature only and what is just a feature that happens to be used by GuC (and equally can be used by HuC too). Regards, Joonas
On 12/13/2017 01:11 AM, Joonas Lahtinen wrote: > On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote: >> Hardware may have specific restrictions on GuC WOPCM partition >> size versus HuC firmware size. With static WOPCM partitioning, >> there's no way to adjust the GuC WOPCM partition size based on >> the actual HuC firmware size, so that GuC/HuC loading failure >> would occur even if there was enough WOPCM space for both >> GuC and HuC firmware. > WOPCM being a shared feature of the hardware, it should not go under > intel_guc_ prefix. > > There should be a clear division of what is specific to GuC feature > only and what is just a feature that happens to be used by GuC (and > equally can be used by HuC too). the intel_guc_wopcm here only refers to the wopcm used by GuC, this structure only defines the GuC related wopcm info. (wopcm partition for GuC). We only need to set these values (defined in this structure) to GuC registers. And this structure should never be touched if GuC was disabled. so it should be a part of GuC. Regards, Jackie > > Regards, Joonas
On Wed, 13 Dec 2017 19:19:06 +0100, Yaodong Li <yaodong.li@intel.com> wrote: > On 12/13/2017 01:11 AM, Joonas Lahtinen wrote: >> On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote: >>> Hardware may have specific restrictions on GuC WOPCM partition >>> size versus HuC firmware size. With static WOPCM partitioning, >>> there's no way to adjust the GuC WOPCM partition size based on >>> the actual HuC firmware size, so that GuC/HuC loading failure >>> would occur even if there was enough WOPCM space for both >>> GuC and HuC firmware. >> WOPCM being a shared feature of the hardware, it should not go under >> intel_guc_ prefix. >> >> There should be a clear division of what is specific to GuC feature >> only and what is just a feature that happens to be used by GuC (and >> equally can be used by HuC too). > the intel_guc_wopcm here only refers to the wopcm used by > GuC, this structure only defines the GuC related wopcm info. > (wopcm partition for GuC). We only need to set these values > (defined in this structure) to GuC registers. And this structure > should never be touched if GuC was disabled. so it should be > a part of GuC. > But note that yours intel_guc_wopcm is just one of many wopcm partitions. I think it would be a good idea to create "intel_wopcm.c|h" and keep all related code and data there (including verification of early setup done by bios, wopcpm reporting, partitioning). Then we can do rest of the programming right there or just take values that will be programmed individually by interested components (but former is preferred to avoid spreading single feature code over too many places) Michal
On 12/13/2017 01:34 PM, Michal Wajdeczko wrote: > On Wed, 13 Dec 2017 19:19:06 +0100, Yaodong Li <yaodong.li@intel.com> > wrote: > >> On 12/13/2017 01:11 AM, Joonas Lahtinen wrote: >>> On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote: >>>> Hardware may have specific restrictions on GuC WOPCM partition >>>> size versus HuC firmware size. With static WOPCM partitioning, >>>> there's no way to adjust the GuC WOPCM partition size based on >>>> the actual HuC firmware size, so that GuC/HuC loading failure >>>> would occur even if there was enough WOPCM space for both >>>> GuC and HuC firmware. >>> WOPCM being a shared feature of the hardware, it should not go under >>> intel_guc_ prefix. >>> >>> There should be a clear division of what is specific to GuC feature >>> only and what is just a feature that happens to be used by GuC (and >>> equally can be used by HuC too). >> the intel_guc_wopcm here only refers to the wopcm used by >> GuC, this structure only defines the GuC related wopcm info. >> (wopcm partition for GuC). We only need to set these values >> (defined in this structure) to GuC registers. And this structure >> should never be touched if GuC was disabled. so it should be >> a part of GuC. >> > > But note that yours intel_guc_wopcm is just one of many wopcm partitions. > I think it would be a good idea to create "intel_wopcm.c|h" and keep > all related code and data there (including verification of early setup > done by bios, wopcpm reporting, partitioning). > > Then we can do rest of the programming right there or just take values > that > will be programmed individually by interested components (but former is > preferred to avoid spreading single feature code over too many places) > The KMD only needs to take care of the setup of the GuC WOPCM partition. Other HW WOPCM (e.g HuC) usages are all transparent to kernel driver. Plus, the GuC WOPM partitioning is needed only when GuC is enabled and uc firmwares are loaded correctly. The only reason for us to have an intel_wopcm is to maintain the overall WOPCM info such as WOPCM size and base. However, it's not necessary since we can reuse existing driver code to get these info. Regards, -Jackie > Michal
On Wed, 2017-12-13 at 14:59 -0800, Yaodong Li wrote: > On 12/13/2017 01:34 PM, Michal Wajdeczko wrote: > > On Wed, 13 Dec 2017 19:19:06 +0100, Yaodong Li <yaodong.li@intel.com> > > wrote: > > > > > On 12/13/2017 01:11 AM, Joonas Lahtinen wrote: > > > > On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote: > > > > > Hardware may have specific restrictions on GuC WOPCM partition > > > > > size versus HuC firmware size. With static WOPCM partitioning, > > > > > there's no way to adjust the GuC WOPCM partition size based on > > > > > the actual HuC firmware size, so that GuC/HuC loading failure > > > > > would occur even if there was enough WOPCM space for both > > > > > GuC and HuC firmware. > > > > > > > > WOPCM being a shared feature of the hardware, it should not go under > > > > intel_guc_ prefix. > > > > > > > > There should be a clear division of what is specific to GuC feature > > > > only and what is just a feature that happens to be used by GuC (and > > > > equally can be used by HuC too). > > > > > > the intel_guc_wopcm here only refers to the wopcm used by > > > GuC, this structure only defines the GuC related wopcm info. > > > (wopcm partition for GuC). We only need to set these values > > > (defined in this structure) to GuC registers. And this structure > > > should never be touched if GuC was disabled. so it should be > > > a part of GuC. > > > > > > > But note that yours intel_guc_wopcm is just one of many wopcm partitions. > > I think it would be a good idea to create "intel_wopcm.c|h" and keep > > all related code and data there (including verification of early setup > > done by bios, wopcpm reporting, partitioning). > > > > Then we can do rest of the programming right there or just take values > > that > > will be programmed individually by interested components (but former is > > preferred to avoid spreading single feature code over too many places) > > > > The KMD only needs to take care of the setup of the GuC WOPCM partition. > Other > HW WOPCM (e.g HuC) usages are all transparent to kernel driver. Plus, > the GuC WOPM > partitioning is needed only when GuC is enabled and uc firmwares are > loaded correctly. > The only reason for us to have an intel_wopcm is to maintain the overall > WOPCM info > such as WOPCM size and base. However, it's not necessary since we can > reuse existing > driver code to get these info. I'd go with Michal here, the WOPCM is its own entity in existence. Partitioning defintely sounds like it should be intel_wopcm stuff, which may yield intel_wopcm_partition under "guc", so then you are still able to reference "guc->wopcm.base" where it makes sense. And how that partition is programmed to GuC registers for it to be used, is then stuff to go under intel_guc. And then you have another intel_wopcm_partition for "huc". We should avoid incorrect abstractions, just to avoid a few lines of code. That's how the hardware features seem to exist, that's how we should map them in the code. Regards, Joonas
On 12/14/2017 03:43 AM, Joonas Lahtinen wrote: > On Wed, 2017-12-13 at 14:59 -0800, Yaodong Li wrote: >> On 12/13/2017 01:34 PM, Michal Wajdeczko wrote: >>> On Wed, 13 Dec 2017 19:19:06 +0100, Yaodong Li <yaodong.li@intel.com> >>> wrote: >>> >>>> On 12/13/2017 01:11 AM, Joonas Lahtinen wrote: >>>>> On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote: >>>>>> Hardware may have specific restrictions on GuC WOPCM partition >>>>>> size versus HuC firmware size. With static WOPCM partitioning, >>>>>> there's no way to adjust the GuC WOPCM partition size based on >>>>>> the actual HuC firmware size, so that GuC/HuC loading failure >>>>>> would occur even if there was enough WOPCM space for both >>>>>> GuC and HuC firmware. >>>>> WOPCM being a shared feature of the hardware, it should not go under >>>>> intel_guc_ prefix. >>>>> >>>>> There should be a clear division of what is specific to GuC feature >>>>> only and what is just a feature that happens to be used by GuC (and >>>>> equally can be used by HuC too). >>>> the intel_guc_wopcm here only refers to the wopcm used by >>>> GuC, this structure only defines the GuC related wopcm info. >>>> (wopcm partition for GuC). We only need to set these values >>>> (defined in this structure) to GuC registers. And this structure >>>> should never be touched if GuC was disabled. so it should be >>>> a part of GuC. >>>> >>> But note that yours intel_guc_wopcm is just one of many wopcm partitions. >>> I think it would be a good idea to create "intel_wopcm.c|h" and keep >>> all related code and data there (including verification of early setup >>> done by bios, wopcpm reporting, partitioning). >>> >>> Then we can do rest of the programming right there or just take values >>> that >>> will be programmed individually by interested components (but former is >>> preferred to avoid spreading single feature code over too many places) >>> >> The KMD only needs to take care of the setup of the GuC WOPCM partition. >> Other >> HW WOPCM (e.g HuC) usages are all transparent to kernel driver. Plus, >> the GuC WOPM >> partitioning is needed only when GuC is enabled and uc firmwares are >> loaded correctly. >> The only reason for us to have an intel_wopcm is to maintain the overall >> WOPCM info >> such as WOPCM size and base. However, it's not necessary since we can >> reuse existing >> driver code to get these info. > I'd go with Michal here, the WOPCM is its own entity in existence. > Partitioning defintely sounds like it should be intel_wopcm stuff, > which may yield intel_wopcm_partition under "guc", so then you are > still able to reference "guc->wopcm.base" where it makes sense. > > And how that partition is programmed to GuC registers for it to be > used, is then stuff to go under intel_guc. And then you have another > intel_wopcm_partition for "huc". > > We should avoid incorrect abstractions, just to avoid a few lines of > code. That's how the hardware features seem to exist, that's how we > should map them in the code. Thanks for your comments. but I have some different opinions. Agreed that wopcm exists no matter GuC is enabled or not. And we can reuse existing code to get/verify related info we need for driver level description of wopcm. that one reason I don't think we need intel_wopcm. Regarding the partitioning - We need it only when GuC was enabled. In this case, it makes sense to do it at least in uc level. Plus, from HW point of view, HW only relies on GuC wopcm offset and size to determine the layout (or say partitions) of the wopcm. In this case, a good abstraction of the HW interface would be: struct guc_wopcm { u32 offset; u32 size; }; guc_wopcm_setup() - which does actual HW status check and GuC wopcm setup. guc_wopcm_init() - which init/verify the offset and size values required by HW. That's the second reason I think use of intel_guc_wopcm.c is more accurate since it reflected the actual HW interface and could be enabled/disabled along with GuC code. Regarding the generic abstraction of intel_wopcm_partition for both GuC & HuC. I am not sure what's the benefit of such an abstraction. For two reasons: a) HW is only aware of the GuC WOPCM boundaries and doesn't provide any interface to configure the partition for HuC, which means we even won't use these info in the rest of the driver code. b) For debugging and tracking propose, we can easily get overall layout of WOPCM by just using overall wopcm description and GuC wopcm usage. Please do let me know if anything was wrong :-) Regards, -Jackie > Regards, Joonas
On Thu, 2017-12-14 at 20:55 -0800, Yaodong Li wrote: > On 12/14/2017 03:43 AM, Joonas Lahtinen wrote: > > On Wed, 2017-12-13 at 14:59 -0800, Yaodong Li wrote: > > > On 12/13/2017 01:34 PM, Michal Wajdeczko wrote: > > > > On Wed, 13 Dec 2017 19:19:06 +0100, Yaodong Li <yaodong.li@intel.com> > > > > wrote: > > > > > > > > > On 12/13/2017 01:11 AM, Joonas Lahtinen wrote: > > > > > > On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote: > > > > > > > Hardware may have specific restrictions on GuC WOPCM partition > > > > > > > size versus HuC firmware size. With static WOPCM partitioning, > > > > > > > there's no way to adjust the GuC WOPCM partition size based on > > > > > > > the actual HuC firmware size, so that GuC/HuC loading failure > > > > > > > would occur even if there was enough WOPCM space for both > > > > > > > GuC and HuC firmware. > > > > > > > > > > > > WOPCM being a shared feature of the hardware, it should not go under > > > > > > intel_guc_ prefix. > > > > > > > > > > > > There should be a clear division of what is specific to GuC feature > > > > > > only and what is just a feature that happens to be used by GuC (and > > > > > > equally can be used by HuC too). > > > > > > > > > > the intel_guc_wopcm here only refers to the wopcm used by > > > > > GuC, this structure only defines the GuC related wopcm info. > > > > > (wopcm partition for GuC). We only need to set these values > > > > > (defined in this structure) to GuC registers. And this structure > > > > > should never be touched if GuC was disabled. so it should be > > > > > a part of GuC. > > > > > > > > > > > > > But note that yours intel_guc_wopcm is just one of many wopcm partitions. > > > > I think it would be a good idea to create "intel_wopcm.c|h" and keep > > > > all related code and data there (including verification of early setup > > > > done by bios, wopcpm reporting, partitioning). > > > > > > > > Then we can do rest of the programming right there or just take values > > > > that > > > > will be programmed individually by interested components (but former is > > > > preferred to avoid spreading single feature code over too many places) > > > > > > > > > > The KMD only needs to take care of the setup of the GuC WOPCM partition. > > > Other > > > HW WOPCM (e.g HuC) usages are all transparent to kernel driver. Plus, > > > the GuC WOPM > > > partitioning is needed only when GuC is enabled and uc firmwares are > > > loaded correctly. > > > The only reason for us to have an intel_wopcm is to maintain the overall > > > WOPCM info > > > such as WOPCM size and base. However, it's not necessary since we can > > > reuse existing > > > driver code to get these info. > > > > I'd go with Michal here, the WOPCM is its own entity in existence. > > Partitioning defintely sounds like it should be intel_wopcm stuff, > > which may yield intel_wopcm_partition under "guc", so then you are > > still able to reference "guc->wopcm.base" where it makes sense. > > > > And how that partition is programmed to GuC registers for it to be > > used, is then stuff to go under intel_guc. And then you have another > > intel_wopcm_partition for "huc". > > > > We should avoid incorrect abstractions, just to avoid a few lines of > > code. That's how the hardware features seem to exist, that's how we > > should map them in the code. > > Thanks for your comments. but I have some different opinions. > > Agreed that wopcm exists no matter GuC is enabled or not. And we > can reuse existing code to get/verify related info we need for driver level > description of wopcm. that one reason I don't think we need intel_wopcm. > > Regarding the partitioning - We need it only when GuC was enabled. In this > case, it makes sense to do it at least in uc level. Plus, from HW point > of view, > HW only relies on GuC wopcm offset and size to determine the layout > (or say partitions) of the wopcm. In this case, a good abstraction of > the HW > interface would be: > struct guc_wopcm { > u32 offset; > u32 size; > }; > guc_wopcm_setup() - which does actual HW status check and GuC wopcm > setup. > guc_wopcm_init() - which init/verify the offset and size values > required by HW. > That's the second reason I think use of intel_guc_wopcm.c is more accurate > since it reflected the actual HW interface and could be enabled/disabled > along with GuC code. > > Regarding the generic abstraction of intel_wopcm_partition for both GuC > & HuC. > I am not sure what's the benefit of such an abstraction. For two reasons: > a) HW is only aware of the GuC WOPCM boundaries and doesn't provide any > interface > to configure the partition for HuC, which means we even won't use > these info in > the rest of the driver code. > b) For debugging and tracking propose, we can easily get overall layout > of WOPCM > by just using overall wopcm description and GuC wopcm usage. > It's literally an entity called WOPCM, which is partitioned and one of the partitions is used for GuC. I don't see how many more resons you need for intel_wopcm prefix, struct intel_wopcm_partition abstraction and struct intel_wopcm_partition instance for GuC? Why would we try to make the naming scheme to imply something else, it'll make the developer's life harder when trying to look at it. I had to go look at the spec to make any sense of this, so let's try to avoid that for the next developer. Regards, Joonas
On 12/15/2017 02:21 AM, Joonas Lahtinen wrote: > On Thu, 2017-12-14 at 20:55 -0800, Yaodong Li wrote: >> On 12/14/2017 03:43 AM, Joonas Lahtinen wrote: >>> On Wed, 2017-12-13 at 14:59 -0800, Yaodong Li wrote: >>>> On 12/13/2017 01:34 PM, Michal Wajdeczko wrote: >>>>> On Wed, 13 Dec 2017 19:19:06 +0100, Yaodong Li <yaodong.li@intel.com> >>>>> wrote: >>>>> >>>>>> On 12/13/2017 01:11 AM, Joonas Lahtinen wrote: >>>>>>> On Tue, 2017-12-12 at 14:56 -0800, Jackie Li wrote: >>>>>>>> Hardware may have specific restrictions on GuC WOPCM partition >>>>>>>> size versus HuC firmware size. With static WOPCM partitioning, >>>>>>>> there's no way to adjust the GuC WOPCM partition size based on >>>>>>>> the actual HuC firmware size, so that GuC/HuC loading failure >>>>>>>> would occur even if there was enough WOPCM space for both >>>>>>>> GuC and HuC firmware. >>>>>>> WOPCM being a shared feature of the hardware, it should not go under >>>>>>> intel_guc_ prefix. >>>>>>> >>>>>>> There should be a clear division of what is specific to GuC feature >>>>>>> only and what is just a feature that happens to be used by GuC (and >>>>>>> equally can be used by HuC too). >>>>>> the intel_guc_wopcm here only refers to the wopcm used by >>>>>> GuC, this structure only defines the GuC related wopcm info. >>>>>> (wopcm partition for GuC). We only need to set these values >>>>>> (defined in this structure) to GuC registers. And this structure >>>>>> should never be touched if GuC was disabled. so it should be >>>>>> a part of GuC. >>>>>> >>>>> But note that yours intel_guc_wopcm is just one of many wopcm partitions. >>>>> I think it would be a good idea to create "intel_wopcm.c|h" and keep >>>>> all related code and data there (including verification of early setup >>>>> done by bios, wopcpm reporting, partitioning). >>>>> >>>>> Then we can do rest of the programming right there or just take values >>>>> that >>>>> will be programmed individually by interested components (but former is >>>>> preferred to avoid spreading single feature code over too many places) >>>>> >>>> The KMD only needs to take care of the setup of the GuC WOPCM partition. >>>> Other >>>> HW WOPCM (e.g HuC) usages are all transparent to kernel driver. Plus, >>>> the GuC WOPM >>>> partitioning is needed only when GuC is enabled and uc firmwares are >>>> loaded correctly. >>>> The only reason for us to have an intel_wopcm is to maintain the overall >>>> WOPCM info >>>> such as WOPCM size and base. However, it's not necessary since we can >>>> reuse existing >>>> driver code to get these info. >>> I'd go with Michal here, the WOPCM is its own entity in existence. >>> Partitioning defintely sounds like it should be intel_wopcm stuff, >>> which may yield intel_wopcm_partition under "guc", so then you are >>> still able to reference "guc->wopcm.base" where it makes sense. >>> >>> And how that partition is programmed to GuC registers for it to be >>> used, is then stuff to go under intel_guc. And then you have another >>> intel_wopcm_partition for "huc". >>> >>> We should avoid incorrect abstractions, just to avoid a few lines of >>> code. That's how the hardware features seem to exist, that's how we >>> should map them in the code. >> Thanks for your comments. but I have some different opinions. >> >> Agreed that wopcm exists no matter GuC is enabled or not. And we >> can reuse existing code to get/verify related info we need for driver level >> description of wopcm. that one reason I don't think we need intel_wopcm. >> >> Regarding the partitioning - We need it only when GuC was enabled. In this >> case, it makes sense to do it at least in uc level. Plus, from HW point >> of view, >> HW only relies on GuC wopcm offset and size to determine the layout >> (or say partitions) of the wopcm. In this case, a good abstraction of >> the HW >> interface would be: >> struct guc_wopcm { >> u32 offset; >> u32 size; >> }; >> guc_wopcm_setup() - which does actual HW status check and GuC wopcm >> setup. >> guc_wopcm_init() - which init/verify the offset and size values >> required by HW. >> That's the second reason I think use of intel_guc_wopcm.c is more accurate >> since it reflected the actual HW interface and could be enabled/disabled >> along with GuC code. >> >> Regarding the generic abstraction of intel_wopcm_partition for both GuC >> & HuC. >> I am not sure what's the benefit of such an abstraction. For two reasons: >> a) HW is only aware of the GuC WOPCM boundaries and doesn't provide any >> interface >> to configure the partition for HuC, which means we even won't use >> these info in >> the rest of the driver code. >> b) For debugging and tracking propose, we can easily get overall layout >> of WOPCM >> by just using overall wopcm description and GuC wopcm usage. >> > It's literally an entity called WOPCM, which is partitioned and one of > the partitions is used for GuC. I don't see how many more resons you > need for intel_wopcm prefix, struct intel_wopcm_partition abstraction > and struct intel_wopcm_partition instance for GuC? > > Why would we try to make the naming scheme to imply something else, > it'll make the developer's life harder when trying to look at it. I had > to go look at the spec to make any sense of this, so let's try to avoid > that for the next developer. Actually I started the patch with both intel_wopcm and intel_wopcm_partition defined and implemented. The more I think about it the more I felt it makes sense to keep the code within GuC level since the main work of these code is quite simple - trying to find valid values for GuC wopcm. that's the reason why I renamed it from intel_wopcm_partition to intel_guc_wopcm because with intel_wopcm_partition defined, it will be confusing that we only have intel_wopcm_partition defined for GuC. Sorry for the naming thing. It kept changing along with the code reviewing, one reason probably because I really wanted to catch up with all these good ideas and provide a solid solution to this problem. Again these are my own understanding. If you still think we should use a intel_wopcm.c|h for these changes. I definitely will respect it and try to make things happen in that way :-) I will hold the submission until we agree on the code structure. Regards, -Jackie > > Regards, Joonas
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 21ce374..016acec 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -312,12 +312,13 @@ __create_hw_context(struct drm_i915_private *dev_priv, ctx->desc_template = default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt); - /* GuC requires the ring to be placed above GUC_WOPCM_TOP. If GuC is not - * present or not in use we still need a small bias as ring wraparound - * at offset 0 sometimes hangs. No idea why. + /* + * GuC requires the ring to be placed above GuC WOPCM top. If GuC is not +- * present or not in use we still need a small bias as ring wraparound +- * at offset 0 sometimes hangs. No idea why. */ if (USES_GUC(dev_priv)) - ctx->ggtt_offset_bias = GUC_WOPCM_TOP; + ctx->ggtt_offset_bias = dev_priv->guc.wopcm.top; else ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE; diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index 41aee96..585a644 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -63,6 +63,7 @@ void intel_guc_init_early(struct intel_guc *guc) { intel_guc_fw_init_early(guc); intel_guc_ct_init_early(&guc->ct); + intel_guc_wopcm_init_early(&guc->wopcm); mutex_init(&guc->send_mutex); guc->send = intel_guc_send_nop; @@ -339,7 +340,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv) * This is a wrapper to create an object for use with the GuC. In order to * use it inside the GuC, an object needs to be pinned lifetime, so we allocate * both some backing storage and a range inside the Global GTT. We must pin - * it in the GGTT somewhere other than than [0, GUC_WOPCM_TOP) because that + * it in the GGTT somewhere other than than [0, GuC WOPCM top) because that * range is reserved inside GuC. * * Return: A i915_vma if successful, otherwise an ERR_PTR. @@ -360,7 +361,7 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size) goto err; ret = i915_vma_pin(vma, 0, PAGE_SIZE, - PIN_GLOBAL | PIN_OFFSET_BIAS | GUC_WOPCM_TOP); + PIN_GLOBAL | PIN_OFFSET_BIAS | guc->wopcm.top); if (ret) { vma = ERR_PTR(ret); goto err; diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index 399612d..b8248ba 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -49,6 +49,7 @@ struct intel_guc { struct intel_uc_fw fw; struct intel_guc_log log; struct intel_guc_ct ct; + struct intel_guc_wopcm wopcm; /* Log snapshot if GuC errors during load */ struct drm_i915_gem_object *load_err_log; @@ -105,10 +106,10 @@ static inline void intel_guc_notify(struct intel_guc *guc) * @guc: intel guc. * @vma: i915 graphics virtual memory area. * - * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP), - * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top address is - * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects - * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM. + * GuC does not allow any gfx GGTT address that falls into range + * [0, GuC WOPCM top), which is reserved for Boot ROM, SRAM and WOPCM. + * All gfx objects used by GuC is pinned with PIN_OFFSET_BIAS along with + * top of WOPCM. * * Return: GGTT offset that meets the GuC gfx address requirement. */ @@ -117,7 +118,8 @@ static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc, { u32 offset = i915_ggtt_offset(vma); - GEM_BUG_ON(offset < GUC_WOPCM_TOP); + GEM_BUG_ON(!guc->wopcm.valid); + GEM_BUG_ON(offset < guc->wopcm.top); GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP)); return offset; diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c b/drivers/gpu/drm/i915/intel_guc_wopcm.c index 87643a0..60a6afe 100644 --- a/drivers/gpu/drm/i915/intel_guc_wopcm.c +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c @@ -25,23 +25,108 @@ #include "intel_guc_wopcm.h" #include "i915_drv.h" +static inline u32 guc_reserved_wopcm_size(struct intel_guc *guc) +{ + struct drm_i915_private *i915 = guc_to_i915(guc); + + /* On BXT, the top of WOPCM is reserved for RC6 context */ + if (IS_GEN9_LP(i915)) + return BXT_WOPCM_RC6_RESERVED; + + return 0; +} + +static inline int gen9_wocpm_size_check(struct drm_i915_private *i915) +{ + struct intel_guc_wopcm *wopcm = &i915->guc.wopcm; + u32 wopcm_base; + u32 delta; + + /* + * Check hardware restriction on Gen9 + * GuC WOPCM size is at least 4 bytes larger than GuC WOPCM base due + * to hardware limitation on Gen9. + */ + wopcm_base = wopcm->offset + GEN9_GUC_WOPCM_OFFSET; + if (unlikely(wopcm_base > wopcm->size)) + return -E2BIG; + + delta = wopcm->size - wopcm_base; + if (unlikely(delta < GEN9_GUC_WOPCM_DELTA)) + return -E2BIG; + + return 0; +} + +static inline int guc_wopcm_size_check(struct intel_guc *guc) +{ + struct drm_i915_private *i915 = guc_to_i915(guc); + + if (IS_GEN9(i915)) + return gen9_wocpm_size_check(i915); + + return 0; +} + /* - * intel_guc_wopcm_size() - Get the size of GuC WOPCM. + * intel_guc_wopcm_init() - Initialize the GuC WOPCM partition. * @guc: intel guc. + * @guc_fw_size: size of GuC firmware. + * @huc_fw_size: size of HuC firmware. * - * Get the platform specific GuC WOPCM size. + * This function tries to initialize the WOPCM partition based on HuC firmware + * size and the reserved WOPCM memory size. * - * Return: size of the GuC WOPCM. + * Return: 0 on success, non-zero error code on failure. */ -u32 intel_guc_wopcm_size(struct intel_guc *guc) +int intel_guc_init_wopcm(struct intel_guc *guc, u32 guc_fw_size, + u32 huc_fw_size) { - struct drm_i915_private *i915 = guc_to_i915(guc); + u32 reserved = guc_reserved_wopcm_size(guc); + u32 offset, size, top; + int err; - u32 wopcm_size = GUC_WOPCM_TOP; + if (guc->wopcm.valid) + return 0; - /* On BXT, the top of WOPCM is reserved for RC6 context */ - if (IS_GEN9_LP(i915)) - wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED; + if (!guc_fw_size) + return -EINVAL; + + if (reserved >= WOPCM_DEFAULT_SIZE) + return -E2BIG; + + offset = huc_fw_size + WOPCM_RESERVED_SIZE; + if (offset >= WOPCM_DEFAULT_SIZE) + return -E2BIG; + + /* Hardware requires GuC WOPCM offset needs to be 16K aligned. */ + offset = ALIGN(offset, WOPCM_OFFSET_ALIGNMENT); + if ((offset + reserved) >= WOPCM_DEFAULT_SIZE) + return -E2BIG; + + top = WOPCM_DEFAULT_SIZE - offset; + size = top - reserved; + + /* + * GuC size needs to be less than or equal to GuC WOPCM size. + * Need extra 8K stack for GuC. + */ + if ((guc_fw_size + GUC_WOPCM_STACK_RESERVED) > size) + return -E2BIG; + + guc->wopcm.offset = offset; + guc->wopcm.size = size; + guc->wopcm.top = top; + + /* Check platform specific restrictions */ + err = guc_wopcm_size_check(guc); + if (err) + return err; + + guc->wopcm.valid = true; + + DRM_DEBUG_DRIVER("GuC WOPCM offset %dKB, size %dKB, top %dKB\n", + offset >> 10, size >> 10, top >> 10); - return wopcm_size; + return 0; } diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h b/drivers/gpu/drm/i915/intel_guc_wopcm.h index 04d61c8..2f9d03a 100644 --- a/drivers/gpu/drm/i915/intel_guc_wopcm.h +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h @@ -29,10 +29,41 @@ struct intel_guc; -/* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */ -#define GUC_WOPCM_TOP (0x80 << 12) /* 512KB */ -#define BXT_GUC_WOPCM_RC6_RESERVED (0x10 << 12) /* 64KB */ +/* Default WOPCM size 1MB */ +#define WOPCM_DEFAULT_SIZE (0x1 << 20) +/* Reserved WOPCM size 16KB */ +#define WOPCM_RESERVED_SIZE (0x4000) +/* GUC WOPCM Offset need to be 16KB aligned */ +#define WOPCM_OFFSET_ALIGNMENT (0x4000) +/* 8KB stack reserved for GuC FW*/ +#define GUC_WOPCM_STACK_RESERVED (0x2000) +/* 24KB WOPCM reserved for RC6 CTX on BXT */ +#define BXT_WOPCM_RC6_RESERVED (0x6000) -u32 intel_guc_wopcm_size(struct intel_guc *guc); +#define GEN9_GUC_WOPCM_DELTA 4 +#define GEN9_GUC_WOPCM_OFFSET (0x24000) + +struct intel_guc_wopcm { + u32 offset; + u32 size; + u32 top; + bool valid; +}; + +/* + * intel_guc_wopcm_init_early() - Early initialization of the GuC WOPCM. + * @wopcm: GuC WOPCM. + * + * Setup the GuC WOPCM top to the top of the overall WOPCM. This will guarantee + * that the allocation of the GuC accessible objects won't fall into WOPCM when + * GuC partition isn't present. + * + */ +static inline void intel_guc_wopcm_init_early(struct intel_guc_wopcm *wopcm) +{ + wopcm->top = WOPCM_DEFAULT_SIZE; +} + +int intel_guc_init_wopcm(struct intel_guc *guc, u32 guc_size, u32 huc_size); #endif diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c index 6482174..dda2955 100644 --- a/drivers/gpu/drm/i915/intel_huc.c +++ b/drivers/gpu/drm/i915/intel_huc.c @@ -217,7 +217,7 @@ int intel_huc_auth(struct intel_huc *huc) return -ENOEXEC; vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0, - PIN_OFFSET_BIAS | GUC_WOPCM_TOP); + PIN_OFFSET_BIAS | guc->wopcm.top); if (IS_ERR(vma)) { ret = PTR_ERR(vma); DRM_ERROR("HuC: Failed to pin huc fw object %d\n", ret); diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 44deb80..d5c2a95 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -191,6 +191,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) { struct intel_guc *guc = &dev_priv->guc; struct intel_huc *huc = &dev_priv->huc; + u32 guc_fw_size = intel_uc_fw_get_size(&guc->fw); + u32 huc_fw_size = intel_uc_fw_get_size(&huc->fw); int ret, attempts; if (!USES_GUC(dev_priv)) @@ -201,6 +203,10 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) goto err_out; } + ret = intel_guc_init_wopcm(guc, guc_fw_size, huc_fw_size); + if (ret) + goto err_out; + guc_disable_communication(guc); gen9_reset_guc_interrupts(dev_priv); @@ -218,9 +224,9 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) } /* init WOPCM */ - I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(guc)); + I915_WRITE(GUC_WOPCM_SIZE, guc->wopcm.size); I915_WRITE(DMA_GUC_WOPCM_OFFSET, - GUC_WOPCM_OFFSET_VALUE | HUC_LOADING_AGENT_GUC); + guc->wopcm.offset | HUC_LOADING_AGENT_GUC); /* WaEnableuKernelHeaderValidFix:skl */ /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */ diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c index 24945cf..791263a 100644 --- a/drivers/gpu/drm/i915/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/intel_uc_fw.c @@ -95,9 +95,13 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size; uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32); - /* Header and uCode will be loaded to WOPCM */ + /* + * Header and uCode will be loaded to WOPCM + * Only check the size against the overall available WOPCM here. Will + * continue to check the size during WOPCM partition calculation. + */ size = uc_fw->header_size + uc_fw->ucode_size; - if (size > intel_guc_wopcm_size(&dev_priv->guc)) { + if (size > WOPCM_DEFAULT_SIZE) { DRM_WARN("%s: Firmware is too large to fit in WOPCM\n", intel_uc_fw_type_repr(uc_fw->type)); err = -E2BIG; @@ -207,6 +211,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, int (*xfer)(struct intel_uc_fw *uc_fw, struct i915_vma *vma)) { + struct drm_i915_private *i915 = to_i915(uc_fw->obj->base.dev); struct i915_vma *vma; int err; @@ -230,7 +235,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, } vma = i915_gem_object_ggtt_pin(uc_fw->obj, NULL, 0, 0, - PIN_OFFSET_BIAS | GUC_WOPCM_TOP); + PIN_OFFSET_BIAS | i915->guc.wopcm.top); if (IS_ERR(vma)) { err = PTR_ERR(vma); DRM_DEBUG_DRIVER("%s fw ggtt-pin err=%d\n", diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h index d5fd460..d00d888 100644 --- a/drivers/gpu/drm/i915/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/intel_uc_fw.h @@ -115,6 +115,22 @@ static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw) return uc_fw->path != NULL; } +/* + * intel_uc_fw_get_size() - Get the size of the firmware. + * @uc_fw: intel_uc_fw structure. + * + * Get the size of the firmware that will be placed in WOPCM. + * + * Return: Zero on invalid firmware status. actual size on success. + */ +static inline u32 intel_uc_fw_get_size(struct intel_uc_fw *uc_fw) +{ + if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) + return 0; + + return uc_fw->header_size + uc_fw->ucode_size; +} + void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, struct intel_uc_fw *uc_fw); int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
Hardware may have specific restrictions on GuC WOPCM partition size versus HuC firmware size. With static WOPCM partitioning, there's no way to adjust the GuC WOPCM partition size based on the actual HuC firmware size, so that GuC/HuC loading failure would occur even if there was enough WOPCM space for both GuC and HuC firmware. This patch enables the dynamic calculation of the WOPCM aperture sizes used by GuC and HuC firmware. GuC WOPCM offset is set to HuC size + reserved WOPCM size. GuC WOPCM size is set to total WOPCM size - GuC WOPCM offset - RC6CTX size. In this case, GuC WOPCM offset will be updated based on the size of HuC firmware while GuC WOPCM size will be set to use all the remaining WOPCM space. v2: - Removed intel_wopcm_init (Ville/Sagar/Joonas) - Renamed and Moved the intel_wopcm_partition into intel_guc (Sagar) - Removed unnecessary function calls (Joonas) - Init GuC WOPCM partition as soon as firmware fetching is completed v3: - Fixed indentation issues (Chris) - Removed layering violation code (Chris/Michal) - Created separat files for GuC wopcm code (Michal) - Used inline function to avoid code duplication (Michal) v4: - Preset the GuC WOPCM top during early GuC init (Chris) - Fail intel_uc_init_hw() as soon as GuC WOPCM partitioning failed Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: John Spotswood <john.a.spotswood@intel.com> Cc: Oscar Mateo <oscar.mateo@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Signed-off-by: Jackie Li <yaodong.li@intel.com> --- drivers/gpu/drm/i915/i915_gem_context.c | 9 +-- drivers/gpu/drm/i915/intel_guc.c | 5 +- drivers/gpu/drm/i915/intel_guc.h | 12 ++-- drivers/gpu/drm/i915/intel_guc_wopcm.c | 105 +++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_guc_wopcm.h | 39 ++++++++++-- drivers/gpu/drm/i915/intel_huc.c | 2 +- drivers/gpu/drm/i915/intel_uc.c | 10 ++- drivers/gpu/drm/i915/intel_uc_fw.c | 11 +++- drivers/gpu/drm/i915/intel_uc_fw.h | 16 +++++ 9 files changed, 178 insertions(+), 31 deletions(-)