On Wed, 07 Feb 2018 05:02:00 +0100, Yaodong Li <yaodong...@intel.com> wrote:



On 02/06/2018 02:56 PM, Michal Wajdeczko wrote:

+    /* Explicitly cast the return value to bool. */
+    return !!(I915_READ(reg) & GUC_WOPCM_REG_LOCKED);

you should avoid reusing bits defined for one register in others

it's a common bit. use BIT(0) instead?

How common?
Note that your function accepts all registers.
Btw, even for these two registers used below bit0 has different
name definitions (Locked vs Offset Valid) which we should use.



+    offset &= DMA_GUC_WOPCM_OFFSET_MASK;

maybe like this for easier reading:

u32 reg;

reg = I915_READ(GUC_WOPCM_SIZE);
size = reg & PAGE_MASK;

reg = I915_READ(DMA_GUC_WOPCM_OFFSET);
offset = reg & DMA_GUC_WOPCM_OFFSET_MASK;
hmm, this will clear the HUC_LOADING_AGENT_GUC.

that's expected

guc_loads_huc = reg & HUC_LOADING_AGENT_GUC;

+
+    return guc_loads_huc && (size == guc->wopcm.size) &&

what if we run in !USES_HUC() mode?

Do you mean the HUC_LOADING_AGENT bit?

this patch series is trying to align with the current driver logic. this bit
is current set regardless USES_HUC()

Are you suggest we should not set this bit for !USE_HUC() mode?

We can set it as before, but when we compare already initialized
registers we can ignore this bit if we're running without HuC.
It must match only if we use HuC.

+        (offset == guc->wopcm.offset);
  #define GEN9_GUC_WOPCM_OFFSET        (0x24000)
  +/* GuC WOPCM flags */
+#define INTEL_GUC_WOPCM_VALID        BIT(0)
+#define INTEL_GUC_WOPCM_HW_UPDATED    BIT(1)

maybe just

bool valid:1;
bool hw_updated:1;

I was trying to avoid bool in struct (really struggling with it actual size), maybe
u32 valid:1;
u32 hw_updated:1

bool:1 will work the same, try it !


Regards,
-Jackie

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to