| Message ID | 20201120001037.10032-3-stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Venus encoder improvements | expand |
On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Currently we rely on firmware to return error when we reach the maximum > supported number of sessions. But this errors are happened at reqbuf > time which is a bit later. The more reasonable way looks like is to > return the error on driver open. > > To achieve that modify hfi_session_create to return error when we reach > maximum count of sessions and thus refuse open. > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/qcom/venus/core.h | 1 + > drivers/media/platform/qcom/venus/hfi.c | 19 +++++++++++++++---- > .../media/platform/qcom/venus/hfi_parser.c | 3 +++ > 3 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index db0e6738281e..3a477fcdd3a8 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -96,6 +96,7 @@ struct venus_format { > #define MAX_CAP_ENTRIES 32 > #define MAX_ALLOC_MODE_ENTRIES 16 > #define MAX_CODEC_NUM 32 > +#define MAX_SESSIONS 16 > > struct raw_formats { > u32 buftype; > diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c > index 638ed5cfe05e..8420be6d3991 100644 > --- a/drivers/media/platform/qcom/venus/hfi.c > +++ b/drivers/media/platform/qcom/venus/hfi.c > @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst) > int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) > { > struct venus_core *core = inst->core; > + int ret; > > if (!ops) > return -EINVAL; > @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) > init_completion(&inst->done); > inst->ops = ops; > > - mutex_lock(&core->lock); > - list_add_tail(&inst->list, &core->instances); > - atomic_inc(&core->insts_count); > + ret = mutex_lock_interruptible(&core->lock); > + if (ret) > + return ret; > + > + ret = atomic_read(&core->insts_count); > + if (ret + 1 > core->max_sessions_supported) { > + ret = -EAGAIN; > + } else { > + atomic_inc(&core->insts_count); > + list_add_tail(&inst->list, &core->instances); > + ret = 0; > + } > + > mutex_unlock(&core->lock); > > - return 0; > + return ret; > } > EXPORT_SYMBOL_GPL(hfi_session_create); > > diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c > index 363ee2a65453..52898633a8e6 100644 > --- a/drivers/media/platform/qcom/venus/hfi_parser.c > +++ b/drivers/media/platform/qcom/venus/hfi_parser.c > @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf, > words_count--; > } > My understanding of the hardware is that there is a max number of macroblocks that can be worked on at a time. That works out to nominally 16 clips. But large clips can take more resources. Does |max_sessions_supported| get updated with the amount that system can use? Or is it always a constant? If it changes depending on system load, then couldn't |core->max_sessions_supported| be 0 if all of the resources have been used up? If that is the case then the below check would appear to be incorrect. > + if (!core->max_sessions_supported) > + core->max_sessions_supported = MAX_SESSIONS; > + > parser_fini(inst, codecs, domain); > > return HFI_ERR_NONE; > -- > 2.17.1 >
On 11/21/20 3:14 AM, Fritz Koenig wrote: > On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> Currently we rely on firmware to return error when we reach the maximum >> supported number of sessions. But this errors are happened at reqbuf >> time which is a bit later. The more reasonable way looks like is to >> return the error on driver open. >> >> To achieve that modify hfi_session_create to return error when we reach >> maximum count of sessions and thus refuse open. >> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/core.h | 1 + >> drivers/media/platform/qcom/venus/hfi.c | 19 +++++++++++++++---- >> .../media/platform/qcom/venus/hfi_parser.c | 3 +++ >> 3 files changed, 19 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >> index db0e6738281e..3a477fcdd3a8 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -96,6 +96,7 @@ struct venus_format { >> #define MAX_CAP_ENTRIES 32 >> #define MAX_ALLOC_MODE_ENTRIES 16 >> #define MAX_CODEC_NUM 32 >> +#define MAX_SESSIONS 16 >> >> struct raw_formats { >> u32 buftype; >> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c >> index 638ed5cfe05e..8420be6d3991 100644 >> --- a/drivers/media/platform/qcom/venus/hfi.c >> +++ b/drivers/media/platform/qcom/venus/hfi.c >> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst) >> int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) >> { >> struct venus_core *core = inst->core; >> + int ret; >> >> if (!ops) >> return -EINVAL; >> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) >> init_completion(&inst->done); >> inst->ops = ops; >> >> - mutex_lock(&core->lock); >> - list_add_tail(&inst->list, &core->instances); >> - atomic_inc(&core->insts_count); >> + ret = mutex_lock_interruptible(&core->lock); >> + if (ret) >> + return ret; >> + >> + ret = atomic_read(&core->insts_count); >> + if (ret + 1 > core->max_sessions_supported) { >> + ret = -EAGAIN; >> + } else { >> + atomic_inc(&core->insts_count); >> + list_add_tail(&inst->list, &core->instances); >> + ret = 0; >> + } >> + >> mutex_unlock(&core->lock); >> >> - return 0; >> + return ret; >> } >> EXPORT_SYMBOL_GPL(hfi_session_create); >> >> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c >> index 363ee2a65453..52898633a8e6 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_parser.c >> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c >> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf, >> words_count--; >> } >> > > My understanding of the hardware is that there is a max number of > macroblocks that can be worked on at a time. That works out to > nominally 16 clips. But large clips can take more resources. Does > |max_sessions_supported| get updated with the amount that system can > use? Or is it always a constant? The number of max sessions supported is constant. > > If it changes depending on system load, then couldn't > |core->max_sessions_supported| be 0 if all of the resources have been > used up? If that is the case then the below check would appear to be > incorrect. No, this is not the case. Changing dynamically the number of max sessions depending on session load is possible but it would be complex to implement. For example, think of decoder dynamic resolution change where we don't know in advance the new resolution (session load). > >> + if (!core->max_sessions_supported) >> + core->max_sessions_supported = MAX_SESSIONS; >> + >> parser_fini(inst, codecs, domain); >> >> return HFI_ERR_NONE; >> -- >> 2.17.1 >>
On Sun, Nov 22, 2020 at 6:48 AM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > > > On 11/21/20 3:14 AM, Fritz Koenig wrote: > > On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov > > <stanimir.varbanov@linaro.org> wrote: > >> > >> Currently we rely on firmware to return error when we reach the maximum > >> supported number of sessions. But this errors are happened at reqbuf > >> time which is a bit later. The more reasonable way looks like is to > >> return the error on driver open. > >> > >> To achieve that modify hfi_session_create to return error when we reach > >> maximum count of sessions and thus refuse open. > >> > >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > >> --- > >> drivers/media/platform/qcom/venus/core.h | 1 + > >> drivers/media/platform/qcom/venus/hfi.c | 19 +++++++++++++++---- > >> .../media/platform/qcom/venus/hfi_parser.c | 3 +++ > >> 3 files changed, 19 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > >> index db0e6738281e..3a477fcdd3a8 100644 > >> --- a/drivers/media/platform/qcom/venus/core.h > >> +++ b/drivers/media/platform/qcom/venus/core.h > >> @@ -96,6 +96,7 @@ struct venus_format { > >> #define MAX_CAP_ENTRIES 32 > >> #define MAX_ALLOC_MODE_ENTRIES 16 > >> #define MAX_CODEC_NUM 32 > >> +#define MAX_SESSIONS 16 > >> > >> struct raw_formats { > >> u32 buftype; > >> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c > >> index 638ed5cfe05e..8420be6d3991 100644 > >> --- a/drivers/media/platform/qcom/venus/hfi.c > >> +++ b/drivers/media/platform/qcom/venus/hfi.c > >> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst) > >> int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) > >> { > >> struct venus_core *core = inst->core; > >> + int ret; > >> > >> if (!ops) > >> return -EINVAL; > >> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) > >> init_completion(&inst->done); > >> inst->ops = ops; > >> > >> - mutex_lock(&core->lock); > >> - list_add_tail(&inst->list, &core->instances); > >> - atomic_inc(&core->insts_count); > >> + ret = mutex_lock_interruptible(&core->lock); > >> + if (ret) > >> + return ret; > >> + > >> + ret = atomic_read(&core->insts_count); > >> + if (ret + 1 > core->max_sessions_supported) { > >> + ret = -EAGAIN; > >> + } else { > >> + atomic_inc(&core->insts_count); > >> + list_add_tail(&inst->list, &core->instances); > >> + ret = 0; > >> + } > >> + > >> mutex_unlock(&core->lock); > >> > >> - return 0; > >> + return ret; > >> } > >> EXPORT_SYMBOL_GPL(hfi_session_create); > >> > >> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c > >> index 363ee2a65453..52898633a8e6 100644 > >> --- a/drivers/media/platform/qcom/venus/hfi_parser.c > >> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c > >> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf, > >> words_count--; > >> } > >> > > > > My understanding of the hardware is that there is a max number of > > macroblocks that can be worked on at a time. That works out to > > nominally 16 clips. But large clips can take more resources. Does > > |max_sessions_supported| get updated with the amount that system can > > use? Or is it always a constant? > > The number of max sessions supported is constant. > > > > > If it changes depending on system load, then couldn't > > be 0 if all of the resources have been > > used up? If that is the case then the below check would appear to be > > incorrect. > > No, this is not the case. Changing dynamically the number of max > sessions depending on session load is possible but it would be complex > to implement. For example, think of decoder dynamic resolution change > where we don't know in advance the new resolution (session load). > Sorry, I should have been more specific. The complexity of dynamically changing the max sessions did not seem to be captured in the patch, so I wanted to make sure that |core->max_sessions_supported| was constant. As it is constant, then this patch looks to capture everything. Reviewed-by: Fritz Koenig <frkoenig@chromium.org> > > > >> + if (!core->max_sessions_supported) > >> + core->max_sessions_supported = MAX_SESSIONS; > >> + > >> parser_fini(inst, codecs, domain); > >> > >> return HFI_ERR_NONE; > >> -- > >> 2.17.1 > >> > > -- > regards, > Stan
On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Currently we rely on firmware to return error when we reach the maximum > supported number of sessions. But this errors are happened at reqbuf > time which is a bit later. The more reasonable way looks like is to > return the error on driver open. > > To achieve that modify hfi_session_create to return error when we reach > maximum count of sessions and thus refuse open. > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/qcom/venus/core.h | 1 + > drivers/media/platform/qcom/venus/hfi.c | 19 +++++++++++++++---- > .../media/platform/qcom/venus/hfi_parser.c | 3 +++ > 3 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index db0e6738281e..3a477fcdd3a8 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -96,6 +96,7 @@ struct venus_format { > #define MAX_CAP_ENTRIES 32 > #define MAX_ALLOC_MODE_ENTRIES 16 > #define MAX_CODEC_NUM 32 > +#define MAX_SESSIONS 16 > > struct raw_formats { > u32 buftype; > diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c > index 638ed5cfe05e..8420be6d3991 100644 > --- a/drivers/media/platform/qcom/venus/hfi.c > +++ b/drivers/media/platform/qcom/venus/hfi.c > @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst) > int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) > { > struct venus_core *core = inst->core; > + int ret; > > if (!ops) > return -EINVAL; > @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) > init_completion(&inst->done); > inst->ops = ops; > > - mutex_lock(&core->lock); > - list_add_tail(&inst->list, &core->instances); > - atomic_inc(&core->insts_count); > + ret = mutex_lock_interruptible(&core->lock); > + if (ret) > + return ret; Why do we change to mutex_lock_interruptible() here? This makes this function return an error even though we could obtain the lock just by trying a bit harder. > + > + ret = atomic_read(&core->insts_count); > + if (ret + 1 > core->max_sessions_supported) { > + ret = -EAGAIN; > + } else { > + atomic_inc(&core->insts_count); > + list_add_tail(&inst->list, &core->instances); > + ret = 0; > + } > + > mutex_unlock(&core->lock); > > - return 0; > + return ret; > } > EXPORT_SYMBOL_GPL(hfi_session_create); > > diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c > index 363ee2a65453..52898633a8e6 100644 > --- a/drivers/media/platform/qcom/venus/hfi_parser.c > +++ b/drivers/media/platform/qcom/venus/hfi_parser.c > @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf, > words_count--; > } > > + if (!core->max_sessions_supported) > + core->max_sessions_supported = MAX_SESSIONS; > + > parser_fini(inst, codecs, domain); > > return HFI_ERR_NONE; > -- > 2.17.1 >
On 11/25/20 5:46 AM, Alexandre Courbot wrote: > On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> Currently we rely on firmware to return error when we reach the maximum >> supported number of sessions. But this errors are happened at reqbuf >> time which is a bit later. The more reasonable way looks like is to >> return the error on driver open. >> >> To achieve that modify hfi_session_create to return error when we reach >> maximum count of sessions and thus refuse open. >> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/core.h | 1 + >> drivers/media/platform/qcom/venus/hfi.c | 19 +++++++++++++++---- >> .../media/platform/qcom/venus/hfi_parser.c | 3 +++ >> 3 files changed, 19 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >> index db0e6738281e..3a477fcdd3a8 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -96,6 +96,7 @@ struct venus_format { >> #define MAX_CAP_ENTRIES 32 >> #define MAX_ALLOC_MODE_ENTRIES 16 >> #define MAX_CODEC_NUM 32 >> +#define MAX_SESSIONS 16 >> >> struct raw_formats { >> u32 buftype; >> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c >> index 638ed5cfe05e..8420be6d3991 100644 >> --- a/drivers/media/platform/qcom/venus/hfi.c >> +++ b/drivers/media/platform/qcom/venus/hfi.c >> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst) >> int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) >> { >> struct venus_core *core = inst->core; >> + int ret; >> >> if (!ops) >> return -EINVAL; >> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) >> init_completion(&inst->done); >> inst->ops = ops; >> >> - mutex_lock(&core->lock); >> - list_add_tail(&inst->list, &core->instances); >> - atomic_inc(&core->insts_count); >> + ret = mutex_lock_interruptible(&core->lock); >> + if (ret) >> + return ret; > > Why do we change to mutex_lock_interruptible() here? This makes this Because mutex_lock_interruptible is preferable in kernel docs, but I agree that changing mutex_lock with mutex_lock_interruptible should be subject of another lock related patches. I will drop this in next patch version. > function return an error even though we could obtain the lock just by > trying a bit harder. I didn't get that. The behavior of mutex_lock_interruptible is that same as mutex_lock, i.e. the it will sleep to acquire the lock. The difference is that the sleep could be interrupted by a signal. You might think about mutex_trylock? > >> + >> + ret = atomic_read(&core->insts_count); >> + if (ret + 1 > core->max_sessions_supported) { >> + ret = -EAGAIN; >> + } else { >> + atomic_inc(&core->insts_count); >> + list_add_tail(&inst->list, &core->instances); >> + ret = 0; >> + } >> + >> mutex_unlock(&core->lock); >> >> - return 0; >> + return ret; >> } >> EXPORT_SYMBOL_GPL(hfi_session_create); >> >> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c >> index 363ee2a65453..52898633a8e6 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_parser.c >> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c >> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf, >> words_count--; >> } >> >> + if (!core->max_sessions_supported) >> + core->max_sessions_supported = MAX_SESSIONS; >> + >> parser_fini(inst, codecs, domain); >> >> return HFI_ERR_NONE; >> -- >> 2.17.1 >>
On Wed, Nov 25, 2020 at 10:01 PM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > > > On 11/25/20 5:46 AM, Alexandre Courbot wrote: > > On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov > > <stanimir.varbanov@linaro.org> wrote: > >> > >> Currently we rely on firmware to return error when we reach the maximum > >> supported number of sessions. But this errors are happened at reqbuf > >> time which is a bit later. The more reasonable way looks like is to > >> return the error on driver open. > >> > >> To achieve that modify hfi_session_create to return error when we reach > >> maximum count of sessions and thus refuse open. > >> > >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > >> --- > >> drivers/media/platform/qcom/venus/core.h | 1 + > >> drivers/media/platform/qcom/venus/hfi.c | 19 +++++++++++++++---- > >> .../media/platform/qcom/venus/hfi_parser.c | 3 +++ > >> 3 files changed, 19 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > >> index db0e6738281e..3a477fcdd3a8 100644 > >> --- a/drivers/media/platform/qcom/venus/core.h > >> +++ b/drivers/media/platform/qcom/venus/core.h > >> @@ -96,6 +96,7 @@ struct venus_format { > >> #define MAX_CAP_ENTRIES 32 > >> #define MAX_ALLOC_MODE_ENTRIES 16 > >> #define MAX_CODEC_NUM 32 > >> +#define MAX_SESSIONS 16 > >> > >> struct raw_formats { > >> u32 buftype; > >> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c > >> index 638ed5cfe05e..8420be6d3991 100644 > >> --- a/drivers/media/platform/qcom/venus/hfi.c > >> +++ b/drivers/media/platform/qcom/venus/hfi.c > >> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst) > >> int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) > >> { > >> struct venus_core *core = inst->core; > >> + int ret; > >> > >> if (!ops) > >> return -EINVAL; > >> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) > >> init_completion(&inst->done); > >> inst->ops = ops; > >> > >> - mutex_lock(&core->lock); > >> - list_add_tail(&inst->list, &core->instances); > >> - atomic_inc(&core->insts_count); > >> + ret = mutex_lock_interruptible(&core->lock); > >> + if (ret) > >> + return ret; > > > > Why do we change to mutex_lock_interruptible() here? This makes this > > Because mutex_lock_interruptible is preferable in kernel docs, but I > agree that changing mutex_lock with mutex_lock_interruptible should be > subject of another lock related patches. I will drop this in next patch > version. > > > function return an error even though we could obtain the lock just by > > trying a bit harder. > > I didn't get that. The behavior of mutex_lock_interruptible is that same > as mutex_lock, i.e. the it will sleep to acquire the lock. The > difference is that the sleep could be interrupted by a signal. You might > think about mutex_trylock? Unless that mutex can be held by someone else for a rather long time (i.e. to the point where we may want to give priority to signals when userspace opens the device, since that's where hfi_session_create() is called), I am not convinced this change is necessary? It may confuse userspace into thinking there was a serious error while there is none. Granted, userspace should manage this case, and from what I can see this code is correct, but I'm not sure we would gain anything by adding this extra complexity. > > > > >> + > >> + ret = atomic_read(&core->insts_count); > >> + if (ret + 1 > core->max_sessions_supported) { > >> + ret = -EAGAIN; > >> + } else { > >> + atomic_inc(&core->insts_count); > >> + list_add_tail(&inst->list, &core->instances); > >> + ret = 0; > >> + } > >> + > >> mutex_unlock(&core->lock); > >> > >> - return 0; > >> + return ret; > >> } > >> EXPORT_SYMBOL_GPL(hfi_session_create); > >> > >> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c > >> index 363ee2a65453..52898633a8e6 100644 > >> --- a/drivers/media/platform/qcom/venus/hfi_parser.c > >> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c > >> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf, > >> words_count--; > >> } > >> > >> + if (!core->max_sessions_supported) > >> + core->max_sessions_supported = MAX_SESSIONS; > >> + > >> parser_fini(inst, codecs, domain); > >> > >> return HFI_ERR_NONE; > >> -- > >> 2.17.1 > >> > > -- > regards, > Stan
On 11/26/20 8:28 AM, Alexandre Courbot wrote: > On Wed, Nov 25, 2020 at 10:01 PM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> >> >> On 11/25/20 5:46 AM, Alexandre Courbot wrote: >>> On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov >>> <stanimir.varbanov@linaro.org> wrote: >>>> >>>> Currently we rely on firmware to return error when we reach the maximum >>>> supported number of sessions. But this errors are happened at reqbuf >>>> time which is a bit later. The more reasonable way looks like is to >>>> return the error on driver open. >>>> >>>> To achieve that modify hfi_session_create to return error when we reach >>>> maximum count of sessions and thus refuse open. >>>> >>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >>>> --- >>>> drivers/media/platform/qcom/venus/core.h | 1 + >>>> drivers/media/platform/qcom/venus/hfi.c | 19 +++++++++++++++---- >>>> .../media/platform/qcom/venus/hfi_parser.c | 3 +++ >>>> 3 files changed, 19 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >>>> index db0e6738281e..3a477fcdd3a8 100644 >>>> --- a/drivers/media/platform/qcom/venus/core.h >>>> +++ b/drivers/media/platform/qcom/venus/core.h >>>> @@ -96,6 +96,7 @@ struct venus_format { >>>> #define MAX_CAP_ENTRIES 32 >>>> #define MAX_ALLOC_MODE_ENTRIES 16 >>>> #define MAX_CODEC_NUM 32 >>>> +#define MAX_SESSIONS 16 >>>> >>>> struct raw_formats { >>>> u32 buftype; >>>> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c >>>> index 638ed5cfe05e..8420be6d3991 100644 >>>> --- a/drivers/media/platform/qcom/venus/hfi.c >>>> +++ b/drivers/media/platform/qcom/venus/hfi.c >>>> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst) >>>> int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) >>>> { >>>> struct venus_core *core = inst->core; >>>> + int ret; >>>> >>>> if (!ops) >>>> return -EINVAL; >>>> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) >>>> init_completion(&inst->done); >>>> inst->ops = ops; >>>> >>>> - mutex_lock(&core->lock); >>>> - list_add_tail(&inst->list, &core->instances); >>>> - atomic_inc(&core->insts_count); >>>> + ret = mutex_lock_interruptible(&core->lock); >>>> + if (ret) >>>> + return ret; >>> >>> Why do we change to mutex_lock_interruptible() here? This makes this >> >> Because mutex_lock_interruptible is preferable in kernel docs, but I >> agree that changing mutex_lock with mutex_lock_interruptible should be >> subject of another lock related patches. I will drop this in next patch >> version. >> >>> function return an error even though we could obtain the lock just by >>> trying a bit harder. >> >> I didn't get that. The behavior of mutex_lock_interruptible is that same >> as mutex_lock, i.e. the it will sleep to acquire the lock. The >> difference is that the sleep could be interrupted by a signal. You might >> think about mutex_trylock? > > Unless that mutex can be held by someone else for a rather long time > (i.e. to the point where we may want to give priority to signals when > userspace opens the device, since that's where hfi_session_create() is > called), I am not convinced this change is necessary? It may confuse Exactly, if there is a case where the core->lock is taken (firmware recovery) and it is not unlocked for very long time (deadlock?) then client process cannot be interrupted with a signal. > userspace into thinking there was a serious error while there is none. The client should be able to handle EINTR, right? > Granted, userspace should manage this case, and from what I can see > this code is correct, but I'm not sure we would gain anything by > adding this extra complexity. The benefit is that if something wrong is happening in the driver the client process will be killable. > >> >>> >>>> + >>>> + ret = atomic_read(&core->insts_count); >>>> + if (ret + 1 > core->max_sessions_supported) { >>>> + ret = -EAGAIN; >>>> + } else { >>>> + atomic_inc(&core->insts_count); >>>> + list_add_tail(&inst->list, &core->instances); >>>> + ret = 0; >>>> + } >>>> + >>>> mutex_unlock(&core->lock); >>>> >>>> - return 0; >>>> + return ret; >>>> } >>>> EXPORT_SYMBOL_GPL(hfi_session_create); >>>> >>>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c >>>> index 363ee2a65453..52898633a8e6 100644 >>>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c >>>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c >>>> @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf, >>>> words_count--; >>>> } >>>> >>>> + if (!core->max_sessions_supported) >>>> + core->max_sessions_supported = MAX_SESSIONS; >>>> + >>>> parser_fini(inst, codecs, domain); >>>> >>>> return HFI_ERR_NONE; >>>> -- >>>> 2.17.1 >>>> >> >> -- >> regards, >> Stan
On Fri, Nov 27, 2020 at 7:42 AM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > > > On 11/26/20 8:28 AM, Alexandre Courbot wrote: > > On Wed, Nov 25, 2020 at 10:01 PM Stanimir Varbanov > > <stanimir.varbanov@linaro.org> wrote: > >> > >> > >> > >> On 11/25/20 5:46 AM, Alexandre Courbot wrote: > >>> On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov > >>> <stanimir.varbanov@linaro.org> wrote: > >>>> > >>>> Currently we rely on firmware to return error when we reach the maximum > >>>> supported number of sessions. But this errors are happened at reqbuf > >>>> time which is a bit later. The more reasonable way looks like is to > >>>> return the error on driver open. > >>>> > >>>> To achieve that modify hfi_session_create to return error when we reach > >>>> maximum count of sessions and thus refuse open. > >>>> > >>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > >>>> --- > >>>> drivers/media/platform/qcom/venus/core.h | 1 + > >>>> drivers/media/platform/qcom/venus/hfi.c | 19 +++++++++++++++---- > >>>> .../media/platform/qcom/venus/hfi_parser.c | 3 +++ > >>>> 3 files changed, 19 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > >>>> index db0e6738281e..3a477fcdd3a8 100644 > >>>> --- a/drivers/media/platform/qcom/venus/core.h > >>>> +++ b/drivers/media/platform/qcom/venus/core.h > >>>> @@ -96,6 +96,7 @@ struct venus_format { > >>>> #define MAX_CAP_ENTRIES 32 > >>>> #define MAX_ALLOC_MODE_ENTRIES 16 > >>>> #define MAX_CODEC_NUM 32 > >>>> +#define MAX_SESSIONS 16 > >>>> > >>>> struct raw_formats { > >>>> u32 buftype; > >>>> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c > >>>> index 638ed5cfe05e..8420be6d3991 100644 > >>>> --- a/drivers/media/platform/qcom/venus/hfi.c > >>>> +++ b/drivers/media/platform/qcom/venus/hfi.c > >>>> @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst) > >>>> int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) > >>>> { > >>>> struct venus_core *core = inst->core; > >>>> + int ret; > >>>> > >>>> if (!ops) > >>>> return -EINVAL; > >>>> @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) > >>>> init_completion(&inst->done); > >>>> inst->ops = ops; > >>>> > >>>> - mutex_lock(&core->lock); > >>>> - list_add_tail(&inst->list, &core->instances); > >>>> - atomic_inc(&core->insts_count); > >>>> + ret = mutex_lock_interruptible(&core->lock); > >>>> + if (ret) > >>>> + return ret; > >>> > >>> Why do we change to mutex_lock_interruptible() here? This makes this > >> > >> Because mutex_lock_interruptible is preferable in kernel docs, but I > >> agree that changing mutex_lock with mutex_lock_interruptible should be > >> subject of another lock related patches. I will drop this in next patch > >> version. > >> > >>> function return an error even though we could obtain the lock just by > >>> trying a bit harder. > >> > >> I didn't get that. The behavior of mutex_lock_interruptible is that same > >> as mutex_lock, i.e. the it will sleep to acquire the lock. The > >> difference is that the sleep could be interrupted by a signal. You might > >> think about mutex_trylock? > > > > Unless that mutex can be held by someone else for a rather long time > > (i.e. to the point where we may want to give priority to signals when > > userspace opens the device, since that's where hfi_session_create() is > > called), I am not convinced this change is necessary? It may confuse > > Exactly, if there is a case where the core->lock is taken (firmware > recovery) and it is not unlocked for very long time (deadlock?) then > client process cannot be interrupted with a signal. > > > userspace into thinking there was a serious error while there is none. > > The client should be able to handle EINTR, right? > > > Granted, userspace should manage this case, and from what I can see > > this code is correct, but I'm not sure we would gain anything by > > adding this extra complexity. > > The benefit is that if something wrong is happening in the driver the > client process will be killable. Ack, that definitely makes sense in that context, even though it should probably be done separately from this patch series. :) Cheers, Alex.
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h index db0e6738281e..3a477fcdd3a8 100644 --- a/drivers/media/platform/qcom/venus/core.h +++ b/drivers/media/platform/qcom/venus/core.h @@ -96,6 +96,7 @@ struct venus_format { #define MAX_CAP_ENTRIES 32 #define MAX_ALLOC_MODE_ENTRIES 16 #define MAX_CODEC_NUM 32 +#define MAX_SESSIONS 16 struct raw_formats { u32 buftype; diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c index 638ed5cfe05e..8420be6d3991 100644 --- a/drivers/media/platform/qcom/venus/hfi.c +++ b/drivers/media/platform/qcom/venus/hfi.c @@ -175,6 +175,7 @@ static int wait_session_msg(struct venus_inst *inst) int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) { struct venus_core *core = inst->core; + int ret; if (!ops) return -EINVAL; @@ -183,12 +184,22 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops) init_completion(&inst->done); inst->ops = ops; - mutex_lock(&core->lock); - list_add_tail(&inst->list, &core->instances); - atomic_inc(&core->insts_count); + ret = mutex_lock_interruptible(&core->lock); + if (ret) + return ret; + + ret = atomic_read(&core->insts_count); + if (ret + 1 > core->max_sessions_supported) { + ret = -EAGAIN; + } else { + atomic_inc(&core->insts_count); + list_add_tail(&inst->list, &core->instances); + ret = 0; + } + mutex_unlock(&core->lock); - return 0; + return ret; } EXPORT_SYMBOL_GPL(hfi_session_create); diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c index 363ee2a65453..52898633a8e6 100644 --- a/drivers/media/platform/qcom/venus/hfi_parser.c +++ b/drivers/media/platform/qcom/venus/hfi_parser.c @@ -276,6 +276,9 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf, words_count--; } + if (!core->max_sessions_supported) + core->max_sessions_supported = MAX_SESSIONS; + parser_fini(inst, codecs, domain); return HFI_ERR_NONE;
Currently we rely on firmware to return error when we reach the maximum supported number of sessions. But this errors are happened at reqbuf time which is a bit later. The more reasonable way looks like is to return the error on driver open. To achieve that modify hfi_session_create to return error when we reach maximum count of sessions and thus refuse open. Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> --- drivers/media/platform/qcom/venus/core.h | 1 + drivers/media/platform/qcom/venus/hfi.c | 19 +++++++++++++++---- .../media/platform/qcom/venus/hfi_parser.c | 3 +++ 3 files changed, 19 insertions(+), 4 deletions(-)