From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-17.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9D81EC64E75 for ; Thu, 26 Nov 2020 23:51:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 502312145D for ; Thu, 26 Nov 2020 23:51:19 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="BvFjIsHV" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388408AbgKZXvH (ORCPT ); Thu, 26 Nov 2020 18:51:07 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39686 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388138AbgKZXvD (ORCPT ); Thu, 26 Nov 2020 18:51:03 -0500 Received: from mail-ed1-x544.google.com (mail-ed1-x544.google.com [IPv6:2a00:1450:4864:20::544]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0B7E3C061A04 for ; Thu, 26 Nov 2020 15:51:01 -0800 (PST) Received: by mail-ed1-x544.google.com with SMTP id l5so3895280edq.11 for ; Thu, 26 Nov 2020 15:51:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=V5KYSx6aCS9NMZ6LBkhsZnID6zJfBDD/ql119aALBaU=; b=BvFjIsHVznSKm0SCyX7mIYruIwzynFNq6nNjPldATYnR/9Kn4OLnH1J6oZe8DndYR8 qaZNte+m8uCJZSY0nFOFdcshKL6nsXVP71dxpFJKptdOGZ6saM+ujUu9dRLLJdmhclUL mMAIYoZXdVTod+NpaYIQd0N82DsifDhARArDWSSxexpzD4K0UO6xINti8XffHGlJMagI Di7/DyNAj0WIS5A+kqL1QExtFA4lhuYlGdouuIDmcTbhSQO9iE2xlvDJnt5qWOt9X9YA JAp0VeyqBvB+SP2QGYuZhyfXztlAIzkhA/AonRyq86JTdvL/unX2h0Z3ff3cvDBGXFZ6 09kA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=V5KYSx6aCS9NMZ6LBkhsZnID6zJfBDD/ql119aALBaU=; b=Q+EXm7px1lrfJLZeBXVu+qFtNO3ZWDLbykPxGIqUqbyJC4B+oKO2wQpegxoGXFnxKw x3myARTA6CkPgJkzqOviXVvjzsub6IeVzB5gPSmDSkz1HKcmxFsitOMkbXiUnA6kZ7/7 fyINVKEDjDqcV8Qr3U24JWhbfouxffz0h/1NGFT+3J3VNa3VxYiGBiEYr3jBWmDLqG1i MCGGE2Q6WmLnKqcRevQWMvVc8SR+kZCnCzSDGFTiiHPHdEAadfq7rKq5uvCizaTq8T1F sy0D9qKBXYmj9UzkasO5jA3QG7bu1BPW7d9LyXw/GXsgXrh5szJddGJea8EMvsxniIHH F2TA== X-Gm-Message-State: AOAM532rgmTwSmtheaGvyuosqqCrvUguBlQ4mZrjcukB3chWfzx2GQAK a+l82DDhufBE+/XowFAfisQHrw== X-Google-Smtp-Source: ABdhPJwQHLhhw/lbWnHZhXTchXr4mWbGlEEXdb85zO5MBxYo1OPRGUb20Zv9zTqCeaUqb2a7HH6svA== X-Received: by 2002:a50:d784:: with SMTP id w4mr4718044edi.201.1606434660535; Thu, 26 Nov 2020 15:51:00 -0800 (PST) Received: from [192.168.0.3] (hst-221-3.medicom.bg. [84.238.221.3]) by smtp.googlemail.com with ESMTPSA id 91sm1638444edy.45.2020.11.26.15.50.59 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 26 Nov 2020 15:50:59 -0800 (PST) Subject: Re: [PATCH 1/3] venus: venc: Init the session only once in queue_setup To: Alexandre Courbot Cc: linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, Vikash Garodia , Mansur Alisha Shaik , Dikshita Agarwal References: <20201120001037.10032-1-stanimir.varbanov@linaro.org> <20201120001037.10032-2-stanimir.varbanov@linaro.org> From: Stanimir Varbanov Message-ID: Date: Fri, 27 Nov 2020 01:50:58 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On 11/25/20 5:13 AM, Alexandre Courbot wrote: > Hi Stan, > > On Fri, Nov 20, 2020 at 9:12 AM Stanimir Varbanov > wrote: >> >> Init the hfi session only once in queue_setup and also cover that >> with inst->lock. >> >> Signed-off-by: Stanimir Varbanov >> --- >> drivers/media/platform/qcom/venus/venc.c | 98 ++++++++++++++++++------ >> 1 file changed, 73 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c >> index 4ecf78e30b59..3a2e449663d8 100644 >> --- a/drivers/media/platform/qcom/venus/venc.c >> +++ b/drivers/media/platform/qcom/venus/venc.c >> @@ -725,8 +725,10 @@ static int venc_init_session(struct venus_inst *inst) >> int ret; >> >> ret = hfi_session_init(inst, inst->fmt_cap->pixfmt); >> - if (ret) >> - return ret; >> + if (ret == -EINVAL) >> + return 0; > > Why is it safe to ignore EINVAL here? The confusion comes from hfi_session_init() return values. Presently hfi_session_init will return EINVAL when the session is already init. Maybe EINVAL is not fitting well with the expected behavior of the function. I thought about EALREADY, EBUSY but it doesn't fit well to me too. > >> + else if (ret) >> + goto deinit; >> >> ret = venus_helper_set_input_resolution(inst, inst->width, >> inst->height); >> @@ -762,17 +764,13 @@ static int venc_out_num_buffers(struct venus_inst *inst, unsigned int *num) >> struct hfi_buffer_requirements bufreq; >> int ret; >> >> - ret = venc_init_session(inst); >> + ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq); >> if (ret) >> return ret; >> >> - ret = venus_helper_get_bufreq(inst, HFI_BUFFER_INPUT, &bufreq); >> - >> *num = bufreq.count_actual; >> >> - hfi_session_deinit(inst); >> - >> - return ret; >> + return 0; >> } >> >> static int venc_queue_setup(struct vb2_queue *q, >> @@ -781,7 +779,7 @@ static int venc_queue_setup(struct vb2_queue *q, >> { >> struct venus_inst *inst = vb2_get_drv_priv(q); >> unsigned int num, min = 4; >> - int ret = 0; >> + int ret; >> >> if (*num_planes) { >> if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE && >> @@ -803,6 +801,17 @@ static int venc_queue_setup(struct vb2_queue *q, >> return 0; >> } >> >> + ret = mutex_lock_interruptible(&inst->lock); I'll keep original mutex_lock here in next version. >> + if (ret) >> + return ret; >> + >> + ret = venc_init_session(inst); >> + >> + mutex_unlock(&inst->lock); >> + >> + if (ret) >> + return ret; >> + >> switch (q->type) { >> case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: >> *num_planes = inst->fmt_out->num_planes; >> @@ -838,6 +847,54 @@ static int venc_queue_setup(struct vb2_queue *q, >> return ret; >> } >> >> +static int venc_buf_init(struct vb2_buffer *vb) >> +{ >> + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); >> + >> + inst->buf_count++; >> + >> + return venus_helper_vb2_buf_init(vb); >> +} >> + >> +static void venc_release_session(struct venus_inst *inst) >> +{ >> + int ret, abort = 0; >> + >> + mutex_lock(&inst->lock); >> + >> + ret = hfi_session_deinit(inst); >> + abort = (ret && ret != -EINVAL) ? 1 : 0; > > Here as well, I think a comment is warranted to explain why we can > ignore EINVAL. OK, will update that. > >> + >> + if (inst->session_error) >> + abort = 1; >> + >> + if (abort) >> + hfi_session_abort(inst); >> + >> + mutex_unlock(&inst->lock); >> + >> + venus_pm_load_scale(inst); >> + INIT_LIST_HEAD(&inst->registeredbufs); >> + venus_pm_release_core(inst); >> +} >> + >> +static void venc_buf_cleanup(struct vb2_buffer *vb) >> +{ >> + struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue); >> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); >> + struct venus_buffer *buf = to_venus_buffer(vbuf); >> + >> + mutex_lock(&inst->lock); >> + if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) >> + if (!list_empty(&inst->registeredbufs)) >> + list_del_init(&buf->reg_list); >> + mutex_unlock(&inst->lock); >> + >> + inst->buf_count--; >> + if (!inst->buf_count) >> + venc_release_session(inst); > > We are calling venc_init_session() during the queue setup but > venc_release_session() when the last buffer is cleaned up. For > symmetry, wouldn't it make sense to call venc_init_session() when the > first buffer is initialized by venc_buf_init()? Otherwise we can No, the session must be initialized in queue_setup in order to return the number and sizes of source/destination buffers. I raised several times the need of symmetrical operation to queue_setup to cover reqbuf(0) but there is no progress on that. Latest suggestion was to use .vidioc_reqbufs ioctl op but I fall with some other issues and at the end I came to this counting buf_init|cleanup solution. > potentially have a scenario where the queue is set up, but no buffer > is ever created, leading to the session never being released. dmabuf import case? -- regards, Stan