| Message ID | 20201120001037.10032-3-stanimir.varbanov@linaro.org |
|---|---|
| State | Superseded |
| 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 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 -- 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(-)