linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stanimir Varbanov <stanimir.varbanov@linaro.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Linux Media Mailing List <linux-media@vger.kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Vikash Garodia <vgarodia@codeaurora.org>,
	Mansur Alisha Shaik <mansur@codeaurora.org>
Subject: Re: [PATCH 1/5] venus: venc: Use pmruntime autosuspend
Date: Mon, 31 May 2021 14:43:52 +0300	[thread overview]
Message-ID: <ad5b75e2-89e9-8ed3-c092-3ccf924f2178@linaro.org> (raw)
In-Reply-To: <CAD=FV=WJZ3=YMcqFLeoXnYh6=fPDzAe_4yAxehSafLGOC31ogQ@mail.gmail.com>

Hi Doug,

Thanks for the comments!

On 5/19/21 7:51 PM, Doug Anderson wrote:
> Hi,
> 
> On Tue, May 18, 2021 at 8:46 AM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> Migrate encoder to use pm-runtime autosuspend APIs.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/venc.c | 104 +++++++++++++++++++++--
>>  1 file changed, 96 insertions(+), 8 deletions(-)
> 
> Not a full review but I happened to skim by this patch and it caught
> my attention...
> 

Thanks!

> 
>> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
>> index 4a7291f934b6..a7a858f03ba3 100644
>> --- a/drivers/media/platform/qcom/venus/venc.c
>> +++ b/drivers/media/platform/qcom/venus/venc.c
>> @@ -536,6 +536,64 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = {
>>         .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
>>  };
>>
>> +static int venc_pm_get(struct venus_inst *inst)
>> +{
>> +       struct venus_core *core = inst->core;
>> +       struct device *dev = core->dev_enc;
>> +       int ret;
>> +
>> +       mutex_lock(&core->pm_lock);
>> +       ret = pm_runtime_get_sync(dev);
>> +       mutex_unlock(&core->pm_lock);
> 
> Why do you need a mutex around this?


> 
>> +
>> +       return ret < 0 ? ret : 0;
> 
> Odd but true: if pm_runtime_get_sync() returns an error you still need
> to put. If your code below isn't going to do this then you should
> handle it here?

I have v2 where I use pm_runtime_resume_and_get().

> 
>> +}
>> +
>> +static int venc_pm_put(struct venus_inst *inst, bool autosuspend)
>> +{
>> +       struct venus_core *core = inst->core;
>> +       struct device *dev = core->dev_enc;
>> +       int ret;
>> +
>> +       mutex_lock(&core->pm_lock);
>> +
>> +       if (autosuspend)
>> +               ret = pm_runtime_put_autosuspend(dev);
>> +       else
>> +               ret = pm_runtime_put_sync(dev);
>> +
>> +       mutex_unlock(&core->pm_lock);
>> +
>> +       return ret < 0 ? ret : 0;
>> +}
>> +
>> +static int venc_pm_get_put(struct venus_inst *inst)
>> +{
>> +       struct venus_core *core = inst->core;
>> +       struct device *dev = core->dev_enc;
>> +       int ret = 0;
>> +
>> +       mutex_lock(&core->pm_lock);
>> +
>> +       if (pm_runtime_suspended(dev)) {
>> +               ret = pm_runtime_get_sync(dev);
>> +               if (ret < 0)
>> +                       goto error;
> 
> If pm_runtime_get_sync() returns an error you still need to put.

In v2 I replaced with pm_runtime_resume_and_get().

> 
> 
>> +
>> +               ret = pm_runtime_put_autosuspend(dev);
>> +       }
>> +
>> +error:
>> +       mutex_unlock(&core->pm_lock);
>> +
>> +       return ret < 0 ? ret : 0;
>> +}
> 
> What is the purpose of "get_put"? It feels like using it would be racy to me.

See below ...

