Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

2020-11-26 Thread Alexandre Courbot 
On Fri, Nov 27, 2020 at 7:42 AM Stanimir Varbanov
 wrote:
>
>
>
> On 11/26/20 8:28 AM, Alexandre Courbot wrote:
> > On Wed, Nov 25, 2020 at 10:01 PM Stanimir Varbanov
> >  wrote:
> >>
> >>
> >>
> >> On 11/25/20 5:46 AM, Alexandre Courbot wrote:
> >>> On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
> >>>  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 
>  ---
>   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_ENTRIES32
>   #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(>done);
>  inst->ops = ops;
> 
>  -   mutex_lock(>lock);
>  -   list_add_tail(>list, >instances);
>  -   atomic_inc(>insts_count);
>  +   ret = mutex_lock_interruptible(>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.

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

2020-11-26 Thread Stanimir Varbanov 



On 11/26/20 8:28 AM, Alexandre Courbot wrote:
> On Wed, Nov 25, 2020 at 10:01 PM Stanimir Varbanov
>  wrote:
>>
>>
>>
>> On 11/25/20 5:46 AM, Alexandre Courbot wrote:
>>> On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
>>>  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 
 ---
  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_ENTRIES32
  #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(>done);
 inst->ops = ops;

 -   mutex_lock(>lock);
 -   list_add_tail(>list, >instances);
 -   atomic_inc(>insts_count);
 +   ret = mutex_lock_interruptible(>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(>insts_count);
 +   if (ret + 1 > core->max_sessions_supported) {
 +   ret = -EAGAIN;
 +   } else {
 +   atomic_inc(>insts_count);
 +   list_add_tail(>list, >instances);
 +   ret = 0;
 +   }
 +
 mutex_unlock(>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)
 +

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

2020-11-25 Thread Alexandre Courbot 
On Wed, Nov 25, 2020 at 10:01 PM Stanimir Varbanov
 wrote:
>
>
>
> On 11/25/20 5:46 AM, Alexandre Courbot wrote:
> > On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
> >  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 
> >> ---
> >>  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_ENTRIES32
> >>  #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(>done);
> >> inst->ops = ops;
> >>
> >> -   mutex_lock(>lock);
> >> -   list_add_tail(>list, >instances);
> >> -   atomic_inc(>insts_count);
> >> +   ret = mutex_lock_interruptible(>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(>insts_count);
> >> +   if (ret + 1 > core->max_sessions_supported) {
> >> +   ret = -EAGAIN;
> >> +   } else {
> >> +   atomic_inc(>insts_count);
> >> +   list_add_tail(>list, >instances);
> >> +   ret = 0;
> >> +   }
> >> +
> >> mutex_unlock(>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

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

2020-11-25 Thread Stanimir Varbanov 



On 11/25/20 5:46 AM, Alexandre Courbot wrote:
> On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
>  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 
>> ---
>>  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_ENTRIES32
>>  #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(>done);
>> inst->ops = ops;
>>
>> -   mutex_lock(>lock);
>> -   list_add_tail(>list, >instances);
>> -   atomic_inc(>insts_count);
>> +   ret = mutex_lock_interruptible(>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(>insts_count);
>> +   if (ret + 1 > core->max_sessions_supported) {
>> +   ret = -EAGAIN;
>> +   } else {
>> +   atomic_inc(>insts_count);
>> +   list_add_tail(>list, >instances);
>> +   ret = 0;
>> +   }
>> +
>> mutex_unlock(>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

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

2020-11-24 Thread Alexandre Courbot 
On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov
 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 
> ---
>  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_ENTRIES32
>  #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(>done);
> inst->ops = ops;
>
> -   mutex_lock(>lock);
> -   list_add_tail(>list, >instances);
> -   atomic_inc(>insts_count);
> +   ret = mutex_lock_interruptible(>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(>insts_count);
> +   if (ret + 1 > core->max_sessions_supported) {
> +   ret = -EAGAIN;
> +   } else {
> +   atomic_inc(>insts_count);
> +   list_add_tail(>list, >instances);
> +   ret = 0;
> +   }
> +
> mutex_unlock(>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
>

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

2020-11-22 Thread Fritz Koenig 
On Sun, Nov 22, 2020 at 6:48 AM Stanimir Varbanov
 wrote:
>
>
>
> On 11/21/20 3:14 AM, Fritz Koenig wrote:
> > On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov
> >  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 
> >> ---
> >>  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_ENTRIES32
> >>  #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(>done);
> >> inst->ops = ops;
> >>
> >> -   mutex_lock(>lock);
> >> -   list_add_tail(>list, >instances);
> >> -   atomic_inc(>insts_count);
> >> +   ret = mutex_lock_interruptible(>lock);
> >> +   if (ret)
> >> +   return ret;
> >> +
> >> +   ret = atomic_read(>insts_count);
> >> +   if (ret + 1 > core->max_sessions_supported) {
> >> +   ret = -EAGAIN;
> >> +   } else {
> >> +   atomic_inc(>insts_count);
> >> +   list_add_tail(>list, >instances);
> >> +   ret = 0;
> >> +   }
> >> +
> >> mutex_unlock(>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 
> >
> >> +   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

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

2020-11-22 Thread Stanimir Varbanov 



On 11/21/20 3:14 AM, Fritz Koenig wrote:
> On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov
>  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 
>> ---
>>  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_ENTRIES32
>>  #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(>done);
>> inst->ops = ops;
>>
>> -   mutex_lock(>lock);
>> -   list_add_tail(>list, >instances);
>> -   atomic_inc(>insts_count);
>> +   ret = mutex_lock_interruptible(>lock);
>> +   if (ret)
>> +   return ret;
>> +
>> +   ret = atomic_read(>insts_count);
>> +   if (ret + 1 > core->max_sessions_supported) {
>> +   ret = -EAGAIN;
>> +   } else {
>> +   atomic_inc(>insts_count);
>> +   list_add_tail(>list, >instances);
>> +   ret = 0;
>> +   }
>> +
>> mutex_unlock(>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
>>

-- 
regards,
Stan

Re: [PATCH 2/3] venus: Limit HFI sessions to the maximum supported

2020-11-20 Thread Fritz Koenig 
On Thu, Nov 19, 2020 at 4:12 PM Stanimir Varbanov
 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 
> ---
>  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_ENTRIES32
>  #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(>done);
> inst->ops = ops;
>
> -   mutex_lock(>lock);
> -   list_add_tail(>list, >instances);
> -   atomic_inc(>insts_count);
> +   ret = mutex_lock_interruptible(>lock);
> +   if (ret)
> +   return ret;
> +
> +   ret = atomic_read(>insts_count);
> +   if (ret + 1 > core->max_sessions_supported) {
> +   ret = -EAGAIN;
> +   } else {
> +   atomic_inc(>insts_count);
> +   list_add_tail(>list, >instances);
> +   ret = 0;
> +   }
> +
> mutex_unlock(>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
>

[PATCH 2/3] venus: Limit HFI sessions to the maximum supported

2020-11-19 Thread Stanimir Varbanov 
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 
---
 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_ENTRIES32
 #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(>done);
inst->ops = ops;
 
-   mutex_lock(>lock);
-   list_add_tail(>list, >instances);
-   atomic_inc(>insts_count);
+   ret = mutex_lock_interruptible(>lock);
+   if (ret)
+   return ret;
+
+   ret = atomic_read(>insts_count);
+   if (ret + 1 > core->max_sessions_supported) {
+   ret = -EAGAIN;
+   } else {
+   atomic_inc(>insts_count);
+   list_add_tail(>list, >instances);
+   ret = 0;
+   }
+
mutex_unlock(>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