Re: [PATCH v8 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

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

 



On 02/07/2018 09:24 AM, Michal Wajdeczko wrote:
On Wed, 07 Feb 2018 05:02:00 +0100, Yaodong Li <yaodong.li@xxxxxxxxx> 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.

this bit suggests the w-o registers were locked, right? (I mean
no matter how we call this bit).


+    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
the values here are expected to be the same as the values
set to registers. otherwise, we need update the registers.
in the case of !USES_HUC() if we would add the change
to set HUC_LOADING_AGENT bit accordingly. we still
need such a check, right?

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.

I can add these changes.
+        (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 !

I sure it's going to work:-) Just obsessed with the ideas that:
0) bool (_Bool) could be any length, but it's fine for us since we know the compiler and hw. 1) I cannot see any benefits for using bits definition for each flag for two reasons:
     a) it's won't provide any performance/code readability improvement.
     b) the struct definition would grow bigger visually and we need explain these are flags and          come back to modify the struct every time we need to add more flags.      One more reason:-) which may not apply here, but the beauty of use of flags is      we can do flags |= (flag 1 | flag 2) once instead of flags.flag1 = 1; flags.flag2 = 1;

So let keep it;-)? (please do let me know if I've made any nonsense assumptions again just like we
need explicitly cast int to bool with !! even the compiler already did so)

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