Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0d77682a-74ff-cdf6-bd96-dbe10f2b5e71@quicinc.com>
Date:   Tue, 15 Aug 2023 00:47:38 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 14/33] iris: vidc: add helpers for state management
Content-Language: en-US
To:     Konrad Dybcio <konrad.dybcio@linaro.org>,
        Vikash Garodia <quic_vgarodia@quicinc.com>,
        <stanimir.k.varbanov@gmail.com>, <agross@kernel.org>,
        <andersson@kernel.org>, <mchehab@kernel.org>,
        <hans.verkuil@cisco.com>, <linux-kernel@vger.kernel.org>,
        <linux-media@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>
References: <1690550624-14642-1-git-send-email-quic_vgarodia@quicinc.com>
 <1690550624-14642-15-git-send-email-quic_vgarodia@quicinc.com>
 <60271d41-7807-0808-34d0-684ab9e81a90@linaro.org>
From:   Dikshita Agarwal <quic_dikshita@quicinc.com>
In-Reply-To: <60271d41-7807-0808-34d0-684ab9e81a90@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


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 <quic_dikshita@quicinc.com>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
> [...]
> 
>> +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