Message-ID: <CAA8EJprMr2vjZki7+p1zepCvmeocnGtZ0w5Ew8rW38=RKyQ7UQ@mail.gmail.com>
Date: Wed, 20 Dec 2023 23:24:48 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Abhinav Kumar <quic_abhinavk@...cinc.com>
Cc: Vikash Garodia <quic_vgarodia@...cinc.com>, Dikshita Agarwal <quic_dikshita@...cinc.com>, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	stanimir.k.varbanov@...il.com, agross@...nel.org, andersson@...nel.org, 
	konrad.dybcio@...aro.org, mchehab@...nel.org, bryan.odonoghue@...aro.org, 
	linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 00/34] Qualcomm video encoder and decoder driver

Hi Abhinav,

On Wed, 20 Dec 2023 at 20:55, Abhinav Kumar <quic_abhinavk@...cinc.com> wrote:
>
> Hi Dmitry and Krzysztof
>
> On 12/20/2023 1:52 AM, Dmitry Baryshkov wrote:
> > On Wed, 20 Dec 2023 at 10:53, Vikash Garodia <quic_vgarodia@...cinc.com> wrote:
> >>
> >> On 12/20/2023 2:09 PM, Dmitry Baryshkov wrote:
> >>> On Wed, 20 Dec 2023 at 10:14, Vikash Garodia <quic_vgarodia@...cinc.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@...cinc.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.
> >
>
> Your comments are valid. The platform driver piece and some other pieces
> still are common between venus and iris despite this initial effort of
> moving common pieces out. I have also seen this from whatever I have
> reviewed.
>
> Video team also acknowledges this part and internally I think they did
> evaluate this option and their feedback was, the more and more they
> changed, they were touching pretty much every file of venus.

This is fine from my POV. Splitting the the common functionality also
touched a significant part of the venus driver in a pretty bad way.

> The missing piece i think in all this discussion is that in addition to
> the forward channel, the reverse channel of HFI, based on which the rest
> of the video state machine changes should also be considered.
>
> So even with respect to the code layout, its not just the forward
> communication but the backwards communication of fw--->hfi--->codec is
> becoming a challenge as the venus layers seem to only work with the hfi
> of venus.

Again, this is a question of the platform backend part. Nobody
questions that newer platforms have their own driver interface.
We are not asking your team to add if(new) { foo; } else { bar; }
conditions all over the code. Instead we have been asking to split all
platform specifics to the 'backend'. Compare this with mdp4, mdp5 and
dpu backends of the single drm/msm frontend. Or with a similar
approach found in other DRM drivers. Adding new driver is frequently
unjustified, instead the existing driver gets extended to support
different generations.

> For adding support for the new HFI events/communication, it was getting
> harder to extend venus.
>
> What I certainly acknowledge is now with iris whether this can be dealt
> with better for future chipsets to avoid another re-write for another
> HFI as that will be too much of maintenance cost.

Another approach, which might also be fine, if that looks better from
your point of view. The current venus driver might have issues with
the internal interfaces. Or it might be not suitable for the backends.
In this case, can we please get older platforms also supported by the
iris driver? This will be a very simple case then: the old deprecated
driver, being phased out and removed, and a new one, which is being
phased in.

The problem is very simple from my side. I do not want to end up in a
situation where we have to change platform code for both drivers if
there is any common issue. Neither would I like for venus and iris
bindings to diverge more than necessary. And as you have seen from my
comments, the iris bindings already do not follow the venus example.

> I will let the video team comment on this part.
>
> Thanks
>
> Abhinav
>
>
>
>
>
> >>
> >>>>
> >>>> [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

Powered by blists - more mailing lists