Re: [PATCH v10 6/7] 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/14/2018 09:07 AM, Michal Wajdeczko wrote:
On Wed, 14 Feb 2018 02:18:29 +0100, Yaodong Li <yaodong.li@xxxxxxxxx> wrote:

On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:


<snip>

+
+#define DEFINE_LOCKABLE_REG(var, rg, lb, func)    \
+    struct lockable_reg var = {    \
+        .name = #rg,    \
+        .guc = guc_wopcm_to_guc(guc_wopcm),    \

btw, implicit macro params are evil...
Agree. but seems we always use similar approach in
I915_READ/WRITE().O:-)

True, but the only reason why it is still not fixed is that
it requires changes in too many places ...
Sure. will avoid to use implicit params then:-) Thanks.

<snip>

+ * @load_huc_fw: whether need to configure GuC to load HuC firmware.

I'm not sure that we need to track this flag inside structure.
It is just a parameter for doing partitioning and final check.

I think it's related to actual reg configuration. Any suggestions since
we do need it in hw_init to setup offset reg?

You can do partitioning at intel_wopcm level, but program hw at intel_uc
level when you know for sure if HuC is in use. Note that there is no wrong
in using results from WOPCM partitioning outside of intel_wopmc.

As mentioned before, we can avoid this flag and "valid" flag if do
partitioning and validation *before* writing final results to the
struct.
In current code, we do verify the ggtt offset against wopcm top in our current code which means current code won't trust the fact that ggtt offset would never be used after uc/guc init failed.

But after failure in WOPCM partitioning, which should be done sooner than
uc init, we should abort driver load, so there shall be no access to ggtt.

I would like to have more discussion on intel_wopcm over intel_guc_wopcm. :-)
This is the reason for this valid bit (which clearly suggests the struct is ready to use) - I won't

I'm not sure that "valid" flag approach is correct - you should trust your code path, otherwise you will have to add "valid" flags to every structure
and this is not a scalable solution ;)

True. I need some courage here:-). I would doubt myself after seeing guc_ggtt_offset validates every offset against the wopcm->guc.top which is the only reason I kept using
this flag:-(

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