Hi Alex,
>>> + }
>>> +}
>>> diff --git a/drivers/gpu/nova-core/gsp/fw.rs
>>> b/drivers/gpu/nova-core/gsp/fw.rs
>>> index 09549f7db52d..228464fd47a0 100644
>>> --- a/drivers/gpu/nova-core/gsp/fw.rs
>>> +++ b/drivers/gpu/nova-core/gsp/fw.rs
>>> @@ -209,6 +209,7 @@ pub(crate) enum MsgFunction {
>>> GspInitPostObjGpu = bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU,
>>> GspRmControl = bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL,
>>> GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
>>> + UnloadingGuestDriver =
>>> bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER,
>>>
>>> // Event codes
>>> GspInitDone = bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE,
>>> @@ -249,6 +250,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
>>> }
>>> bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL =>
>>> Ok(MsgFunction::GspRmControl),
>>> bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO =>
>>> Ok(MsgFunction::GetStaticInfo),
>>> + bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER => {
>>> + Ok(MsgFunction::UnloadingGuestDriver)
>>> + }
>>> bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE =>
>>> Ok(MsgFunction::GspInitDone),
>>> bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER => {
>>> Ok(MsgFunction::GspRunCpuSequencer)
>>> diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs
>>> b/drivers/gpu/nova-core/gsp/fw/commands.rs
>>> index 85465521de32..c7df4783ad21 100644
>>> --- a/drivers/gpu/nova-core/gsp/fw/commands.rs
>>> +++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
>>> @@ -125,3 +125,30 @@ unsafe impl AsBytes for GspStaticConfigInfo {}
>>> // SAFETY: This struct only contains integer types for which all bit
>>> patterns
>>> // are valid.
>>> unsafe impl FromBytes for GspStaticConfigInfo {}
>>> +
>>> +/// Payload of the `UnloadingGuestDriver` command and message.
>>> +#[repr(transparent)]
>>> +#[derive(Clone, Copy, Debug, Zeroable)]
>>> +pub(crate) struct UnloadingGuestDriver {
>>> + inner: bindings::rpc_unloading_guest_driver_v1F_07,
>>> +}
>>> +
>>> +impl UnloadingGuestDriver {
>>> + pub(crate) fn new(suspend: bool) -> Self {
>>> + Self {
>>> + inner: bindings::rpc_unloading_guest_driver_v1F_07 {
>>
>> We should go through intermediate firmware representation than direct
>> bindings access?
>
> Only if the size of the bindings justifies it - here having an opaque
> wrapper just just well, and spares us some code down the line as we
> would have to initialize the bindings anyway.
I am not sure about that, it sounds like a layering violation. It would be good
not to keep the rules fuzzy about this, because then we could do it either way
in all cases.
Another reason is that we cannot anticipate in advance which specific helper
functions we will need to add in the future. Down the line, we may need to add
some helper functions to the struct as well. Also having V1F07 in the name
sounds very magic number-ish. It would be good to abstract that out with a
better-named struct anyway.
Thanks,
- Joel
>
>>
>>
>>> + bInPMTransition: if suspend { 1 } else { 0 },
>>
>> Then this can just be passed as a bool.
>>
>>> + bGc6Entering: 0,
>>> + newLevel: if suspend { 3 } else { 0 },
>
> Note to self to figure out these magic numbers. :)
>
