Re: [PATCH 7/7] ARM: dts: at91: sama5d4: add vdec0 component

[Emil Velikov <emil.l.velikov@gmail.com>]

 Hi Ezequiel,

Thanks for the prompt reply

On Sat, 6 Mar 2021 at 11:25, Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> (adding another Nicolas)
>
> Hi Emil,
>
> Thanks a lot for the patch.
>
> On Fri, 2021-03-05 at 18:39 +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
> > The SoC features a Hantro G1 compatible video decoder.
> >
> > Cc: Ezequiel Garcia <ezequiel@collabora.com>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Cc: linux-media@vger.kernel.org
> > Cc: linux-rockchip@lists.infradead.org
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > ---
> >  arch/arm/boot/dts/sama5d4.dtsi                |   9 ++
>
> Usually device-tree changes go to their own patch.
>
> The new compatible string "atmel,sama5d4-vdec" needs
> an update to the DT bindings, see Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> for an example.
>
> DT bindings change should also go to a separate
> patch, see Documentation/devicetree/bindings/submitting-patches.rst.
>
Will do. Thanks

> >  arch/arm/configs/sama5_defconfig              |   3 +
>
> Better if config changes are a separate patch.
>
> But also, the driver is in staging and we haven't enabled
> in ARM64 defconfig. Let's leave that decision to the machine
> maintainer to decide.
>
Makes sense. Will keep it separate patch for completeness sake, with
explicit note.
ARM/Microchip (AT91) SoC maintainers will be in CC list and will defer
the decision to them.

> > +static const struct hantro_fmt sama5d4_vdec_postproc_fmts[] = {
> > +       {
> > +               .fourcc = V4L2_PIX_FMT_YUYV,
> > +               .codec_mode = HANTRO_MODE_NONE,
> > +       },
> > +};
> > +
>
> I haven't found information on how the series
> was tested in the cover letter, could you add that to the next
> iteration?
>
> Please test the YUYV post-processed output and MPEG-2 decoding as well.
>
Any recommendations for MPEG-2 and post-processing testing? For the
former I could use gstreamer on Big Buck Bunny or other media, yet not
sure about the latter.

> Also add the fluster score on this platform, and while here you could
> give a pass at v4l2-compliance, which should pass without failures.
> Note that you need to use v4l2-compliance HEAD from git.
>
> https://git.linuxtv.org/v4l-utils.git
>
Ack, will do. Fwiw I did not see any results in the i.MX8M series so I
followed suit :-P

> > +static int sama5d4_hw_init(struct hantro_dev *vpu)
> > +{
> > +       return 0;
>
> Ah, the hantro_variant.init ops is not optional, but
> if this VPU has no hw-specific init, then it should be.
>
> In any case, we might get rid of it soon: Benjamin's work
> will hopefully remove the i.MX8M need for ctrl_base.
>
> And then the static clk_set_rate() in Rockchip variants could
> be replaced with some dynamic rate using devfreq.
>
Neat looking forward to it.

Thanks
Emil