From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Vikash Garodia <quic_vgarodia@quicinc.com>
Cc: Dikshita Agarwal <quic_dikshita@quicinc.com>,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
stanimir.k.varbanov@gmail.com, agross@kernel.org,
andersson@kernel.org, konrad.dybcio@linaro.org,
mchehab@kernel.org, bryan.odonoghue@linaro.org,
linux-arm-msm@vger.kernel.org, quic_abhinavk@quicinc.com
Subject: Re: [PATCH v2 00/34] Qualcomm video encoder and decoder driver
Date: Wed, 20 Dec 2023 11:52:38 +0200 [thread overview]
Message-ID: <CAA8EJprZ1TK7UwfhSh2PtwuNJLUMace7MWnzQkrUMqV5R+WgOA@mail.gmail.com> (raw)
In-Reply-To: <eb288a33-a8c3-9dea-ffc1-e97a69be9a4c@quicinc.com>
On Wed, 20 Dec 2023 at 10:53, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
>
> On 12/20/2023 2:09 PM, Dmitry Baryshkov wrote:
> > On Wed, 20 Dec 2023 at 10:14, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
> >>
> >> Hi,
> >>
> >> On 12/20/2023 1:07 PM, Dmitry Baryshkov wrote:
> >>> On Wed, 20 Dec 2023 at 08:32, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
> >>>>
> >>>> Hi Dmitry,
> >>>>
> >>>> On 12/19/2023 12:08 AM, Dmitry Baryshkov wrote:
> >>>>> On 18/12/2023 13:31, Dikshita Agarwal wrote:
> >>>>>> This patch series introduces support for Qualcomm new video acceleration
> >>>>>> hardware architecture, used for video stream decoding/encoding. This driver
> >>>>>> is based on new communication protocol between video hardware and application
> >>>>>> processor.
> >>>>>
> >>>>> This doesn't answer one important point, you have been asked for v1. What is the
> >>>>> actual change point between Venus and Iris? What has been changed so much that
> >>>>> it demands a separate driver. This is the main question for the cover letter,
> >>>>> which has not been answered so far.
> >>>>>
> >>>>> From what I see from you bindings, the hardware is pretty close to what we see
> >>>>> in the latest venus generations. I asssme that there was a change in the vcodec
> >>>>> inteface to the firmware and other similar changes. Could you please point out,
> >>>>> which parts of Venus driver do no longer work or are not applicable for sm8550
> >>>>
> >>>> The motivation behind having a separate IRIS driver was discussed earlier in [1]
> >>>> In the same discussion, it was ellaborated on how the impact would be with
> >>>> change in the new firmware interface and other video layers in the driver. I can
> >>>> add this in cover letter in the next revision.
> >>>
> >>> Ok. So the changes cover the HFI interface. Is that correct?
> >> Change wise, yes.
> >>
> >>>> We see some duplication of code and to handle the same, the series brings in a
> >>>> common code reusability between iris and venus. Aligning the common peices of
> >>>> venus and iris will be a work in progress, once we land the base driver for iris.
> >>>
> >>> This is not how it usually works. Especially not with the patches you
> >>> have posted.
> >>>
> >>> I have the following suggestion how this story can continue:
> >>> You can _start_ by reworking venus driver, separating the HFI /
> >>> firmware / etc interface to an internal interface in the driver. Then
> >>> implement Iris as a plug in for that interface. I might be mistaken
> >>> here, but I think this is the way how this can be beneficial for both
> >>> the video en/decoding on both old and new platforms.
> >>
> >> HFI/firmware interface is already confined to HFI layer in the existing venus
> >> driver. We explained in the previous discussion [1], on how the HFI change
> >> impacts the other layers by taking example of a DRC usecase. Please have a look
> >> considering the usecase and the impact it brings to other layers in the driver.
> >
> > I have looked at it. And I still see huge change in the interface
> > side, but it doesn't tell me about the hardware changes.
>
> I hope you noticed how the common layers like decoder, response, state layers
> are impacted in handling one of usecase. Now add that to all the different
> scenarios like seek, drain, DRC during seek, DRC during drain, etc.
Yes, for sure.
>
> > Have you evaluated the other opportunity?
> >
> > To have a common platform interface and firmware-specific backend?
> >
> > You have already done a part of it, but from a different perspective.
> > You have tried to move common code out of the driver. Instead we are
> > asking you to do a different thing. Move non-common code within the
> > driver. Then add your code on top of that.
>
> For common platform - yes, we are bringing in common stuff like PIL.
> Other than that, abstraction to firmware interface is not that confined to one
> layer. It spreads over decoder/encoder/common layers. Now when majority of the
> layers/code is different, we planned to make it in parallel to venus and have a
> common layer having common things to both iris and venus.
My suggestion still holds. Start with this common platform code.
Rather than arguing back and forth, could you please perform an
experiment on the current venus driver and move firmware interface to
subdirs, leaving just the platform / PIL / formats / etc in place?
This will at least allow us to determine whether it is a feasible
concept or not.
>
> >>
> >> [1] https://lore.kernel.org/lkml/8c97d866-1cab-0106-4ab3-3ca070945ef7@quicinc.com/
> >>> Short rationale:
> >>> The venus driver has a history of supported platforms. There is
> >>> already some kind of buffer management in place. Both developers and
> >>> testers have spent their effort on finding issues there. Sending new
> >>> driver means that we have to spend the same amount of efforts on this.
> >>> Moreover, even from the porter point of view. You are creating new
> >>> bindings for the new hardware. Which do not follow the
> >>> venus-common.yaml. And they do not follow the defined bindings for the
> >>> recent venus platforms. Which means that as a developer I have to care
> >>> about two different ways to describe nearly the same hardware.>> Again qualcomm video team does not have a plan to support sm8550/x1e80100 on
> >>>> venus as the changes are too interleaved to absorb in venus driver. And there is
> >>>> significant interest in community to start validating video driver on sm8550 or
> >>>> x1e80100.
> >>>>
> >>>> [1] https://lore.kernel.org/lkml/8c97d866-1cab-0106-4ab3-3ca070945ef7@quicinc.com/
> >>>>
> >>>> Regards,
> >>>> Vikash
> >>>
> >>>
> >>>
> >
> >
> >
--
With best wishes
Dmitry
next prev parent reply other threads:[~2023-12-20 9:52 UTC|newest]
Thread overview: 103+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-18 11:31 [PATCH v2 00/34] Qualcomm video encoder and decoder driver Dikshita Agarwal
2023-12-18 11:31 ` [PATCH v2 01/34] media: introduce common helpers for video firmware handling Dikshita Agarwal
2023-12-18 18:24 ` Dmitry Baryshkov
2023-12-20 8:01 ` Dikshita Agarwal
2023-12-20 8:12 ` Dmitry Baryshkov
2023-12-20 17:10 ` Abhinav Kumar
2023-12-20 20:56 ` Dmitry Baryshkov
2023-12-20 21:03 ` Abhinav Kumar
2023-12-19 11:40 ` Bryan O'Donoghue
2023-12-19 13:26 ` Dmitry Baryshkov
2023-12-20 8:01 ` Dikshita Agarwal
2023-12-18 11:31 ` [PATCH v2 02/34] media: introduce common helpers for queues handling Dikshita Agarwal
2023-12-18 18:29 ` Dmitry Baryshkov
2023-12-20 8:03 ` Dikshita Agarwal
2023-12-18 11:31 ` [PATCH v2 03/34] media: introduce common helpers for buffer size calculation Dikshita Agarwal
2023-12-18 18:32 ` Dmitry Baryshkov
2023-12-20 8:04 ` Dikshita Agarwal
2023-12-20 8:15 ` Dmitry Baryshkov
2023-12-18 11:31 ` [PATCH v2 04/34] dt-bindings: media: Add sm8550 dt schema Dikshita Agarwal
2023-12-18 16:10 ` Krzysztof Kozlowski
2023-12-18 18:17 ` Dmitry Baryshkov
2023-12-18 11:32 ` [PATCH v2 05/34] media: MAINTAINERS: Add Qualcomm Iris video accelerator driver Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 06/34] media: iris: register video device to platform driver Dikshita Agarwal
2023-12-18 18:40 ` Dmitry Baryshkov
2023-12-18 11:32 ` [PATCH v2 07/34] media: iris: initialize power resources Dikshita Agarwal
2023-12-18 15:09 ` Konrad Dybcio
2023-12-20 8:04 ` Dikshita Agarwal
2024-01-03 13:45 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 08/34] media: iris: introduce state machine for iris core Dikshita Agarwal
2023-12-18 18:46 ` Dmitry Baryshkov
2023-12-20 8:05 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 09/34] media: iris: initialize shared queues for host and firmware communication Dikshita Agarwal
2023-12-18 18:46 ` Dmitry Baryshkov
2023-12-20 8:05 ` Dikshita Agarwal
2023-12-18 21:33 ` Konrad Dybcio
2023-12-20 8:05 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 10/34] media: iris: add PIL functionality for video firmware Dikshita Agarwal
2023-12-18 21:40 ` Konrad Dybcio
2023-12-20 8:15 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 11/34] media: iris: introduce packetization layer for creating HFI packets Dikshita Agarwal
2023-12-18 21:50 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 12/34] media: iris: add video processing unit(VPU) specific register handling Dikshita Agarwal
2023-12-18 16:19 ` Krzysztof Kozlowski
2023-12-18 22:00 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 13/34] media: iris: introduce platform specific capabilities for core and instance Dikshita Agarwal
2023-12-18 22:08 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 14/34] media: iris: implement iris v4l2 file ops Dikshita Agarwal
2023-12-19 11:57 ` Bryan O'Donoghue
2023-12-18 11:32 ` [PATCH v2 15/34] media: iris: add handling for interrupt service routine(ISR) invoked by hardware Dikshita Agarwal
2023-12-19 11:48 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 16/34] media: iris: implement iris v4l2_ctrl_ops and prepare capabilities Dikshita Agarwal
2023-12-19 11:50 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 17/34] media: iris: implement vb2_ops queue setup Dikshita Agarwal
2023-12-19 11:56 ` Konrad Dybcio
2023-12-20 8:37 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 18/34] media: iris: introduce and implement iris vb2 mem ops Dikshita Agarwal
2023-12-19 11:58 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 19/34] media: iris: implement HFI to queue and release buffers Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 20/34] media: iris: add video hardware internal buffer count and size calculation Dikshita Agarwal
2023-12-19 12:06 ` Bryan O'Donoghue
2023-12-20 8:29 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 21/34] media: iris: implement internal buffer management Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 22/34] media: iris: introduce instance states Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 23/34] media: iris: implement iris v4l2 ioctl ops supported by decoder Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 24/34] media: iris: subscribe input and output properties to firmware Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 25/34] media: iris: subscribe src change and handle firmware responses Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 26/34] media: iris: implement vb2 streaming ops on capture and output planes Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 27/34] media: iris: implement vb2 ops for buf_queue and firmware response Dikshita Agarwal
2023-12-19 12:21 ` Konrad Dybcio
2023-12-20 8:25 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 28/34] media: iris: add instance sub states and implement DRC and Drain sequence Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 29/34] media: iris: implement power management Dikshita Agarwal
2023-12-19 12:24 ` Konrad Dybcio
2023-12-20 8:23 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 30/34] media: iris: register video encoder device to platform driver Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 31/34] media: iris: add platform specific instance capabilities for encoder Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 32/34] media: iris: implement iris v4l2 ioctl ops supported by encoder Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 33/34] media: iris: add vb2 streaming and buffer ops for encoder Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 34/34] media: iris: add power management " Dikshita Agarwal
2023-12-19 12:26 ` Konrad Dybcio
2023-12-18 12:09 ` [PATCH v2 00/34] Qualcomm video encoder and decoder driver Dikshita Agarwal
2023-12-18 14:36 ` Bryan O'Donoghue
2023-12-18 18:38 ` Dmitry Baryshkov
2023-12-19 12:10 ` Bryan O'Donoghue
2023-12-20 6:32 ` Vikash Garodia
2023-12-20 7:37 ` Dmitry Baryshkov
2023-12-20 8:14 ` Vikash Garodia
2023-12-20 8:39 ` Dmitry Baryshkov
2023-12-20 8:53 ` Vikash Garodia
2023-12-20 9:52 ` Dmitry Baryshkov [this message]
2023-12-20 18:55 ` Abhinav Kumar
2023-12-20 21:24 ` Dmitry Baryshkov
2023-12-20 8:15 ` Krzysztof Kozlowski
2023-12-20 8:32 ` Vikash Garodia
2023-12-20 20:51 ` Nicolas Dufresne
2024-02-29 15:09 ` Vikash Garodia
2024-03-12 10:37 ` Hans Verkuil
2024-03-15 13:51 ` Vikash Garodia
2024-04-12 7:13 ` Hyunjun Ko
2024-04-12 13:52 ` Bryan O'Donoghue
2024-05-16 7:57 ` Hyunjun Ko
2024-05-16 8:06 ` Vikash Garodia
-- strict thread matches above, loose matches on Subject: below --
2023-12-18 16:07 Xilin Wu
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=CAA8EJprZ1TK7UwfhSh2PtwuNJLUMace7MWnzQkrUMqV5R+WgOA@mail.gmail.com \
--to=dmitry.baryshkov@linaro.org \
--cc=agross@kernel.org \
--cc=andersson@kernel.org \
--cc=bryan.odonoghue@linaro.org \
--cc=konrad.dybcio@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=quic_abhinavk@quicinc.com \
--cc=quic_dikshita@quicinc.com \
--cc=quic_vgarodia@quicinc.com \
--cc=stanimir.k.varbanov@gmail.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).