> 
> 
>> +
>> +static void venc_pm_touch(struct venus_inst *inst)
>> +{
>> +       pm_runtime_mark_last_busy(inst->core->dev_enc);
>> +}
>> +
>>  static int venc_set_properties(struct venus_inst *inst)
>>  {
>>         struct venc_controls *ctr = &inst->controls.enc;
>> @@ -891,10 +949,18 @@ static int venc_queue_setup(struct vb2_queue *q,
>>                 return 0;
>>         }
>>
>> +       ret = venc_pm_get(inst);
>> +       if (ret)
>> +               return ret;
>> +
>>         mutex_lock(&inst->lock);
>>         ret = venc_init_session(inst);
>>         mutex_unlock(&inst->lock);
>>
>> +       if (ret)
>> +               goto put_power;
>> +
>> +       ret = venc_pm_put(inst, false);
>>         if (ret)
>>                 return ret;
>>
>> @@ -930,6 +996,9 @@ static int venc_queue_setup(struct vb2_queue *q,
>>                 break;
>>         }
>>
>> +       return ret;
>> +put_power:
>> +       venc_pm_put(inst, false);
>>         return ret;
>>  }
>>
>> @@ -946,6 +1015,8 @@ static void venc_release_session(struct venus_inst *inst)
>>  {
>>         int ret;
>>
>> +       venc_pm_get(inst);
>> +
>>         mutex_lock(&inst->lock);
>>
>>         ret = hfi_session_deinit(inst);
>> @@ -957,6 +1028,8 @@ static void venc_release_session(struct venus_inst *inst)
>>         venus_pm_load_scale(inst);
>>         INIT_LIST_HEAD(&inst->registeredbufs);
>>         venus_pm_release_core(inst);
>> +
>> +       venc_pm_put(inst, false);
>>  }
>>
>>  static void venc_buf_cleanup(struct vb2_buffer *vb)
>> @@ -1026,7 +1099,15 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
>>         inst->sequence_cap = 0;
>>         inst->sequence_out = 0;
>>
>> +       ret = venc_pm_get(inst);
>> +       if (ret)
>> +               goto error;
>> +
>>         ret = venus_pm_acquire_core(inst);
>> +       if (ret)
>> +               goto put_power;
>> +
>> +       ret = venc_pm_put(inst, true);
>>         if (ret)
>>                 goto error;
>>
>> @@ -1051,6 +1132,8 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
>>
>>         return 0;
>>
>> +put_power:
>> +       venc_pm_put(inst, false);
>>  error:
>>         venus_helper_buffers_done(inst, q->type, VB2_BUF_STATE_QUEUED);
>>         if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>> @@ -1065,6 +1148,8 @@ static void venc_vb2_buf_queue(struct vb2_buffer *vb)
>>  {
>>         struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
>>
>> +       venc_pm_get_put(inst);
>> +
> 
> I don't know this code at all, but I don't understand the point of the
> "get_put". Couldn't the task running this code get scheduled out for 2
> seconds right after your call to venc_pm_get_put() and then it would
> be just like you didn't call it?
> 
> ...or maybe the device wasn't suspended but it was 10 us away from
> being suspended so your "get_put" decided it didn't need to do
> anything. Then you get scheduled out for 10 us and it powers off.
> 
> Maybe there's a good reason for get_put() to exist and a good reason
> why it's race-free but it feels like the kind of thing that needs a
> comment.
> 
> 

This technique was used in decoder for some time now without any issues,
so I guess it is fine in respect to races.

The idea of venc|vdec_pm_get_put was to resume pmruntime in case that 2s
elapsed and the driver does not receive any buffer (through
vb2_buf_queue()) during that time period. In this case (and there is no
other activity) the power and clocks will be turned off and we have to
power them up on next vb2_buf_queue.

>>         mutex_lock(&inst->lock);
>>         venus_helper_vb2_buf_queue(vb);
>>         mutex_unlock(&inst->lock);
>> @@ -1088,6 +1173,8 @@ static void venc_buf_done(struct venus_inst *inst, unsigned int buf_type,
>>         struct vb2_buffer *vb;
>>         unsigned int type;
>>
>> +       venc_pm_touch(inst);
>> +
>>         if (buf_type == HFI_BUFFER_INPUT)
>>                 type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
>>         else
>> @@ -1117,6 +1204,8 @@ static void venc_event_notify(struct venus_inst *inst, u32 event,
>>  {
>>         struct device *dev = inst->core->dev_enc;
>>
>> +       venc_pm_touch(inst);
>> +
>>         if (event == EVT_SESSION_ERROR) {
>>                 inst->session_error = true;
>>                 dev_err(dev, "enc: event session error %x\n", inst->error);
>> @@ -1205,13 +1294,9 @@ static int venc_open(struct file *file)
>>
>>         venus_helper_init_instance(inst);
>>
>> -       ret = pm_runtime_get_sync(core->dev_enc);
>> -       if (ret < 0)
>> -               goto err_put_sync;
>> -
>>         ret = venc_ctrl_init(inst);
>>         if (ret)
>> -               goto err_put_sync;
>> +               goto err_free;
>>
>>         ret = hfi_session_create(inst, &venc_hfi_ops);
>>         if (ret)
>> @@ -1250,8 +1335,7 @@ static int venc_open(struct file *file)
>>         hfi_session_destroy(inst);
>>  err_ctrl_deinit:
>>         venc_ctrl_deinit(inst);
>> -err_put_sync:
>> -       pm_runtime_put_sync(core->dev_enc);
>> +err_free:
>>         kfree(inst);
>>         return ret;
>>  }
>> @@ -1260,6 +1344,8 @@ static int venc_close(struct file *file)
>>  {
>>         struct venus_inst *inst = to_inst(file);
>>
>> +       venc_pm_get(inst);
>> +
>>         v4l2_m2m_ctx_release(inst->m2m_ctx);
>>         v4l2_m2m_release(inst->m2m_dev);
>>         venc_ctrl_deinit(inst);
>> @@ -1268,7 +1354,7 @@ static int venc_close(struct file *file)
>>         v4l2_fh_del(&inst->fh);
>>         v4l2_fh_exit(&inst->fh);
>>
>> -       pm_runtime_put_sync(inst->core->dev_enc);
>> +       venc_pm_put(inst, false);
>>
>>         kfree(inst);
>>         return 0;
>> @@ -1325,6 +1411,8 @@ static int venc_probe(struct platform_device *pdev)
>>         core->dev_enc = dev;
>>
>>         video_set_drvdata(vdev, core);
>> +       pm_runtime_set_autosuspend_delay(dev, 2000);
>> +       pm_runtime_use_autosuspend(dev);
>>         pm_runtime_enable(dev);
> 
> You have the same bug that I just made in another module! :-P
> Specifically, the pm_runtime docs say:
> 
>> Drivers in ->remove() callback should undo the runtime PM changes done
>> in ->probe(). Usually this means calling pm_runtime_disable(),
>> pm_runtime_dont_use_autosuspend() etc.
> 

Sure I will correct this in v2. Thanks!

> -Doug
> 

-- 
regards,
Stan

  reply	other threads:[~2021-05-31 11:43 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-18 15:45 [PATCH 0/5] Venus fatal error handling Stanimir Varbanov
2021-05-18 15:45 ` [PATCH 1/5] venus: venc: Use pmruntime autosuspend Stanimir Varbanov
2021-05-19 16:51   ` Doug Anderson
2021-05-31 11:43     ` Stanimir Varbanov [this message]
2021-05-18 15:45 ` [PATCH 2/5] venus: Make sys_error flag an atomic bitops Stanimir Varbanov
2021-05-18 15:45 ` [PATCH 3/5] venus: hfi: Check for sys error on session hfi functions Stanimir Varbanov
2021-05-18 15:45 ` [PATCH 4/5] venus: helpers: Add helper to mark fatal vb2 error Stanimir Varbanov
2021-05-18 15:45 ` [PATCH 5/5] venus: Handle fatal errors during encoding and decoding Stanimir Varbanov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ad5b75e2-89e9-8ed3-c092-3ccf924f2178@linaro.org \
    --to=stanimir.varbanov@linaro.org \
    --cc=dianders@chromium.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mansur@codeaurora.org \
    --cc=vgarodia@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).