by Dikshita Agarwal

[permalink] [raw]

Subject: Re: [PATCH 14/33] iris: vidc: add helpers for state management

On 7/28/2023 11:22 PM, Konrad Dybcio wrote:
> On 28.07.2023 15:23, Vikash Garodia wrote:
>> This implements the functions to handle different core
>> and instance state transitions.
>>
>> Signed-off-by: Dikshita Agarwal <[email protected]>
>> Signed-off-by: Vikash Garodia <[email protected]>
>> ---
> [...]
>
>> +enum msm_vidc_core_sub_state {
>> + CORE_SUBSTATE_NONE = 0x0,
>> + CORE_SUBSTATE_POWER_ENABLE = BIT(0),
>> + CORE_SUBSTATE_GDSC_HANDOFF = BIT(1),
>> + CORE_SUBSTATE_PM_SUSPEND = BIT(2),
>> + CORE_SUBSTATE_FW_PWR_CTRL = BIT(3),
>> + CORE_SUBSTATE_PAGE_FAULT = BIT(4),
>> + CORE_SUBSTATE_CPU_WATCHDOG = BIT(5),
>> + CORE_SUBSTATE_VIDEO_UNRESPONSIVE = BIT(6),
>> + CORE_SUBSTATE_MAX = BIT(7),
> Why store it in an enum if they're not consecutive? You can make them
> preprocessor #defines.
>
I understand that these are not consecutive but a enum for these makes them
under one roof which is easy to read and maintain, we will loose this if
replaced with macro.
>> +};
>> +
>> +enum msm_vidc_core_event_type {
>> + CORE_EVENT_NONE = BIT(0),
>> + CORE_EVENT_UPDATE_SUB_STATE = BIT(1),
>> +};
> Ditto (even though techinically they're consecutive)
>
>> +
>> +enum msm_vidc_state {
>> + MSM_VIDC_OPEN,
>> + MSM_VIDC_INPUT_STREAMING,
>> + MSM_VIDC_OUTPUT_STREAMING,
>> + MSM_VIDC_STREAMING,
>> + MSM_VIDC_CLOSE,
>> + MSM_VIDC_ERROR,
>> +};
>> +
>> +#define MSM_VIDC_SUB_STATE_NONE 0
>> +#define MSM_VIDC_MAX_SUB_STATES 6
>> +/*
>> + * max value of inst->sub_state if all
>> + * the 6 valid bits are set i.e 111111==>63
>> + */
>> +#define MSM_VIDC_MAX_SUB_STATE_VALUE ((1 << MSM_VIDC_MAX_SUB_STATES) - 1)
>> +
>> +enum msm_vidc_sub_state {
>> + MSM_VIDC_DRAIN = BIT(0),
>> + MSM_VIDC_DRC = BIT(1),
>> + MSM_VIDC_DRAIN_LAST_BUFFER = BIT(2),
>> + MSM_VIDC_DRC_LAST_BUFFER = BIT(3),
>> + MSM_VIDC_INPUT_PAUSE = BIT(4),
>> + MSM_VIDC_OUTPUT_PAUSE = BIT(5),
> Ditto
>
these are bit wise and are being used in state machine. At a time, two or
more bits can be set to define the state of and instance, hence needed.

> [...]
>
>> +static int msm_vidc_core_init_wait_state(struct msm_vidc_core *core,
>> + enum msm_vidc_core_event_type type,
>> + struct msm_vidc_event_data *data)
>> +{
>> + int rc = 0;
> rc seems never assigned again, good to drop
>
> [...]
>
Sure, will remove in next version
>> +
>> +static int msm_vidc_core_init_state(struct msm_vidc_core *core,
>> + enum msm_vidc_core_event_type type,
>> + struct msm_vidc_event_data *data)
>> +{
>> + int rc = 0;
> Ditto
>
> [...]
>
>> +static int msm_vidc_core_error_state(struct msm_vidc_core *core,
>> + enum msm_vidc_core_event_type type,
>> + struct msm_vidc_event_data *data)
>> +{
>> + int rc = 0;
> Ditto
>
> [...]
>
>> +int msm_vidc_update_core_state(struct msm_vidc_core *core,
>> + enum msm_vidc_core_state request_state, const char *func)
>> +{
>> + struct msm_vidc_core_state_handle *state_handle = NULL;
>> + int rc = 0;
> Ditto
>
> [...]
>
>> +int msm_vidc_change_core_state(struct msm_vidc_core *core,
>> + enum msm_vidc_core_state request_state, const char *func)
>> +{
>> + enum msm_vidc_allow allow;
>> + int rc = 0;
> Ditto
>
will remove all such instances of unused rc in next version
> [...]
>
>> +bool is_state(struct msm_vidc_inst *inst, enum msm_vidc_state state)
>> +{
>> + return inst->state == state;
>> +}
>> +
>> +bool is_sub_state(struct msm_vidc_inst *inst, enum msm_vidc_sub_state sub_state)
>> +{
>> + return (inst->sub_state & sub_state);
>> +}
> Why are there 2 separate funcs for core and inst? Don't we have
> a pointer within one to the other?
>
>
core and instance sub states are maintained differently for ex in SSR, we
need to check the core sub state, if we combine instance and core state
checks, we won't know against which sub state we should check.
> [...]
>

>> +
>> +int msm_vidc_update_state(struct msm_vidc_inst *inst,
>> + enum msm_vidc_state request_state, const char *func)
>> +{
>> + struct msm_vidc_state_handle *state_handle = NULL;
>> + int rc = 0;
> rc is unused
>
> [...]
>
>> +static int msm_vidc_set_sub_state(struct msm_vidc_inst *inst,
>> + enum msm_vidc_sub_state sub_state, const char *func)
>> +{
>> + char sub_state_name[MAX_NAME_LENGTH];
>> + int cnt, rc = 0;
> ditto
>
Thanks for pointing these out, will remove all unused rc.

Thanks,
Dikshita
> Konrad

2023-08-20 16:39:11

by Dikshita Agarwal

[permalink] [raw]

Subject: Re: [PATCH 18/33] iris: vidc: hfi: add Host Firmware Interface (HFI)

On 7/28/2023 9:28 PM, Bryan O'Donoghue wrote:
> On 28/07/2023 14:23, Vikash Garodia wrote:
>> +    rc = hfi_packet_sys_intraframe_powercollapse(core, core->packet,
>> +                             core->packet_size, enable);
>> +    if (rc)
>> +        return rc;
>
> I'm 99.9999999999 % sure this is misnamed.
>
> "Inter" means in-between two things.
> "Intra" means inside of one thing.
>
> So "intraframe" means inside of one frame "interframe" would mean power
> collapsing in-between two frames, which is what I think this does.
>
> And I'd still rather be adding inter-frame power-collapse to as many
> different versions of the existing silicon and new silicon as opposed to
> segregating it off in a new driver.
>
> I'm assuming that more than sm8550 supports it since @ the end of the day
> this is a firmware feature to power-collapse during an active session when
> we aren't busy.
>
You are actually 100% correct here, it is indeed inter frame power collapse,
Will rename this api with hfi_packet_sys_interframe_powercollapse

Thanks,
Dikshita
> ---
> bod