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
