|
smu_feature_set_enabled() is refreshed to update the stored copy only because there do has the use case. Another example is vega20_set_ppfeature_status() which needs also updating the stored feature masks only. That is missing now and is a bug.
I have noticed that for a while and do not have time to fix that yet. A new API smu_enable_smc_feature() is provided if you want to enable the hw feature also(plus updating stored copy). The naming is much more straightforward.
I cannot see any problem with these changes and cannot understand why you are so resistant to them. Regards, Evan From: Wang, Kevin(Yang) <Kevin1.Wang@xxxxxxx> it seems this feature is specific feature on some asic in smc firmware. why not create a specific function to handle this case, the apis of smu_feature_is_enabled and smu_feature_set_enabled is public api for all smu asic. may be this case not work on other smu or other asic. if you still do, it means that when developer use smu_feature_is_enabled api, the user must know the message detail information in smc firmware. sw-bitmap and smc firwmare bitmap must be same in smu driver. if not, this sw-bitmap is meaningless. Best Regards, Kevin From: Quan, Evan <Evan.Quan@xxxxxxx> The implementation of SMU_MSG_PowerDown[UP]Vcn (in smu fw) already includes the identical hardware enablement/disablement of SMU_FEATURE_VCN_PG_BIT. Thus after that, only update the bitmask is enough. This is done on purpose. Regards, Evan From: Wang, Kevin(Yang) <Kevin1.Wang@xxxxxxx>
in fact, the smu feature bitmap is cached of smc firmware hardware bitmap, most of time, when driver want to check whether enable is available. the driver should talk with smc to check featue enabled. i think it is very low efficiency. so the driver will provide a bitmap structure to store hardware feature status. every time, the driver update hardware feature state, also should be update software bitmap to sync it. if only update bitmap and don't set firmware feature enabled, the smu_feature_is_enabeld is maybe not work correctly. in your previous patch: drm/amd/powerplay: correct Navi10 VCN powergate control. after your patch, the function smu_feature_is_enabled(smu, SMU_FEATURE_VCN_PG_BIT) will retrun true, but in the firmware, this feature maybe is not enabled. so the function smu_feature_is_enabled is not work well. and smu_feature_is_supported is full software feature, this is helper function to set allowed feature mask when smu power on, but this function is not used in smu driver, i want to remove them long long ago. Best Regards, From: Quan, Evan <Evan.Quan@xxxxxxx> I cannot get your point. What do you mean “pairs of functions”? Yes, this patch does not bring real changes. But this helps for future maintain and fit common logic. 1. As in my previous patch(“drm/amd/powerplay: correct Navi10 VCN powergate control” ), “smu_feature_set_enabled(smu, SMU_FEATURE_VCN_PG_BIT, enable);” was mistakenly used. I thought and expected smu_feature_set_enabled set the bitmask
on only. I do not want it to enable or disable the feature. 2. smu_feature_set_enabled should in the same logic as smu_feature_set_supported. It updates only the saved bit mask. That’s the expected behavior for some APIs named as _set_enabled/supported. 3. The original callers who want feature enablement/disablement are updated to use smu_enable_smc_feature(). I do not see any problem with that. Regards, Evan From: Wang, Kevin(Yang) <Kevin1.Wang@xxxxxxx>
before this patch, we have 4 apis to manage smu feature bits.
the patch add a new one in them, but it is not add any feature in smu. before your patch: smu_feature_is_enabled and smu_feature_set_enabled is pair of functions, after your patch: smu_feature_is_enabled and smu_enable_smc_feature is pair of functions; so the driver don't have scenario needs to use smu_feature_set_enabeld, do you agree it? most of the time we update SMC feature, we must update software bitmap in smu_feature structure. if not, the smu_feature_is_enabled funciton is not work well. extern int smu_feature_init_dpm(struct smu_context *smu); extern int smu_feature_is_enabled(struct smu_context *smu, enum smu_feature_mask mask); extern int smu_feature_set_enabled(struct smu_context *smu, enum smu_feature_mask mask, bool enable); extern int smu_feature_is_supported(struct smu_context *smu, enum smu_feature_mask mask); extern int smu_feature_set_supported(struct smu_context *smu, enum smu_feature_mask mask, bool enable); Best Regards, From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> on behalf of Evan Quan
<evan.quan@xxxxxxx> It does more than updating the bitmask. In fact it enables also the |
_______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
