[Intel-gfx] [PATCH 04/12] drm/i915/uc: introduce intel_uc_fw_supported
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Wed Jul 10 21:46:25 UTC 2019
On 7/10/19 9:57 AM, Michal Wajdeczko wrote:
> On Wed, 10 Jul 2019 02:54:29 +0200, Daniele Ceraolo Spurio
> <daniele.ceraolospurio at intel.com> wrote:
>
>> Instead of always checking in the device config is GuC and HuC are
>
> s/is/if
>
>> supported or not, we can save the state in the uc_fw structure and
>> avoid going through i915 every time from the low-level uc management
>> code. while at it FIRMWARE_NONE has been renamed to better indicate that
>
> s/while/While
>
>> we haven't started the fetch/load yet, but we might have already selected
>> a blob.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_guc_fw.c | 6 +++++-
>> drivers/gpu/drm/i915/intel_huc_fw.c | 6 +++++-
>> drivers/gpu/drm/i915/intel_uc.c | 25 ++++++++++++------------
>> drivers/gpu/drm/i915/intel_uc_fw.c | 4 +++-
>> drivers/gpu/drm/i915/intel_uc_fw.h | 30 ++++++++++++++++++++++++-----
>> 5 files changed, 51 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c
>> b/drivers/gpu/drm/i915/intel_guc_fw.c
>> index db1e0daca7db..ee95d4960c5c 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
>> @@ -79,8 +79,12 @@ static void guc_fw_select(struct intel_uc_fw *guc_fw)
>> GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
>> - if (!HAS_GUC(i915))
>> + if (!HAS_GUC(i915)) {
>> + guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>> return;
>> + }
>> +
>> + guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
>> if (i915_modparams.guc_firmware_path) {
>> guc_fw->path = i915_modparams.guc_firmware_path;
>> diff --git a/drivers/gpu/drm/i915/intel_huc_fw.c
>> b/drivers/gpu/drm/i915/intel_huc_fw.c
>> index 05cbf8338f53..06e726ba9863 100644
>> --- a/drivers/gpu/drm/i915/intel_huc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_huc_fw.c
>> @@ -73,8 +73,12 @@ static void huc_fw_select(struct intel_uc_fw *huc_fw)
>> GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
>> - if (!HAS_HUC(dev_priv))
>> + if (!HAS_HUC(dev_priv)) {
>> + huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>> return;
>> + }
>> +
>> + huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
>> if (i915_modparams.huc_firmware_path) {
>> huc_fw->path = i915_modparams.huc_firmware_path;
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 789b0bccfb41..ef2a864b8990 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -71,7 +71,8 @@ static int __get_default_guc_log_level(struct
>> drm_i915_private *i915)
>> {
>> int guc_log_level;
>> - if (!HAS_GUC(i915) || !intel_uc_is_using_guc(i915))
>> + if (!intel_uc_fw_supported(&i915->guc.fw) ||
>
> this goes too far, we should limit number of direct accesses to .fw
> maybe we can have:
>
> inline bool intel_uc_has_guc(i915)
> {
> return intel_guc_is_present(&i915->guc);
> }
>
> inline bool intel_guc_is_present(guc)
> {
> return intel_uc_fw_is_defined(&guc->fw);
> }
>
Maybe instead of saving this info in the uc_fw it'd be better to just
change guc_init_early to not do anything if !HAS_GUC and then do
something like:
bool intel_guc_is_present(guc)
{
return !!guc->send;
}
What do you think? otherwise I'll split it like you suggested
>> + !intel_uc_is_using_guc(i915))
>> guc_log_level = GUC_LOG_LEVEL_DISABLED;
>> else if (IS_ENABLED(CONFIG_DRM_I915_DEBUG) ||
>> IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
>> @@ -119,16 +120,16 @@ static void sanitize_options_early(struct
>> drm_i915_private *i915)
>> if (intel_uc_is_using_guc(i915) &&
>> !intel_uc_fw_is_selected(guc_fw)) {
>> DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
>> "enable_guc", i915_modparams.enable_guc,
>> - !HAS_GUC(i915) ? "no GuC hardware" :
>> - "no GuC firmware");
>> + !intel_uc_fw_supported(guc_fw) ?
>> + "no GuC hardware" : "no GuC firmware");
>> }
>> /* Verify HuC firmware availability */
>> if (intel_uc_is_using_huc(i915) &&
>> !intel_uc_fw_is_selected(huc_fw)) {
>> DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
>> "enable_guc", i915_modparams.enable_guc,
>> - !HAS_HUC(i915) ? "no HuC hardware" :
>> - "no HuC firmware");
>> + !intel_uc_fw_supported(huc_fw) ?
>> + "no HuC hardware" : "no HuC firmware");
>> }
>> /* XXX: GuC submission is unavailable for now */
>> @@ -148,8 +149,8 @@ static void sanitize_options_early(struct
>> drm_i915_private *i915)
>> if (i915_modparams.guc_log_level > 0 &&
>> !intel_uc_is_using_guc(i915)) {
>> DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
>> "guc_log_level", i915_modparams.guc_log_level,
>> - !HAS_GUC(i915) ? "no GuC hardware" :
>> - "GuC not enabled");
>> + !intel_uc_fw_supported(guc_fw) ?
>> + "no GuC hardware" : "GuC not enabled");
>> i915_modparams.guc_log_level = 0;
>> }
>> @@ -376,7 +377,7 @@ int intel_uc_init(struct drm_i915_private *i915)
>> if (!USES_GUC(i915))
>> return 0;
>> - if (!HAS_GUC(i915))
>> + if (!intel_uc_fw_supported(&guc->fw))
>> return -ENODEV;
>> /* XXX: GuC submission is unavailable for now */
>> @@ -419,7 +420,7 @@ void intel_uc_fini(struct drm_i915_private *i915)
>> if (!USES_GUC(i915))
>> return;
>> - GEM_BUG_ON(!HAS_GUC(i915));
>> + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
>> if (USES_GUC_SUBMISSION(i915))
>> intel_guc_submission_fini(guc);
>> @@ -435,7 +436,7 @@ static void __uc_sanitize(struct drm_i915_private
>> *i915)
>> struct intel_guc *guc = &i915->guc;
>> struct intel_huc *huc = &i915->huc;
>> - GEM_BUG_ON(!HAS_GUC(i915));
>> + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
>> intel_huc_sanitize(huc);
>> intel_guc_sanitize(guc);
>> @@ -460,7 +461,7 @@ int intel_uc_init_hw(struct drm_i915_private *i915)
>> if (!USES_GUC(i915))
>> return 0;
>> - GEM_BUG_ON(!HAS_GUC(i915));
>> + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
>> guc_reset_interrupts(guc);
>> @@ -557,7 +558,7 @@ void intel_uc_fini_hw(struct drm_i915_private *i915)
>> if (!intel_guc_is_loaded(guc))
>> return;
>> - GEM_BUG_ON(!HAS_GUC(i915));
>> + GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
>> if (USES_GUC_SUBMISSION(i915))
>> intel_guc_submission_disable(guc);
>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c
>> b/drivers/gpu/drm/i915/intel_uc_fw.c
>> index f342ddd47df8..8ce7210907c0 100644
>> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
>> @@ -47,6 +47,8 @@ void intel_uc_fw_fetch(struct drm_i915_private
>> *dev_priv,
>> size_t size;
>> int err;
>> + GEM_BUG_ON(!intel_uc_fw_supported(uc_fw));
>> +
>> if (!uc_fw->path) {
>> dev_info(dev_priv->drm.dev,
>> "%s: No firmware was defined for %s!\n",
>> @@ -328,7 +330,7 @@ void intel_uc_fw_cleanup_fetch(struct intel_uc_fw
>> *uc_fw)
>> if (obj)
>> i915_gem_object_put(obj);
>> - uc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
>> + uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
>> }
>> /**
>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h
>> b/drivers/gpu/drm/i915/intel_uc_fw.h
>> index 24e66469153c..833d04d06576 100644
>> --- a/drivers/gpu/drm/i915/intel_uc_fw.h
>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
>> @@ -26,6 +26,7 @@
>> #define _INTEL_UC_FW_H_
>> #include <linux/types.h>
>> +#include "i915_gem.h"
>> struct drm_printer;
>> struct drm_i915_private;
>> @@ -34,8 +35,10 @@ struct drm_i915_private;
>> #define INTEL_UC_FIRMWARE_URL
>> "https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915"
>>
>> enum intel_uc_fw_status {
>> + INTEL_UC_FIRMWARE_NOT_SUPPORTED = -2, /* no uc HW */
>> INTEL_UC_FIRMWARE_FAIL = -1,
>> - INTEL_UC_FIRMWARE_NONE = 0,
>> + INTEL_UC_FIRMWARE_UNINITIALIZED = 0, /* used to catch checks done
>> too early */
>> + INTEL_UC_FIRMWARE_NOT_STARTED = 1,
>> INTEL_UC_FIRMWARE_PENDING,
>> INTEL_UC_FIRMWARE_SUCCESS
>> };
>> @@ -79,10 +82,14 @@ static inline
>> const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
>> {
>> switch (status) {
>> + case INTEL_UC_FIRMWARE_NOT_SUPPORTED:
>
> maybe INTEL_UC_FIRMWARE_NOT_APPLICABLE, as used in string below?
ok
>
>> + return "N/A - uc HW not available";
>
> note that other states have short strings, so just "N/A" ?
ok
>
>> case INTEL_UC_FIRMWARE_FAIL:
>> return "FAIL";
>> - case INTEL_UC_FIRMWARE_NONE:
>> - return "NONE";
>> + case INTEL_UC_FIRMWARE_UNINITIALIZED:
>> + return "UNINITIALIZED";
>> + case INTEL_UC_FIRMWARE_NOT_STARTED:
>> + return "NOT_STARTED";
>
> NOT_STARTED and PENDING are not that much different,
> see below
NOT_STARTED is not a new case I've added, I've just renamed the NONE
case to indicate that we hadn't even started the fetch process, but we
might be past the FW selection.
>
>> case INTEL_UC_FIRMWARE_PENDING:
>> return "PENDING";
>> case INTEL_UC_FIRMWARE_SUCCESS:
>
> maybe we should drop split on fetch/load and use
> single status, something like:
>
> UNINITIALIZED --> N/A (no HW)
> |
> DEFINED ----> MISSING (no valid fw)
> |
> -> FETCHED/UNLOADED <-
> / | \
> \ LOADING /
> \ / \ /
> \- READY FAILED -/
>
>
Nothing against this, but I don't think it belongs in this series.
>> @@ -106,9 +113,15 @@ static inline
>> void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
>> enum intel_uc_fw_type type)
>> {
>> + /*
>> + * we use FIRMWARE_UNINITIALIZED to detect checks against
>> fetch_status
>> + * before we're looked at the HW caps to see if we have uc support
>> + */
>> + BUILD_BUG_ON(INTEL_UC_FIRMWARE_UNINITIALIZED);
>> +
>> uc_fw->path = NULL;
>> - uc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
>> - uc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>> + uc_fw->fetch_status = INTEL_UC_FIRMWARE_UNINITIALIZED;
>> + uc_fw->load_status = INTEL_UC_FIRMWARE_NOT_STARTED;
>> uc_fw->type = type;
>> }
>> @@ -122,6 +135,13 @@ static inline bool intel_uc_fw_is_loaded(struct
>> intel_uc_fw *uc_fw)
>> return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS;
>> }
>> +static inline bool intel_uc_fw_supported(struct intel_uc_fw *uc_fw)
>
> returning "supported" in case of "no blob/no defs" might be misleading
> maybe "defined" will clearer ?
>
My idea was to track if the HW had support for FW loading (i.e. if there
is a guc/huc in the GT), independently from us defining or finding a
firmware for it or not. hw_supported?
Daniele
>> +{
>> + /* shouldn't call this before checking hw/blob availability */
>> + GEM_BUG_ON(uc_fw->fetch_status == INTEL_UC_FIRMWARE_UNINITIALIZED);
>> + return uc_fw->fetch_status != INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>> +}
>> +
>> static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
>> {
>> if (intel_uc_fw_is_loaded(uc_fw))
More information about the Intel-gfx
mailing list