Re: [PATCH v9 03/13] media: hantro: Use syscon instead of 'ctrl' register

[Ezequiel Garcia]

Hi Benjamin,

On Mon, 2021-06-28 at 15:35 +0200, Benjamin Gaignard wrote:
> 
> Le 16/04/2021 à 12:54, Lucas Stach a écrit :
> > Am Mittwoch, dem 07.04.2021 um 09:35 +0200 schrieb Benjamin Gaignard:
> > > In order to be able to share the control hardware block between
> > > VPUs use a syscon instead a ioremap it in the driver.
> > > To keep the compatibility with older DT if 'nxp,imx8mq-vpu-ctrl'
> > > phandle is not found look at 'ctrl' reg-name.
> > > With the method it becomes useless to provide a list of register
> > > names so remove it.
> > Sorry for putting a spoke in the wheel after many iterations of the
> > series.
> > 
> > We just discussed a way forward on how to handle the clocks and resets
> > provided by the blkctl block on i.MX8MM and later and it seems there is
> > a consensus on trying to provide virtual power domains from a blkctl
> > driver, controlling clocks and resets for the devices in the power
> > domain. I would like to avoid introducing yet another way of handling
> > the blkctl and thus would like to align the i.MX8MQ VPU blkctl with
> > what we are planning to do on the later chip generations.
> > 
> > CC'ing Jacky Bai and Peng Fan from NXP, as they were going to give this
> > virtual power domain thing a shot.
> 
> Hey guys,
> 
> I may I have miss them but I haven't see patches about power domain for IMX8MQ
> VPU control block ?
> Is it something that you still plan to do ?
> If not, I can resend my patches where I use syscon.
> 

Please see "soc: imx: add i.MX BLK-CTL support" [1] sent by Peng
a couple weeks ago. It adds the VPUMIX for i.MX8MM, so it seems
the best way forward is to follow that design, extending it for
i.MX8MQ.

That's still under discussion, but hopefully it will be sorted out for v5.15.

Speaking of i.MX8MM, I got a report that the Hantro G1 block mostly
work, but needs to be restricted to 1920x1080. If you could add a new
compatible and variant for that, maybe we can find someone to test it.

[1] 
https://lore.kernel.org/linux-arm-kernel/7683ab0b-f905-dff1-aa4f-76ad638da...@oss.nxp.com/T/#mf73fe4a13aec0a8e633a14a5d9c2d5609799acb4

Kindly,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v9 03/13] media: hantro: Use syscon instead of 'ctrl' register

[Ezequiel Garcia]

Hi Lucas,

On Mon, 2021-05-17 at 12:52 +0200, Lucas Stach wrote:
> Hi Ezequiel,
> 
> Am Sonntag, dem 16.05.2021 um 19:40 -0300 schrieb Ezequiel Garcia:
> > Hi Lucas,
> > 
> > On Fri, 2021-04-16 at 12:54 +0200, Lucas Stach wrote:
> > > Am Mittwoch, dem 07.04.2021 um 09:35 +0200 schrieb Benjamin Gaignard:
> > > > In order to be able to share the control hardware block between
> > > > VPUs use a syscon instead a ioremap it in the driver.
> > > > To keep the compatibility with older DT if 'nxp,imx8mq-vpu-ctrl'
> > > > phandle is not found look at 'ctrl' reg-name.
> > > > With the method it becomes useless to provide a list of register
> > > > names so remove it.
> > > 
> > > Sorry for putting a spoke in the wheel after many iterations of the
> > > series.
> > > 
> > > We just discussed a way forward on how to handle the clocks and resets
> > > provided by the blkctl block on i.MX8MM and later and it seems there is
> > > a consensus on trying to provide virtual power domains from a blkctl
> > > driver, controlling clocks and resets for the devices in the power
> > > domain. I would like to avoid introducing yet another way of handling
> > > the blkctl and thus would like to align the i.MX8MQ VPU blkctl with
> > > what we are planning to do on the later chip generations.
> > > 
> > > CC'ing Jacky Bai and Peng Fan from NXP, as they were going to give this
> > > virtual power domain thing a shot.
> > > 
> > 
> > It seems the i.MX8MM BLK-CTL series are moving forward:
> > 
> > https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=479175
> > 
> > ... but I'm unable to wrap my head around how this affects the
> > devicetree VPU modelling for i.MX8MQ (and also i.MX8MM, i.MX8MP, ...).
> > 
> > 
> For the i.MX8MQ we want to have the same virtual power-domains provided
> by a BLK-CTRL driver for the VPUs, as on i.MX8MM. This way we should be
> able to use the same DT bindings for the VPUs on i.MX8MQ and i.MX8MM,
> even though the SoC integration with the blk-ctrl is a little
> different.
> 
> > Can you clarify that?
> > 
> I'm planning on sending some patches adding i.MX8MQ VPU support to the
> BLK-CTRL driver in the next few days. I guess that should clarify
> things. :)
> 

As a gentle reminder, Hans sent the i.MX8MQ G2 HEVC support pull request
and Benjamin just posted a series adding support for more features.

Do you think we could have the blk-ctrl support landing in v5.14?

If you work on the patches, and you happen to test the G1 and G2 on
i.MX8MM it would be great to add that too.

Meanwhile, our next steps would be to improve the HEVC V4L2 uAPI itself.

Thanks a lot!
Ezequiel 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v9 03/13] media: hantro: Use syscon instead of 'ctrl' register

[Ezequiel Garcia]

On Mon, 2021-05-17 at 12:52 +0200, Lucas Stach wrote:
> Hi Ezequiel,
> 
> Am Sonntag, dem 16.05.2021 um 19:40 -0300 schrieb Ezequiel Garcia:
> > Hi Lucas,
> > 
> > On Fri, 2021-04-16 at 12:54 +0200, Lucas Stach wrote:
> > > Am Mittwoch, dem 07.04.2021 um 09:35 +0200 schrieb Benjamin Gaignard:
> > > > In order to be able to share the control hardware block between
> > > > VPUs use a syscon instead a ioremap it in the driver.
> > > > To keep the compatibility with older DT if 'nxp,imx8mq-vpu-ctrl'
> > > > phandle is not found look at 'ctrl' reg-name.
> > > > With the method it becomes useless to provide a list of register
> > > > names so remove it.
> > > 
> > > Sorry for putting a spoke in the wheel after many iterations of the
> > > series.
> > > 
> > > We just discussed a way forward on how to handle the clocks and resets
> > > provided by the blkctl block on i.MX8MM and later and it seems there is
> > > a consensus on trying to provide virtual power domains from a blkctl
> > > driver, controlling clocks and resets for the devices in the power
> > > domain. I would like to avoid introducing yet another way of handling
> > > the blkctl and thus would like to align the i.MX8MQ VPU blkctl with
> > > what we are planning to do on the later chip generations.
> > > 
> > > CC'ing Jacky Bai and Peng Fan from NXP, as they were going to give this
> > > virtual power domain thing a shot.
> > > 
> > 
> > It seems the i.MX8MM BLK-CTL series are moving forward:
> > 
> > https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=479175
> > 
> > ... but I'm unable to wrap my head around how this affects the
> > devicetree VPU modelling for i.MX8MQ (and also i.MX8MM, i.MX8MP, ...).
> > 
> > 
> For the i.MX8MQ we want to have the same virtual power-domains provided
> by a BLK-CTRL driver for the VPUs, as on i.MX8MM. This way we should be
> able to use the same DT bindings for the VPUs on i.MX8MQ and i.MX8MM,
> even though the SoC integration with the blk-ctrl is a little
> different.
> 

AFAICS, there's not support for i.MX8MP VPU power domains. I suppose
we should make sure we'll be able to cover those as well.

Will i.MX8MP need its own driver as well?

> > Can you clarify that?
> > 
> I'm planning on sending some patches adding i.MX8MQ VPU support to the
> BLK-CTRL driver in the next few days. I guess that should clarify
> things. :)
> 

Great.

Thanks a lot,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v10 6/9] media: uapi: Add a control for HANTRO driver

[Ezequiel Garcia]

Hi Hans,

On Thu, 2021-05-06 at 14:50 +0200, Hans Verkuil wrote:
> On 05/05/2021 17:20, Benjamin Gaignard wrote:
> > 
> > Le 05/05/2021 à 16:55, Hans Verkuil a écrit :
> > > On 20/04/2021 14:10, Benjamin Gaignard wrote:
> > > > The HEVC HANTRO driver needs to know the number of bits to skip at
> > > > the beginning of the slice header.
> > > > That is a hardware specific requirement so create a dedicated control
> > > > for this purpose.
> > > > 
> > > > Signed-off-by: Benjamin Gaignard 
> > > > ---
> > > >   .../userspace-api/media/drivers/hantro.rst    | 19 +++
> > > >   .../userspace-api/media/drivers/index.rst |  1 +
> > > >   include/media/hevc-ctrls.h    | 13 +
> > > >   3 files changed, 33 insertions(+)
> > > >   create mode 100644 
> > > > Documentation/userspace-api/media/drivers/hantro.rst
> > > > 
> > > > diff --git a/Documentation/userspace-api/media/drivers/hantro.rst 
> > > > b/Documentation/userspace-api/media/drivers/hantro.rst
> > > > new file mode 100644
> > > > index ..cd9754b4e005
> > > > --- /dev/null
> > > > +++ b/Documentation/userspace-api/media/drivers/hantro.rst
> > > > @@ -0,0 +1,19 @@
> > > > +.. SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +Hantro video decoder driver
> > > > +===
> > > > +
> > > > +The Hantro video decoder driver implements the following 
> > > > driver-specific controls:
> > > > +
> > > > +``V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP (integer)``
> > > > +    Specifies to Hantro HEVC video decoder driver the number of data 
> > > > (in bits) to
> > > > +    skip in the slice segment header.
> > > > +    If non-IDR, the bits to be skipped go from syntax element 
> > > > "pic_output_flag"
> > > > +    to before syntax element "slice_temporal_mvp_enabled_flag".
> > > > +    If IDR, the skipped bits are just "pic_output_flag"
> > > > +    (separate_colour_plane_flag is not supported).
> > > I'm not very keen on this. Without this information the video data cannot 
> > > be
> > > decoded, or will it just be suboptimal?
> > 
> > Without that information the video can't be decoded.
> > 
> > > 
> > > The problem is that a generic decoder would have to know that the HW is a 
> > > hantro,
> > > and then call this control. If they don't (and are testing on non-hantro 
> > > HW), then
> > > it won't work, thus defeating the purpose of the HW independent decoder 
> > > API.
> > > 
> > > Since hantro is widely used, and if there is no other way to do this 
> > > beside explitely
> > > setting this control, then perhaps this should be part of the standard 
> > > HEVC API.
> > > Non-hantro drivers that do not need this can just skip it.
> > 
> > Even if I put this parameter in decode_params structure that would means 
> > that a generic
> > userland decoder will have to know how the compute this value for hantro HW 
> > since it
> > isn't something that could be done on kernel side.
> 
> But since hantro is very common, any userland decoder will need to calculate 
> this anyway.
> So perhaps it is better to have this as part of the decode_params?
> 
> I'd like to know what others think about this.
> 

As you know, I'm not a fan of carrying these "unstable" APIs around.
I know it's better than nothing, but I feel they create the illusion
of the interface being supported in mainline. Since it's unstable,
it's difficult for applications to adopt them.

As Nicolas mentioned, this means neither FFmpeg nor GStreamer will adopt
these APIs, which worries me, as that means we lose two major user bases.

My personal take from this, is that we need to find ways to stabilize
our stateless codec APIs in less time and perhaps with less effort.

IMO, a less stiff interface could help us here, and that's why I think
having hardware-specific controls can be useful. Hardware designers
can be so creative :)

I'm not against introducing this specific parameter in
v4l2_ctrl_hevc_codec_params, arguing that Hantro is widely used,
but I'd like us to be open to hardware-specific controls as a way
to extend the APIs seamlessly.

Applications won't have to _know_ what hardware they are running on,
they can just use VIDIOC_QUERYCTRL to find out which controls are needed.

Thanks,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v9 03/13] media: hantro: Use syscon instead of 'ctrl' register

[Ezequiel Garcia]

Hi Lucas,

On Fri, 2021-04-16 at 12:54 +0200, Lucas Stach wrote:
> Am Mittwoch, dem 07.04.2021 um 09:35 +0200 schrieb Benjamin Gaignard:
> > In order to be able to share the control hardware block between
> > VPUs use a syscon instead a ioremap it in the driver.
> > To keep the compatibility with older DT if 'nxp,imx8mq-vpu-ctrl'
> > phandle is not found look at 'ctrl' reg-name.
> > With the method it becomes useless to provide a list of register
> > names so remove it.
> 
> Sorry for putting a spoke in the wheel after many iterations of the
> series.
> 
> We just discussed a way forward on how to handle the clocks and resets
> provided by the blkctl block on i.MX8MM and later and it seems there is
> a consensus on trying to provide virtual power domains from a blkctl
> driver, controlling clocks and resets for the devices in the power
> domain. I would like to avoid introducing yet another way of handling
> the blkctl and thus would like to align the i.MX8MQ VPU blkctl with
> what we are planning to do on the later chip generations.
> 
> CC'ing Jacky Bai and Peng Fan from NXP, as they were going to give this
> virtual power domain thing a shot.
> 

It seems the i.MX8MM BLK-CTL series are moving forward:

https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=479175

... but I'm unable to wrap my head around how this affects the
devicetree VPU modelling for i.MX8MQ (and also i.MX8MM, i.MX8MP, ...).

Can you clarify that?

Thanks,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v10 6/9] media: uapi: Add a control for HANTRO driver

[Ezequiel Garcia]

On Wed, 2021-05-05 at 16:55 +0200, Hans Verkuil wrote:
> On 20/04/2021 14:10, Benjamin Gaignard wrote:
> > The HEVC HANTRO driver needs to know the number of bits to skip at
> > the beginning of the slice header.
> > That is a hardware specific requirement so create a dedicated control
> > for this purpose.
> > 
> > Signed-off-by: Benjamin Gaignard 
> > ---
> >  .../userspace-api/media/drivers/hantro.rst    | 19 +++
> >  .../userspace-api/media/drivers/index.rst |  1 +
> >  include/media/hevc-ctrls.h    | 13 +
> >  3 files changed, 33 insertions(+)
> >  create mode 100644 Documentation/userspace-api/media/drivers/hantro.rst
> > 
> > diff --git a/Documentation/userspace-api/media/drivers/hantro.rst 
> > b/Documentation/userspace-api/media/drivers/hantro.rst
> > new file mode 100644
> > index ..cd9754b4e005
> > --- /dev/null
> > +++ b/Documentation/userspace-api/media/drivers/hantro.rst
> > @@ -0,0 +1,19 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +Hantro video decoder driver
> > +===
> > +
> > +The Hantro video decoder driver implements the following driver-specific 
> > controls:
> > +
> > +``V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP (integer)``
> > +    Specifies to Hantro HEVC video decoder driver the number of data (in 
> > bits) to
> > +    skip in the slice segment header.
> > +    If non-IDR, the bits to be skipped go from syntax element 
> > "pic_output_flag"
> > +    to before syntax element "slice_temporal_mvp_enabled_flag".
> > +    If IDR, the skipped bits are just "pic_output_flag"
> > +    (separate_colour_plane_flag is not supported).
> 
> I'm not very keen on this. Without this information the video data cannot be
> decoded, or will it just be suboptimal?
> 
> The problem is that a generic decoder would have to know that the HW is a 
> hantro,

Applications can just query which controls are exposed by a video device,
and if this control is found, then it means it needs to be set.

> and then call this control. If they don't (and are testing on non-hantro HW), 
> then
> it won't work, thus defeating the purpose of the HW independent decoder API.
> 
> Since hantro is widely used, and if there is no other way to do this beside 
> explitely
> setting this control, then perhaps this should be part of the standard HEVC 
> API.
> Non-hantro drivers that do not need this can just skip it.
> 

The decision to move it out of the HEVC API is not really to avoid setting it.
In the end, most/all applications will end up required to set this 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: rkvdec: Fix .buf_prepare

[Ezequiel Garcia]

Hi Andrzej,

Thanks a lot for picking this up.

On Tue, 2021-05-04 at 13:37 +0200, Andrzej Pietrasiewicz wrote:
> From: Ezequiel Garcia 
> 
> The driver should only set the payload on .buf_prepare if the
> buffer is CAPTURE type. If an OUTPUT buffer has a zero bytesused
> set by userspace then v4l2-core will set it to buffer length.
> 
> Fixes: cd33c830448ba ("media: rkvdec: Add the rkvdec driver")
> Signed-off-by: Ezequiel Garcia 
> Signed-off-by: Adrian Ratiu 
> Signed-off-by: Andrzej Pietrasiewicz 
> 
> ---
> @Hans: I haven't had anyone complain about the issue. The fix is needed for
> the rkvdec vp9 work, so I think 5.14 is fine.
> 
>  drivers/staging/media/rkvdec/rkvdec.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c 
> b/drivers/staging/media/rkvdec/rkvdec.c
> index d821661d30f3..ef2166043127 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -481,7 +481,15 @@ static int rkvdec_buf_prepare(struct vb2_buffer *vb)
> if (vb2_plane_size(vb, i) < sizeimage)
> return -EINVAL;
> }
> -   vb2_set_plane_payload(vb, 0, f->fmt.pix_mp.plane_fmt[0].sizeimage);
> +
> +   /*
> +    * Buffer bytesused is written by driver for CAPTURE buffers.
> +    * (if userspace passes 0 bytesused for OUTPUT buffers, v4l2-core sets
> +    * it to buffer length).
> +    */
> +   if (!V4L2_TYPE_IS_OUTPUT(vq->type))

Please use V4L2_TYPE_IS_CAPTURE here.

Also, why is this change needed in rkvdec, but not in cedrus
or hantro?

Thanks!
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v10 6/9] media: uapi: Add a control for HANTRO driver

[Ezequiel Garcia]

On Tue, 2021-04-20 at 14:10 +0200, Benjamin Gaignard wrote:
> The HEVC HANTRO driver needs to know the number of bits to skip at
> the beginning of the slice header.
> That is a hardware specific requirement so create a dedicated control
> for this purpose.
> 
> Signed-off-by: Benjamin Gaignard 

Reviewed-by: Ezequiel Garcia 

> ---
>  .../userspace-api/media/drivers/hantro.rst    | 19 +++
>  .../userspace-api/media/drivers/index.rst |  1 +
>  include/media/hevc-ctrls.h    | 13 +
>  3 files changed, 33 insertions(+)
>  create mode 100644 Documentation/userspace-api/media/drivers/hantro.rst
> 
> diff --git a/Documentation/userspace-api/media/drivers/hantro.rst 
> b/Documentation/userspace-api/media/drivers/hantro.rst
> new file mode 100644
> index ..cd9754b4e005
> --- /dev/null
> +++ b/Documentation/userspace-api/media/drivers/hantro.rst
> @@ -0,0 +1,19 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Hantro video decoder driver
> +===
> +
> +The Hantro video decoder driver implements the following driver-specific 
> controls:
> +
> +``V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP (integer)``
> +    Specifies to Hantro HEVC video decoder driver the number of data (in 
> bits) to
> +    skip in the slice segment header.
> +    If non-IDR, the bits to be skipped go from syntax element 
> "pic_output_flag"
> +    to before syntax element "slice_temporal_mvp_enabled_flag".
> +    If IDR, the skipped bits are just "pic_output_flag"
> +    (separate_colour_plane_flag is not supported).
> +
> +.. note::
> +
> +    This control is not yet part of the public kernel API and
> +    it is expected to change.
> diff --git a/Documentation/userspace-api/media/drivers/index.rst 
> b/Documentation/userspace-api/media/drivers/index.rst
> index 1a9038f5f9fa..12e3c512d718 100644
> --- a/Documentation/userspace-api/media/drivers/index.rst
> +++ b/Documentation/userspace-api/media/drivers/index.rst
> @@ -33,6 +33,7 @@ For more details see the file COPYING in the source 
> distribution of Linux.
>  
> ccs
> cx2341x-uapi
> +    hantro
> imx-uapi
> max2175
> meye-uapi
> diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
> index 8e0109eea454..b713eeed1915 100644
> --- a/include/media/hevc-ctrls.h
> +++ b/include/media/hevc-ctrls.h
> @@ -224,4 +224,17 @@ struct v4l2_ctrl_hevc_decode_params {
> __u64   flags;
>  };
>  
> +/*  MPEG-class control IDs specific to the Hantro driver as defined by V4L2 
> */
> +#define V4L2_CID_CODEC_HANTRO_BASE 
> (V4L2_CTRL_CLASS_CODEC | 0x1200)
> +/*
> + * V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP -
> + * the number of data (in bits) to skip in the
> + * slice segment header.
> + * If non-IDR, the bits to be skipped go from syntax element 
> "pic_output_flag"
> + * to before syntax element "slice_temporal_mvp_enabled_flag".
> + * If IDR, the skipped bits are just "pic_output_flag"
> + * (separate_colour_plane_flag is not supported).
> + */
> +#define V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP (V4L2_CID_CODEC_HANTRO_BASE + 
> 0)
> +
>  #endif


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 79/79] media: hantro: do a PM resume earlier

[Ezequiel Garcia]

Hi Mauro,

Thanks a lot for taking care of this.

On Wed, 2021-04-28 at 16:52 +0200, Mauro Carvalho Chehab wrote:
> The device_run() first enables the clock and then
> tries to resume PM runtime, checking for errors.
> 
> Well, if for some reason the pm_runtime can not resume,
> it would be better to detect it beforehand.
> 
> So, change the order inside device_run().
> 
> Signed-off-by: Mauro Carvalho Chehab 

Clocks could be behind power-domains, IIRC, so this change
is fixing that.

However, this has ever been a problem for this driver,
so I don't think it makes sense to bother with Fixes tag.

Reviewed-by: Ezequiel Garcia 

Thanks,
Ezequiel

> ---
>  drivers/staging/media/hantro/hantro_drv.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c 
> b/drivers/staging/media/hantro/hantro_drv.c
> index 25fa36e7e773..67de6b15236d 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -160,14 +160,14 @@ static void device_run(void *priv)
> src = hantro_get_src_buf(ctx);
> dst = hantro_get_dst_buf(ctx);
>  
> -   ret = clk_bulk_enable(ctx->dev->variant->num_clocks, 
> ctx->dev->clocks);
> -   if (ret)
> -   goto err_cancel_job;
> -
> ret = pm_runtime_resume_and_get(ctx->dev->dev);
> if (ret < 0)
> goto err_cancel_job;
>  
> +   ret = clk_bulk_enable(ctx->dev->variant->num_clocks, 
> ctx->dev->clocks);
> +   if (ret)
> +   goto err_cancel_job;
> +
> v4l2_m2m_buf_copy_metadata(src, dst, true);
>  
> ctx->codec_ops->run(ctx);


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 78/79] media: hantro: use pm_runtime_resume_and_get()

[Ezequiel Garcia]

On Wed, 2021-04-28 at 16:52 +0200, Mauro Carvalho Chehab wrote:
> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with 
> usage counter")
> added pm_runtime_resume_and_get() in order to automatically handle
> dev->power.usage_count decrement on errors.
> 
> While there's nothing wrong with the current usage on this driver,
> as we're getting rid of the pm_runtime_get_sync() call all over
> the media subsystem, let's remove the last occurrence on this
> driver.
> 
> Signed-off-by: Mauro Carvalho Chehab 

Looks good.

Reviewed-by: Ezequiel Garcia 

Thanks,
Ezequiel

> ---
>  drivers/staging/media/hantro/hantro_drv.c | 23 ---
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c 
> b/drivers/staging/media/hantro/hantro_drv.c
> index 595e82a82728..25fa36e7e773 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -56,14 +56,12 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts)
> return hantro_get_dec_buf_addr(ctx, buf);
>  }
>  
> -static void hantro_job_finish(struct hantro_dev *vpu,
> - struct hantro_ctx *ctx,
> - enum vb2_buffer_state result)
> +static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> +   struct hantro_ctx *ctx,
> +   enum vb2_buffer_state result)
>  {
> struct vb2_v4l2_buffer *src, *dst;
>  
> -   pm_runtime_mark_last_busy(vpu->dev);
> -   pm_runtime_put_autosuspend(vpu->dev);
> clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
>  
> src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> @@ -81,6 +79,16 @@ static void hantro_job_finish(struct hantro_dev *vpu,
>  result);
>  }
>  
> +static void hantro_job_finish(struct hantro_dev *vpu,
> + struct hantro_ctx *ctx,
> + enum vb2_buffer_state result)
> +{
> +   pm_runtime_mark_last_busy(vpu->dev);
> +   pm_runtime_put_autosuspend(vpu->dev);
> +
> +   hantro_job_finish_no_pm(vpu, ctx, result);
> +}
> +
>  void hantro_irq_done(struct hantro_dev *vpu,
>  enum vb2_buffer_state result)
>  {
> @@ -155,7 +163,8 @@ static void device_run(void *priv)
> ret = clk_bulk_enable(ctx->dev->variant->num_clocks, 
> ctx->dev->clocks);
> if (ret)
> goto err_cancel_job;
> -   ret = pm_runtime_get_sync(ctx->dev->dev);
> +
> +   ret = pm_runtime_resume_and_get(ctx->dev->dev);
> if (ret < 0)
> goto err_cancel_job;
>  
> @@ -165,7 +174,7 @@ static void device_run(void *priv)
> return;
>  
>  err_cancel_job:
> -   hantro_job_finish(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> +   hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
>  }
>  
>  static struct v4l2_m2m_ops vpu_m2m_ops = {


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 79/79] media: hantro: document the usage of pm_runtime_get_sync()

[Ezequiel Garcia]

Hi Mauro,

On Wed, 2021-04-28 at 08:44 +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 28 Apr 2021 08:27:42 +0200
> Mauro Carvalho Chehab  escreveu:
> 
> > Em Tue, 27 Apr 2021 12:18:32 -0300
> > Ezequiel Garcia  escreveu:
> > 
> > > On Tue, 2021-04-27 at 16:08 +0100, Robin Murphy wrote:  
> > > > On 2021-04-27 11:27, Mauro Carvalho Chehab wrote:    
> > > > > Despite other *_get()/*_put() functions, where usage count is
> > > > > incremented only if not errors, the pm_runtime_get_sync() has
> > > > > a different behavior, incrementing the counter *even* on
> > > > > errors.
> > > > > 
> > > > > That's an error prone behavior, as people often forget to
> > > > > decrement the usage counter.
> > > > > 
> > > > > However, the hantro driver depends on this behavior, as it
> > > > > will decrement the usage_count unconditionally at the m2m
> > > > > job finish time, which makes sense.
> > > > > 
> > > > > So, intead of using the pm_runtime_resume_and_get() that
> > > > > would decrement the counter on error, keep the current
> > > > > API, but add a documentation explaining the rationale for
> > > > > keep using pm_runtime_get_sync().
> > > > > 
> > > > > Signed-off-by: Mauro Carvalho Chehab 
> > > > > ---
> > > > >   drivers/staging/media/hantro/hantro_drv.c | 7 +++
> > > > >   1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c 
> > > > > b/drivers/staging/media/hantro/hantro_drv.c
> > > > > index 595e82a82728..96f940c1c85c 100644
> > > > > --- a/drivers/staging/media/hantro/hantro_drv.c
> > > > > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > > > > @@ -155,6 +155,13 @@ static void device_run(void *priv)
> > > > > ret = clk_bulk_enable(ctx->dev->variant->num_clocks, 
> > > > > ctx->dev->clocks);
> > > > > if (ret)
> > > > > goto err_cancel_job;    
> > > > 
> > > > ..except this can also cause the same pm_runtime_put_autosuspend() call 
> > > > without even reaching the "matching" get below, so rather than some 
> > > > kind 
> > > > of cleverness it seems more like it's just broken :/
> > > >     
> > > 
> > > Indeed, I was trying to find time to cook a quick patch, but kept
> > > getting preempted.
> > > 
> > > Feel free to submit a fix for this, otherwise, I'll try to find
> > > time later this week.  
> > 
> > What about doing this instead:
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c 
> > b/drivers/staging/media/hantro/hantro_drv.c
> > index 595e82a82728..67de6b15236d 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -56,14 +56,12 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 
> > ts)
> > return hantro_get_dec_buf_addr(ctx, buf);
> >  }
> >  
> > -static void hantro_job_finish(struct hantro_dev *vpu,
> > - struct hantro_ctx *ctx,
> > - enum vb2_buffer_state result)
> > +static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> > +   struct hantro_ctx *ctx,
> > +   enum vb2_buffer_state result)
> >  {
> > struct vb2_v4l2_buffer *src, *dst;
> >  
> > -   pm_runtime_mark_last_busy(vpu->dev);
> > -   pm_runtime_put_autosuspend(vpu->dev);
> > clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> >  
> > src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > @@ -81,6 +79,16 @@ static void hantro_job_finish(struct hantro_dev *vpu,
> >  result);
> >  }
> >  
> > +static void hantro_job_finish(struct hantro_dev *vpu,
> > + struct hantro_ctx *ctx,
> > + enum vb2_buffer_state result)
> > +{
> > +   pm_runtime_mark_last_busy(vpu->dev);
> > +   pm_runtime_put_autosuspend(vpu->dev);
> > +
> > +   hantro_job_finish_no_pm(vpu, ctx, result);
> > +}
> > +
> >  void hantro_irq_done(struct hantro_dev *vpu,
> >    

Re: [PATCH v3 79/79] media: hantro: document the usage of pm_runtime_get_sync()

[Ezequiel Garcia]

On Tue, 2021-04-27 at 16:08 +0100, Robin Murphy wrote:
> On 2021-04-27 11:27, Mauro Carvalho Chehab wrote:
> > Despite other *_get()/*_put() functions, where usage count is
> > incremented only if not errors, the pm_runtime_get_sync() has
> > a different behavior, incrementing the counter *even* on
> > errors.
> > 
> > That's an error prone behavior, as people often forget to
> > decrement the usage counter.
> > 
> > However, the hantro driver depends on this behavior, as it
> > will decrement the usage_count unconditionally at the m2m
> > job finish time, which makes sense.
> > 
> > So, intead of using the pm_runtime_resume_and_get() that
> > would decrement the counter on error, keep the current
> > API, but add a documentation explaining the rationale for
> > keep using pm_runtime_get_sync().
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> > ---
> >   drivers/staging/media/hantro/hantro_drv.c | 7 +++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c 
> > b/drivers/staging/media/hantro/hantro_drv.c
> > index 595e82a82728..96f940c1c85c 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -155,6 +155,13 @@ static void device_run(void *priv)
> > ret = clk_bulk_enable(ctx->dev->variant->num_clocks, 
> > ctx->dev->clocks);
> > if (ret)
> > goto err_cancel_job;
> 
> ..except this can also cause the same pm_runtime_put_autosuspend() call 
> without even reaching the "matching" get below, so rather than some kind 
> of cleverness it seems more like it's just broken :/
> 

Indeed, I was trying to find time to cook a quick patch, but kept
getting preempted.

Feel free to submit a fix for this, otherwise, I'll try to find
time later this week.

Thanks,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC RESEND 0/3] vp9 v4l2 stateless uapi

[Ezequiel Garcia]

On Mon, 26 Apr 2021 at 14:38, Nicolas Dufresne  wrote:
>
> Le lundi 26 avril 2021 à 09:38 +0200, Hans Verkuil a écrit :
> > Hi Andrzej,
> >
> > Thank you for working on this!
> >
> > On 21/04/2021 12:00, Andrzej Pietrasiewicz wrote:
> > > Dear All,
> > >
> > > This is an RFC on stateless uapi for vp9 decoding with v4l2. This work is 
> > > based on https://lkml.org/lkml/2020/11/2/1043, but has been substantially 
> > > reworked. The important change is that the v4l2 control used to pass 
> > > boolean decoder probabilities has been made unidirectional, and is now 
> > > called V4L2_CID_STATELESS_VP9_COMPRESSED_HDR_PROBS.
> > >
> > > In the previous proposal, to queue a frame the userspace must fully 
> > > dequeue the previous one, which effectively results in a forced lockstep 
> > > behavior and defeats vb2's capability to enqueue multiple buffers. Such a 
> > > design was a consequence of backward probability updates being performed 
> > > by the kernel driver (which has direct access to appropriate counter 
> > > values) but forward probability updates being coupled with compressed 
> > > header parsing performed by the userspace.
> > >
> > > In vp9 the boolean decoder used to decode the bitstream needs certain 
> > > parameters to work. Those are probabilities, which change with each 
> > > frame. After each frame is decoded it is known how many times a given 
> > > symbol occured in the frame, so the probabilities can be adapted. This 
> > > process is known as backward probabilities update. A next frame header 
> > > can also contain information which modifies probabilities resulting from 
> > > backward update. The said modification is called forward probabilities 
> > > update. The data for backward update is generated by the decoder 
> > > hardware, while the data for forward update is prepared by reading the 
> > > compressed frame header. The natural place to parse something is 
> > > userspace, while the natural place to access hardware-provided counters 
> > > is the kernel. Such responsibilties assignment was used in the original 
> > > work.
> > >
> > > To overcome the lockstep, we moved forward probability updates to the 
> > > kernel, while leaving parsing them in userspace. This way the v4l2 
> > > control which is used to pass the probs becomes unidirectional 
> > > (user->kernel) and the userspace can keep parsing and enqueueing 
> > > succeeding frames.
> > >
> > > If a particular driver parses the compressed header and does backward 
> > > probability updates on its own then 
> > > V4L2_CID_STATELESS_VP9_COMPRESSED_HDR_PROBS does not need to be used.
> > >
> > > This series adds vp9 uapi in proper locations, which means it is a 
> > > proper, "official" uapi, as opposed to staging uapi which was proposed in 
> > > the above mentioned lkml thread.
> >
> > Why? I rather liked the way that the other codec APIs started life in a 
> > private header
> > (like include/media/vp8-ctrls.h) and were given time to mature before 
> > moving them to
> > the uAPI. Is there a reason why you think that VP9 doesn't need that?
>
> I'll be honest, I accepted early code into GStreamer for H264, and it ended up
> in a nightmare for the users. We now have a released GStreamer that supports
> kernel API up to 5.9, a blackwhole at 5.10 and finally master catched up and 
> can
> support 5.11+. It is so complicated for packagers to understand what is going
> on, that they endup wasting a lot of their time for a single feature in their
> OS. Same breakage is happening for VP8 in 5.13, even though VP8 has been 
> working
> great all this time. I will for sure for now on ignore any contribution that
> depends on staged uAPI.
>
> As for FFMPEG, even though now H264 API is table, the maintainers just simply
> ignore the patches as they have been bitten by the reviewing stuff based on
> unstable APIs and downstream work.
>
> I believe the staged uAPI has been used wrongly in the past. Stuff has been
> staged quicky right before associated project budget for it was exhausted, so 
> it
> was in the end a way to look good, and someone else had to pick it up and 
> finish
> it. Going straight for final API put more pressure on making good research 
> from
> the start, doing more in-depth reviews and avoiding delaying for multiple 
> years
> the support. I believe the staging API are confusing even for the Linux
> projects. Going straight to stable here is a commitment to finish this work 
> and
> doing it correctly.
>
> This specially make sense for VP9, which is a very Open CODEC and were all HW
> implementation are Google/Hantro derivatives. Also, unlike when this work all
> started, we do have multiple HW we can look at to validate the API, with more
> then enough in-depth information to make the right decisions.
>

+1

Although I can understand how, from the kernel point of view, it's
tempting to merge
the uAPI as staging first and then de-stage it, I have to say that I
agree fully with
Nicolas, the experience 

Re: [PATCH 13/78] staging: media: hantro_drv: use pm_runtime_resume_and_get()

[Ezequiel Garcia]

Hi Mauro,

On Sat, 2021-04-24 at 08:44 +0200, Mauro Carvalho Chehab wrote:
> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with 
> usage counter")
> added pm_runtime_resume_and_get() in order to automatically handle
> dev->power.usage_count decrement on errors.
> 
> Use the new API, in order to cleanup the error check logic.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/staging/media/hantro/hantro_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c 
> b/drivers/staging/media/hantro/hantro_drv.c
> index 595e82a82728..3147dcbebeb9 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -155,7 +155,7 @@ static void device_run(void *priv)
> ret = clk_bulk_enable(ctx->dev->variant->num_clocks, 
> ctx->dev->clocks);
> if (ret)
> goto err_cancel_job;
> -   ret = pm_runtime_get_sync(ctx->dev->dev);
> +   ret = pm_runtime_resume_and_get(ctx->dev->dev);
> if (ret < 0)
> goto err_cancel_job;
>  

Seems this one needs a different fix: err_cancel_job
will call hantro_job_finish which has a pm_runtime put.

Thanks,
Ezequiel 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 11/78] staging: media: rkvdec: fix pm_runtime_get_sync() usage count

[Ezequiel Garcia]

On Sat, 2021-04-24 at 08:44 +0200, Mauro Carvalho Chehab wrote:
> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> replace it by the new pm_runtime_resume_and_get(), introduced by:
> commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with 
> usage counter")
> in order to properly decrement the usage counter and avoid memory
> leaks.
> 
> Signed-off-by: Mauro Carvalho Chehab 

Reviewed-by: Ezequiel Garcia 

Thanks a lot for taking care of this.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 09/13] media: uapi: Add a control for HANTRO driver

[Ezequiel Garcia]

Hi Benjamin,

Thanks for the patch.

On Thu, 2021-04-01 at 17:59 +0200, Benjamin Gaignard wrote:
> The HEVC HANTRO driver needs to know the number of bits to skip at
> the beginning of the slice header.
> That is a hardware specific requirement so create a dedicated control
> that this purpose.
> 
> Signed-off-by: Benjamin Gaignard 
> ---
> version 5:
>  - Be even more verbose in control documentation.
>  - Do not create class for the control.
> version 4:
> - The control is now an integer which is enough to provide the numbers
>   of bits to skip.
> version 3:
> - Fix typo in field name
> 
>  .../userspace-api/media/drivers/hantro.rst | 14 ++
>  .../userspace-api/media/drivers/index.rst  |  1 +
>  include/uapi/linux/v4l2-controls.h | 13 +
>  3 files changed, 28 insertions(+)
>  create mode 100644 Documentation/userspace-api/media/drivers/hantro.rst
> 
> diff --git a/Documentation/userspace-api/media/drivers/hantro.rst 
> b/Documentation/userspace-api/media/drivers/hantro.rst
> new file mode 100644
> index ..78dcd2a44a03
> --- /dev/null
> +++ b/Documentation/userspace-api/media/drivers/hantro.rst
> @@ -0,0 +1,14 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Hantro video decoder driver
> +===
> +
> +The Hantro video decoder driver implements the following driver-specific 
> controls:
> +
> +``V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP (integer)``
> +    Specifies to Hantro HEVC video decoder driver the number of data (in 
> bits) to
> +    skip in the slice segment header.
> +    If non-IDR, the bits to be skipped go from syntax element 
> "pic_output_flag"
> +    to before syntax element "slice_temporal_mvp_enabled_flag".
> +    If IDR, the skipped bits are just "pic_output_flag"
> +    (separate_colour_plane_flag is not supported).
> diff --git a/Documentation/userspace-api/media/drivers/index.rst 
> b/Documentation/userspace-api/media/drivers/index.rst
> index 1a9038f5f9fa..12e3c512d718 100644
> --- a/Documentation/userspace-api/media/drivers/index.rst
> +++ b/Documentation/userspace-api/media/drivers/index.rst
> @@ -33,6 +33,7 @@ For more details see the file COPYING in the source 
> distribution of Linux.
>  
> ccs
> cx2341x-uapi
> +    hantro
> imx-uapi
> max2175
> meye-uapi
> diff --git a/include/uapi/linux/v4l2-controls.h 
> b/include/uapi/linux/v4l2-controls.h
> index f3376aafea65..1dfb874b6272 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -869,6 +869,19 @@ enum v4l2_mpeg_mfc51_video_force_frame_type {
>  #define V4L2_CID_MPEG_MFC51_VIDEO_H264_ADAPTIVE_RC_STATIC  
> (V4L2_CID_CODEC_MFC51_BASE+53)
>  #define V4L2_CID_MPEG_MFC51_VIDEO_H264_NUM_REF_PIC_FOR_P   
> (V4L2_CID_CODEC_MFC51_BASE+54)
>  
> +/*  MPEG-class control IDs specific to the Hantro driver as defined by V4L2 
> */

We are moving away from "MPEG" terminology for codecs.

> +#define V4L2_CID_CODEC_HANTRO_BASE 
> (V4L2_CTRL_CLASS_CODEC | 0x1200)

Using V4L2_CTRL_CLASS_CODEC_STATELESS is IMO better,
since this belongs to a stateless decoder.

And also, since we are still a bit unsure about the
syntax of this parameter (given it's not documented):

how about keeping the V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP
definition in drivers/staging/media/hantro/hantro.h ?

This would be a hint for applications that this control
is a quirk.

Hans, Philipp, any thoughts on this?

Regards,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 05/13] media: hevc: Add decode params control

[Ezequiel Garcia]

On Mon, 2021-03-29 at 08:57 +0200, Benjamin Gaignard wrote:
> Add decode params control and it associated structure to regroup
> all the information that are needed to decode a reference frame as
> it is describe in ITU-T Rec. H.265 section "8.3.2 Decoding process
> for reference picture set".
> 
> Adapt Cedrus driver to these changes.
> 
> Signed-off-by: Benjamin Gaignard 

Reviewed-by: Ezequiel Garcia 

> ---
> version 7:
>  - rebased on top of media_tree/master branch
> 
> version 6:
>  - fix compilation errors
> 
>  .../media/v4l/ext-ctrls-codec.rst | 94 +++
>  .../media/v4l/vidioc-queryctrl.rst    |  6 ++
>  drivers/media/v4l2-core/v4l2-ctrls.c  | 26 +++--
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |  6 ++
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  1 +
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
>  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 12 ++-
>  include/media/hevc-ctrls.h    | 29 --
>  8 files changed, 137 insertions(+), 39 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst 
> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 92314aec655a..7552869687f7 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -3181,9 +3181,6 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>  * - __u8
>    - ``pic_struct``
>    -
> -    * - __u8
> -  - ``num_active_dpb_entries``
> -  - The number of entries in ``dpb``.
>  * - __u8
>    - ``ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>    - The list of L0 reference elements as indices in the DPB.
> @@ -3191,22 +3188,8 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>    - ``ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>    - The list of L1 reference elements as indices in the DPB.
>  * - __u8
> -  - ``num_rps_poc_st_curr_before``
> -  - The number of reference pictures in the short-term set that come 
> before
> -    the current frame.
> -    * - __u8
> -  - ``num_rps_poc_st_curr_after``
> -  - The number of reference pictures in the short-term set that come 
> after
> -    the current frame.
> -    * - __u8
> -  - ``num_rps_poc_lt_curr``
> -  - The number of reference pictures in the long-term set.
> -    * - __u8
> -  - ``padding[7]``
> +  - ``padding``
>    - Applications and drivers must set this to zero.
> -    * - struct :c:type:`v4l2_hevc_dpb_entry`
> -  - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> -  - The decoded picture buffer, for meta-data about reference frames.
>  * - struct :c:type:`v4l2_hevc_pred_weight_table`
>    - ``pred_weight_table``
>    - The prediction weight coefficients for inter-picture prediction.
> @@ -3441,3 +3424,78 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>  so this has to come from client.
>  This is applicable to H264 and valid Range is from 0 to 63.
>  Source Rec. ITU-T H.264 (06/2019); G.7.4.1.1, G.8.8.1.
> +
> +``V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS (struct)``
> +    Specifies various decode parameters, especially the references picture 
> order
> +    count (POC) for all the lists (short, long, before, current, after) and 
> the
> +    number of entries for each of them.
> +    These parameters are defined according to :ref:`hevc`.
> +    They are described in section 8.3 "Slice decoding process" of the
> +    specification.
> +
> +.. c:type:: v4l2_ctrl_hevc_decode_params
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: struct v4l2_ctrl_hevc_decode_params
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:   1 1 2
> +
> +    * - __s32
> +  - ``pic_order_cnt_val``
> +  - PicOrderCntVal as described in section 8.3.1 "Decoding process
> +    for picture order count" of the specification.
> +    * - __u8
> +  - ``num_active_dpb_entries``
> +  - The number of entries in ``dpb``.
> +    * - struct :c:type:`v4l2_hevc_dpb_entry`
> +  - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> +  - The decoded picture buffer, for meta-data about reference frames.
> +    * - __u8
> +  - ``num_poc_st_curr_before``
> +  - The number of reference pictures in the short-term set that come 
> before
> +    the current frame.
> +    * - __u8
> +  - ``num_poc_st_curr_after``
> +  - The number of reference pictures in the short-term set that come 
> after
> +    the current frame.
> +    * - __u8
> +  - ``num_poc_lt_curr``
> +  - The number of refe

Re: [PATCH v7 12/13] media: hantro: IMX8M: add variant for G2/HEVC codec

[Ezequiel Garcia]

On Mon, 2021-03-29 at 08:57 +0200, Benjamin Gaignard wrote:
> Add variant to IMX8M to enable G2/HEVC codec.
> Define the capabilities for the hardware up to 3840x2160.
> G2 doesn't have postprocessor, use the same clocks and got it
> own interruption.
> 
> Signed-off-by: Benjamin Gaignard 
> Reviewed-by: Philipp Zabel 

Reviewed-by: Ezequiel Garcia 

> ---
> version 7:
>  - Add Philipp Reviewed-by tag.
> 
> version 5:
>  - remove useless postproc fields for G2
> 
> version 2:
> - remove useless clocks
> 
>  drivers/staging/media/hantro/hantro_drv.c   |  1 +
>  drivers/staging/media/hantro/hantro_hw.h    |  1 +
>  drivers/staging/media/hantro/imx8m_vpu_hw.c | 76 -
>  3 files changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c 
> b/drivers/staging/media/hantro/hantro_drv.c
> index 33b8bd38eac1..ed380a8bef93 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -574,6 +574,7 @@ static const struct of_device_id of_hantro_match[] = {
>  #endif
>  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
> { .compatible = "nxp,imx8mq-vpu", .data = _vpu_variant, },
> +   { .compatible = "nxp,imx8mq-vpu-g2", .data = _vpu_g2_variant },
>  #endif
> { /* sentinel */ }
>  };
> diff --git a/drivers/staging/media/hantro/hantro_hw.h 
> b/drivers/staging/media/hantro/hantro_hw.h
> index 5788188aae50..b4e7490bbe45 100644
> --- a/drivers/staging/media/hantro/hantro_hw.h
> +++ b/drivers/staging/media/hantro/hantro_hw.h
> @@ -193,6 +193,7 @@ extern const struct hantro_variant rk3399_vpu_variant;
>  extern const struct hantro_variant rk3328_vpu_variant;
>  extern const struct hantro_variant rk3288_vpu_variant;
>  extern const struct hantro_variant imx8mq_vpu_variant;
> +extern const struct hantro_variant imx8mq_vpu_g2_variant;
>  
>  extern const struct hantro_postproc_regs hantro_g1_postproc_regs;
>  
> diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c 
> b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> index 8d0c3425234b..6de43e0edc36 100644
> --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> @@ -12,6 +12,7 @@
>  #include "hantro.h"
>  #include "hantro_jpeg.h"
>  #include "hantro_g1_regs.h"
> +#include "hantro_g2_regs.h"
>  
>  #define CTRL_SOFT_RESET0x00
>  #define RESET_G1   BIT(1)
> @@ -129,6 +130,26 @@ static const struct hantro_fmt imx8m_vpu_dec_fmts[] = {
> },
>  };
>  
> +static const struct hantro_fmt imx8m_vpu_g2_dec_fmts[] = {
> +   {
> +   .fourcc = V4L2_PIX_FMT_NV12,
> +   .codec_mode = HANTRO_MODE_NONE,
> +   },
> +   {
> +   .fourcc = V4L2_PIX_FMT_HEVC_SLICE,
> +   .codec_mode = HANTRO_MODE_HEVC_DEC,
> +   .max_depth = 2,
> +   .frmsize = {
> +   .min_width = 48,
> +   .max_width = 3840,
> +   .step_width = MB_DIM,
> +   .min_height = 48,
> +   .max_height = 2160,
> +   .step_height = MB_DIM,
> +   },
> +   },
> +};
> +
>  static irqreturn_t imx8m_vpu_g1_irq(int irq, void *dev_id)
>  {
> struct hantro_dev *vpu = dev_id;
> @@ -147,6 +168,24 @@ static irqreturn_t imx8m_vpu_g1_irq(int irq, void 
> *dev_id)
> return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id)
> +{
> +   struct hantro_dev *vpu = dev_id;
> +   enum vb2_buffer_state state;
> +   u32 status;
> +
> +   status = vdpu_read(vpu, HEVC_REG_INTERRUPT);
> +   state = (status & HEVC_REG_INTERRUPT_DEC_RDY_INT) ?
> +    VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
> +
> +   vdpu_write(vpu, 0, HEVC_REG_INTERRUPT);
> +   vdpu_write(vpu, HEVC_REG_CONFIG_DEC_CLK_GATE_E, HEVC_REG_CONFIG);
> +
> +   hantro_irq_done(vpu, state);
> +
> +   return IRQ_HANDLED;
> +}
> +
>  static int imx8mq_vpu_hw_init(struct hantro_dev *vpu)
>  {
> struct device_node *np = vpu->dev->of_node;
> @@ -176,6 +215,13 @@ static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx)
> imx8m_soft_reset(vpu, RESET_G1);
>  }
>  
> +static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> +{
> +   struct hantro_dev *vpu = ctx->dev;
> +
> +   imx8m_soft_reset(vpu, RESET_G2);
> +}
> +
>  /*
>   * Supported codec ops.
>   */
> @@ -201,16 +247,28 @@ static const struct hantro_codec_ops 
> imx8mq_vpu_co

Re: [PATCH v7 10/13] media: hantro: handle V4L2_PIX_FMT_HEVC_SLICE control

[Ezequiel Garcia]

On Mon, 2021-03-29 at 08:57 +0200, Benjamin Gaignard wrote:
> Make sure that V4L2_PIX_FMT_HEVC_SLICE is correctly handle by v4l2
> of the driver.
> 
> Signed-off-by: Benjamin Gaignard 

Reviewed-by: Ezequiel Garcia 

> ---
>  drivers/staging/media/hantro/hantro_v4l2.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c 
> b/drivers/staging/media/hantro/hantro_v4l2.c
> index 77d7fe62ce81..0655324fd0d4 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -392,6 +392,7 @@ hantro_update_requires_request(struct hantro_ctx *ctx, 
> u32 fourcc)
> case V4L2_PIX_FMT_MPEG2_SLICE:
> case V4L2_PIX_FMT_VP8_FRAME:
> case V4L2_PIX_FMT_H264_SLICE:
> +   case V4L2_PIX_FMT_HEVC_SLICE:
> ctx->fh.m2m_ctx->out_q_ctx.q.requires_requests = true;
> break;
> default:


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 08/13] media: hantro: Only use postproc when post processed formats are defined

[Ezequiel Garcia]

On Mon, 2021-03-29 at 08:57 +0200, Benjamin Gaignard wrote:
> If the variant doesn't offert postprocessed formats make sure it will
> be ok.
> 
> Signed-off-by: Benjamin Gaignard 

Reviewed-by: Ezequiel Garcia 

> ---
>  drivers/staging/media/hantro/hantro.h  |  8 ++--
>  drivers/staging/media/hantro/hantro_postproc.c | 14 ++
>  drivers/staging/media/hantro/hantro_v4l2.c |  4 +++-
>  3 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h 
> b/drivers/staging/media/hantro/hantro.h
> index edb4561a6887..7a5ad93466c8 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -414,12 +414,8 @@ hantro_get_dst_buf(struct hantro_ctx *ctx)
> return v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>  }
>  
> -static inline bool
> -hantro_needs_postproc(const struct hantro_ctx *ctx,
> - const struct hantro_fmt *fmt)
> -{
> -   return !ctx->is_encoder && fmt->fourcc != V4L2_PIX_FMT_NV12;
> -}
> +bool hantro_needs_postproc(const struct hantro_ctx *ctx,
> +  const struct hantro_fmt *fmt);
>  
>  static inline dma_addr_t
>  hantro_get_dec_buf_addr(struct hantro_ctx *ctx, struct vb2_buffer *vb)
> diff --git a/drivers/staging/media/hantro/hantro_postproc.c 
> b/drivers/staging/media/hantro/hantro_postproc.c
> index 6d2a8f2a8f0b..ed8916c950a4 100644
> --- a/drivers/staging/media/hantro/hantro_postproc.c
> +++ b/drivers/staging/media/hantro/hantro_postproc.c
> @@ -50,6 +50,20 @@ const struct hantro_postproc_regs hantro_g1_postproc_regs 
> = {
> .display_width = {G1_REG_PP_DISPLAY_WIDTH, 0, 0xfff},
>  };
>  
> +bool hantro_needs_postproc(const struct hantro_ctx *ctx,
> +  const struct hantro_fmt *fmt)
> +{
> +   struct hantro_dev *vpu = ctx->dev;
> +
> +   if (ctx->is_encoder)
> +   return false;
> +
> +   if (!vpu->variant->postproc_fmts)
> +   return false;
> +
> +   return fmt->fourcc != V4L2_PIX_FMT_NV12;
> +}
> +
>  void hantro_postproc_enable(struct hantro_ctx *ctx)
>  {
> struct hantro_dev *vpu = ctx->dev;
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c 
> b/drivers/staging/media/hantro/hantro_v4l2.c
> index 1bc118e375a1..77d7fe62ce81 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -55,7 +55,9 @@ static const struct hantro_fmt *
>  hantro_get_postproc_formats(const struct hantro_ctx *ctx,
>     unsigned int *num_fmts)
>  {
> -   if (ctx->is_encoder) {
> +   struct hantro_dev *vpu = ctx->dev;
> +
> +   if (ctx->is_encoder || !vpu->variant->postproc_fmts) {
> *num_fmts = 0;
> return NULL;
> }


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 07/13] media: hantro: Define HEVC codec profiles and supported features

[Ezequiel Garcia]

On Mon, 2021-03-29 at 08:57 +0200, Benjamin Gaignard wrote:
> Define which HEVC profiles (up to level 5.1) and features
> (no scaling, no 10 bits) are supported by the driver.
> 
> Signed-off-by: Benjamin Gaignard 

Reviewed-by: Ezequiel Garcia 

> ---
>  drivers/staging/media/hantro/hantro.h |  3 ++
>  drivers/staging/media/hantro/hantro_drv.c | 58 +++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h 
> b/drivers/staging/media/hantro/hantro.h
> index 37b9ce04bd4e..edb4561a6887 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -35,6 +35,7 @@ struct hantro_codec_ops;
>  #define HANTRO_MPEG2_DECODER   BIT(16)
>  #define HANTRO_VP8_DECODER BIT(17)
>  #define HANTRO_H264_DECODERBIT(18)
> +#define HANTRO_HEVC_DECODERBIT(19)
>  #define HANTRO_DECODERS0x
>  
>  /**
> @@ -100,6 +101,7 @@ struct hantro_variant {
>   * @HANTRO_MODE_H264_DEC: H264 decoder.
>   * @HANTRO_MODE_MPEG2_DEC: MPEG-2 decoder.
>   * @HANTRO_MODE_VP8_DEC: VP8 decoder.
> + * @HANTRO_MODE_HEVC_DEC: HEVC decoder.
>   */
>  enum hantro_codec_mode {
> HANTRO_MODE_NONE = -1,
> @@ -107,6 +109,7 @@ enum hantro_codec_mode {
> HANTRO_MODE_H264_DEC,
> HANTRO_MODE_MPEG2_DEC,
> HANTRO_MODE_VP8_DEC,
> +   HANTRO_MODE_HEVC_DEC,
>  };
>  
>  /*
> diff --git a/drivers/staging/media/hantro/hantro_drv.c 
> b/drivers/staging/media/hantro/hantro_drv.c
> index 02c5c2f1a88b..d9a3a5ef9330 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -245,6 +245,18 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> if (sps->bit_depth_luma_minus8 != 0)
> /* Only 8-bit is supported */
> return -EINVAL;
> +   } else if (ctrl->id == V4L2_CID_MPEG_VIDEO_HEVC_SPS) {
> +   const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_sps;
> +
> +   if (sps->bit_depth_luma_minus8 != 
> sps->bit_depth_chroma_minus8)
> +   /* Luma and chroma bit depth mismatch */
> +   return -EINVAL;
> +   if (sps->bit_depth_luma_minus8 != 0)
> +   /* Only 8-bit is supported */
> +   return -EINVAL;
> +   if (sps->flags & V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED)
> +   /* No scaling support */
> +   return -EINVAL;
> }
> return 0;
>  }
> @@ -351,6 +363,52 @@ static const struct hantro_ctrl controls[] = {
> .def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
> }
> }, {
> +   .codec = HANTRO_HEVC_DECODER,
> +   .cfg = {
> +   .id = V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE,
> +   .min = V4L2_MPEG_VIDEO_HEVC_DECODE_MODE_FRAME_BASED,
> +   .max = V4L2_MPEG_VIDEO_HEVC_DECODE_MODE_FRAME_BASED,
> +   .def = V4L2_MPEG_VIDEO_HEVC_DECODE_MODE_FRAME_BASED,
> +   },
> +   }, {
> +   .codec = HANTRO_HEVC_DECODER,
> +   .cfg = {
> +   .id = V4L2_CID_MPEG_VIDEO_HEVC_START_CODE,
> +   .min = V4L2_MPEG_VIDEO_HEVC_START_CODE_ANNEX_B,
> +   .max = V4L2_MPEG_VIDEO_HEVC_START_CODE_ANNEX_B,
> +   .def = V4L2_MPEG_VIDEO_HEVC_START_CODE_ANNEX_B,
> +   },
> +   }, {
> +   .codec = HANTRO_HEVC_DECODER,
> +   .cfg = {
> +   .id = V4L2_CID_MPEG_VIDEO_HEVC_PROFILE,
> +   .min = V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN,
> +   .max = V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN_10,
> +   .def = V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN,
> +   },
> +   }, {
> +   .codec = HANTRO_HEVC_DECODER,
> +   .cfg = {
> +   .id = V4L2_CID_MPEG_VIDEO_HEVC_LEVEL,
> +   .min = V4L2_MPEG_VIDEO_HEVC_LEVEL_1,
> +   .max = V4L2_MPEG_VIDEO_HEVC_LEVEL_5_1,
> +   },
> +   }, {
> +   .codec = HANTRO_HEVC_DECODER,
> +   .cfg = {
> +   .id = V4L2_CID_MPEG_VIDEO_HEVC_SPS,
> +   .ops = _ctrl_ops,
> +   },
> +   }, {
> +   .codec = HANTRO_HEVC_DECODER,
> +   .cfg = {
> +   .id = V4L2_CID_MPEG_VIDEO_HEVC_PPS,
> +   },
> + 

Re: [PATCH v7 06/13] media: hantro: change hantro_codec_ops run prototype to return errors

[Ezequiel Garcia]

On Mon, 2021-03-29 at 08:57 +0200, Benjamin Gaignard wrote:
> Change hantro_codec_ops run prototype from 'void' to 'int'.
> This allow to cancel the job if an error occur while configuring
> the hardware.
> 
> Signed-off-by: Benjamin Gaignard 

Reviewed-by: Ezequiel Garcia 

> ---
> version 5:
>  - forward hantro_h264_dec_prepare_run() return value in case
>    of error
> 
>  drivers/staging/media/hantro/hantro_drv.c |  4 +++-
>  .../staging/media/hantro/hantro_g1_h264_dec.c | 10 +++---
>  .../media/hantro/hantro_g1_mpeg2_dec.c    |  4 +++-
>  .../staging/media/hantro/hantro_g1_vp8_dec.c  |  6 --
>  .../staging/media/hantro/hantro_h1_jpeg_enc.c |  4 +++-
>  drivers/staging/media/hantro/hantro_hw.h  | 19 ++-
>  .../media/hantro/rk3399_vpu_hw_jpeg_enc.c |  4 +++-
>  .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c    |  4 +++-
>  .../media/hantro/rk3399_vpu_hw_vp8_dec.c  |  6 --
>  9 files changed, 40 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c 
> b/drivers/staging/media/hantro/hantro_drv.c
> index 595e82a82728..02c5c2f1a88b 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -161,7 +161,9 @@ static void device_run(void *priv)
>  
> v4l2_m2m_buf_copy_metadata(src, dst, true);
>  
> -   ctx->codec_ops->run(ctx);
> +   if (ctx->codec_ops->run(ctx))
> +   goto err_cancel_job;
> +
> return;
>  
>  err_cancel_job:
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c 
> b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index 845bef73d218..5c792b7bcb79 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -273,13 +273,15 @@ static void set_buffers(struct hantro_ctx *ctx)
> vdpu_write_relaxed(vpu, ctx->h264_dec.priv.dma, G1_REG_ADDR_QTABLE);
>  }
>  
> -void hantro_g1_h264_dec_run(struct hantro_ctx *ctx)
> +int hantro_g1_h264_dec_run(struct hantro_ctx *ctx)
>  {
> struct hantro_dev *vpu = ctx->dev;
> +   int ret;
>  
> /* Prepare the H264 decoder context. */
> -   if (hantro_h264_dec_prepare_run(ctx))
> -   return;
> +   ret = hantro_h264_dec_prepare_run(ctx);
> +   if (ret)
> +   return ret;
>  
> /* Configure hardware registers. */
> set_params(ctx);
> @@ -301,4 +303,6 @@ void hantro_g1_h264_dec_run(struct hantro_ctx *ctx)
>    G1_REG_CONFIG_DEC_CLK_GATE_E,
>    G1_REG_CONFIG);
> vdpu_write(vpu, G1_REG_INTERRUPT_DEC_E, G1_REG_INTERRUPT);
> +
> +   return 0;
>  }
> diff --git a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c 
> b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
> index 6386a3989bfe..5e8943d31dc5 100644
> --- a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
> @@ -155,7 +155,7 @@ hantro_g1_mpeg2_dec_set_buffers(struct hantro_dev *vpu, 
> struct hantro_ctx *ctx,
> vdpu_write_relaxed(vpu, backward_addr, G1_REG_REFER3_BASE);
>  }
>  
> -void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
> +int hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
>  {
> struct hantro_dev *vpu = ctx->dev;
> struct vb2_v4l2_buffer *src_buf, *dst_buf;
> @@ -248,4 +248,6 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
>  
> reg = G1_REG_DEC_E(1);
> vdpu_write(vpu, reg, G1_SWREG(1));
> +
> +   return 0;
>  }
> diff --git a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c 
> b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c
> index 57002ba70176..96622a7f8279 100644
> --- a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c
> @@ -425,7 +425,7 @@ static void cfg_buffers(struct hantro_ctx *ctx,
> vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST);
>  }
>  
> -void hantro_g1_vp8_dec_run(struct hantro_ctx *ctx)
> +int hantro_g1_vp8_dec_run(struct hantro_ctx *ctx)
>  {
> const struct v4l2_ctrl_vp8_frame *hdr;
> struct hantro_dev *vpu = ctx->dev;
> @@ -438,7 +438,7 @@ void hantro_g1_vp8_dec_run(struct hantro_ctx *ctx)
>  
> hdr = hantro_get_ctrl(ctx, V4L2_CID_STATELESS_VP8_FRAME);
> if (WARN_ON(!hdr))
> -   return;
> +   return -EINVAL;
>  
> /* Reset segment_map buffer in keyframe */
> if (V4L2_VP8_FRAME_IS_KEY_FRAME(hdr) && ctx->vp8_dec.segment_map.cpu)
> @@ -498,4 +498,6 @@ void hantro_g

Re: [PATCH v7 04/13] media: hevc: Add fields and flags for hevc PPS

[Ezequiel Garcia]

On Mon, 2021-03-29 at 08:57 +0200, Benjamin Gaignard wrote:
> Add fields and flags as they are defined in
> 7.4.3.3.1 "General picture parameter set RBSP semantics of the
> H.265 ITU specification.
> 
> Signed-off-by: Benjamin Gaignard 

Reviewed-by: Ezequiel Garcia 

> ---
>  .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 14 ++
>  include/media/hevc-ctrls.h |  4 
>  2 files changed, 18 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst 
> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 188aef8e40d0..92314aec655a 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -2967,6 +2967,12 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>  * - __u8
>    - ``num_extra_slice_header_bits``
>    -
> +    * - __u8
> +  - ``num_ref_idx_l0_default_active_minus1``
> +  - Specifies the inferred value of num_ref_idx_l0_active_minus1
> +    * - __u8
> +  - ``num_ref_idx_l1_default_active_minus1``
> +  - Specifies the inferred value of num_ref_idx_l1_active_minus1
>  * - __s8
>    - ``init_qp_minus26``
>    -
> @@ -3077,6 +3083,14 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>  * - ``V4L2_HEVC_PPS_FLAG_SLICE_SEGMENT_HEADER_EXTENSION_PRESENT``
>    - 0x0004
>    -
> +    * - ``V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT``
> +  - 0x0008
> +  - Specifies the presence of deblocking filter control syntax elements 
> in
> +    the PPS
> +    * - ``V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING``
> +  - 0x0010
> +  - Specifies that tile column boundaries and likewise tile row 
> boundaries
> +    are distributed uniformly across the picture
>  
>  .. raw:: latex
>  
> diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
> index b4cb2ef02f17..003f819ecb26 100644
> --- a/include/media/hevc-ctrls.h
> +++ b/include/media/hevc-ctrls.h
> @@ -100,10 +100,14 @@ struct v4l2_ctrl_hevc_sps {
>  #define V4L2_HEVC_PPS_FLAG_PPS_DISABLE_DEBLOCKING_FILTER   (1ULL << 16)
>  #define V4L2_HEVC_PPS_FLAG_LISTS_MODIFICATION_PRESENT  (1ULL << 17)
>  #define V4L2_HEVC_PPS_FLAG_SLICE_SEGMENT_HEADER_EXTENSION_PRESENT (1ULL << 
> 18)
> +#define V4L2_HEVC_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT   (1ULL << 19)
> +#define V4L2_HEVC_PPS_FLAG_UNIFORM_SPACING (1ULL << 20)
>  
>  struct v4l2_ctrl_hevc_pps {
> /* ISO/IEC 23008-2, ITU-T Rec. H.265: Picture parameter set */
> __u8num_extra_slice_header_bits;
> +   __u8num_ref_idx_l0_default_active_minus1;
> +   __u8num_ref_idx_l1_default_active_minus1;
> __s8init_qp_minus26;
> __u8diff_cu_qp_delta_depth;
> __s8pps_cb_qp_offset;


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 13/13] arm64: dts: imx8mq: Add node to G2 hardware

[Ezequiel Garcia]

On Fri, 2021-03-26 at 15:33 +0100, Benjamin Gaignard wrote:
> 
> Le 26/03/2021 à 15:24, Philipp Zabel a écrit :
> > On Thu, Mar 18, 2021 at 09:20:46AM +0100, Benjamin Gaignard wrote:
> > > Split VPU node in two: one for G1 and one for G2 since they are
> > > different hardware blocks.
> > > Add syscon for hardware control block.
> > > Remove reg-names property that is useless.
> > > Each VPU node only need one interrupt.
> > > 
> > > Signed-off-by: Benjamin Gaignard 
> > > ---
> > > version 5:
> > >   - use syscon instead of VPU reset
> > > 
> > >   arch/arm64/boot/dts/freescale/imx8mq.dtsi | 43 ++-
> > >   1 file changed, 34 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi 
> > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > > index 17c449e12c2e..b537d153ebbd 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > > @@ -1329,15 +1329,16 @@ usb3_phy1: usb-phy@382f0040 {
> > > status = "disabled";
> > > };
> > >   
> > > -   vpu: video-codec@3830 {
> > > +   vpu_ctrl: syscon@3832 {
> > > +   compatible = "nxp,imx8mq-vpu-ctrl", "syscon";
> > > +   reg = <0x3832 0x1>;
> > > +   };
> > > +
> > > +   vpu_g1: video-codec@3830 {
> > > compatible = "nxp,imx8mq-vpu";
> > > -   reg = <0x3830 0x1>,
> > > - <0x3831 0x1>,
> > > - <0x3832 0x1>;
> > > -   reg-names = "g1", "g2", "ctrl";
> > > -   interrupts = ,
> > > -    ;
> > > -   interrupt-names = "g1", "g2";
> > > +   reg = <0x3830 0x1>;
> > > +   interrupts = ;
> > > +   interrupt-names = "g1";
> > > clocks = < IMX8MQ_CLK_VPU_G1_ROOT>,
> > >  < IMX8MQ_CLK_VPU_G2_ROOT>,
> > >  < IMX8MQ_CLK_VPU_DEC_ROOT>;
> > > @@ -1350,9 +1351,33 @@ vpu: video-codec@3830 {
> > >  < 
> > > IMX8MQ_VPU_PLL_OUT>,
> > >  < 
> > > IMX8MQ_SYS1_PLL_800M>,
> > >  < IMX8MQ_VPU_PLL>;
> > > -   assigned-clock-rates = <6>, <6>,
> > > +   assigned-clock-rates = <6>, <3>,
> > I'd like to see this mentioned in the commit message.
> 
> Yes I would do that.
> The value comes from the datasheet.
> 
> > 
> > > +  <8>, <0>;
> > > +   power-domains = <_vpu>;
> > > +   nxp,imx8mq-vpu-ctrl = <_ctrl>;
> > > +   };
> > > +
> > > +   vpu_g2: video-codec@3831 {
> > > +   compatible = "nxp,imx8mq-vpu-g2";
> > > +   reg = <0x3831 0x1>;
> > > +   interrupts = ;
> > > +   interrupt-names = "g2";
> > > +   clocks = < IMX8MQ_CLK_VPU_G1_ROOT>,
> > > +    < IMX8MQ_CLK_VPU_G2_ROOT>,
> > > +    < IMX8MQ_CLK_VPU_DEC_ROOT>;
> > > +   clock-names = "g1", "g2",  "bus";
> > > +   assigned-clocks = < IMX8MQ_CLK_VPU_G1>,
> > Can the G1 clock configuration be dropped from the G2 device node and
> > the G2 clock configuration from the G1 device node? It looks weird that
> > these devices configure each other's clocks.
> 
> No because if only one device node is enabled we need to configure the both
> clocks anyway.
> 

Since this is akward, how about adding a comment here in the dtsi to clarify it?

Thanks,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: hantro: Auto generate the AXI ID to avoid conflicts

[Ezequiel Garcia]

Hi Enric,

Thanks a lot for the patch.

On Thu, 25 Feb 2021 at 09:08, Enric Balletbo i Serra
 wrote:
>
> The AXI ID is an AXI bus configuration for improve bus performance. If
> read and write operations use different ID the operations can be
> paralleled, whereas when they have the same ID the operations will be
> serialized. Right now, the write ID is fixed to 0 but we can set it to
> 0xff to get auto generated ID to avoid possible conflicts.
>
> This change has no functional changes, but seems reasonable to let the
> hardware to autogenerate the ID instead of hardcoding in software.
>
> Signed-off-by: Enric Balletbo i Serra 
> ---
>
>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c 
> b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index 845bef73d218..090088cd98ea 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -30,7 +30,7 @@ static void set_params(struct hantro_ctx *ctx)
> u32 reg;
>
> /* Decoder control register 0. */
> -   reg = G1_REG_DEC_CTRL0_DEC_AXI_WR_ID(0x0);
> +   reg = G1_REG_DEC_CTRL0_DEC_AXI_WR_ID(0xff);

Can we define a macro to avoid this magic number,
and add some comments explaining what's 0xff for?

Given this is AXI configuration, I'd expect it's CODEC-agnostic.
Maybe we could move CODEC-agnostic path to avoid duplicating the code?

Does this change apply to the rkvdec driver?

Thanks,
Ezequiel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 13/18] media: hantro: Introduce G2/HEVC decoder

[Ezequiel Garcia]

Hi Benjamin,

Before I review the implementation in detail,
there's one thing that looks suspicious.

On Wed, 2021-02-17 at 09:03 +0100, Benjamin Gaignard wrote:
> Implement all the logic to get G2 hardware decoding HEVC frames.
> It support up level 5.1 HEVC stream.
> It doesn't support yet 10 bits formats or scaling feature.
> 
> Add HANTRO HEVC dedicated control to skip some bits at the beginning
> of the slice header. That is very specific to this hardware so can't
> go into uapi structures. Compute the needed value is complex and require
> information from the stream that only the userland knows so let it
> provide the correct value to the driver.
> 
> Signed-off-by: Benjamin Gaignard 
> Signed-off-by: Ezequiel Garcia 
> Signed-off-by: Adrian Ratiu 
> ---
>  drivers/staging/media/hantro/Makefile |   2 +
>  drivers/staging/media/hantro/hantro_drv.c |  41 ++
>  .../staging/media/hantro/hantro_g2_hevc_dec.c | 637 ++
>  drivers/staging/media/hantro/hantro_g2_regs.h | 198 ++
>  drivers/staging/media/hantro/hantro_hevc.c    | 274 
>  drivers/staging/media/hantro/hantro_hw.h  |  14 +
>  6 files changed, 1166 insertions(+)
>  create mode 100644 drivers/staging/media/hantro/hantro_g2_hevc_dec.c
>  create mode 100644 drivers/staging/media/hantro/hantro_g2_regs.h
>  create mode 100644 drivers/staging/media/hantro/hantro_hevc.c
> 
> diff --git a/drivers/staging/media/hantro/Makefile 
> b/drivers/staging/media/hantro/Makefile
> index 743ce08eb184..0357f1772267 100644
> --- a/drivers/staging/media/hantro/Makefile
> +++ b/drivers/staging/media/hantro/Makefile
> @@ -9,12 +9,14 @@ hantro-vpu-y += \
> hantro_h1_jpeg_enc.o \
> hantro_g1_h264_dec.o \
> hantro_g1_mpeg2_dec.o \
> +   hantro_g2_hevc_dec.o \
> hantro_g1_vp8_dec.o \
> rk3399_vpu_hw_jpeg_enc.o \
> rk3399_vpu_hw_mpeg2_dec.o \
> rk3399_vpu_hw_vp8_dec.o \
> hantro_jpeg.o \
> hantro_h264.o \
> +   hantro_hevc.o \
> hantro_mpeg2.o \
> hantro_vp8.o
>  
> diff --git a/drivers/staging/media/hantro/hantro_drv.c 
> b/drivers/staging/media/hantro/hantro_drv.c
> index e1443c394f62..d171fb80876a 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -280,6 +280,20 @@ static int hantro_jpeg_s_ctrl(struct v4l2_ctrl *ctrl)
> return 0;
>  }
>  
> +static int hantro_extra_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +   const struct hantro_hevc_extra_decode_params *extra_params;
> +   struct hantro_ctx *ctx;
> +
> +   ctx = container_of(ctrl->handler,
> +  struct hantro_ctx, ctrl_handler);
> +   extra_params = >hevc_dec.ctrls.extra_params;
> +
> +   memcpy((void *)extra_params, ctrl->p_new.p_u8, sizeof(extra_params));
> +
> +   return 0;
> +}
> +
>  static const struct v4l2_ctrl_ops hantro_ctrl_ops = {
> .try_ctrl = hantro_try_ctrl,
>  };
> @@ -288,6 +302,10 @@ static const struct v4l2_ctrl_ops hantro_jpeg_ctrl_ops = 
> {
> .s_ctrl = hantro_jpeg_s_ctrl,
>  };
>  
> +static const struct v4l2_ctrl_ops hantro_extra_ctrl_ops = {
> +   .s_ctrl = hantro_extra_s_ctrl,
> +};
> +
>  static const struct hantro_ctrl controls[] = {
> {
> .codec = HANTRO_JPEG_ENCODER,
> @@ -413,6 +431,29 @@ static const struct hantro_ctrl controls[] = {
> .cfg = {
> .id = V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS,
> },
> +   }, {
> +   .codec = HANTRO_HEVC_DECODER,
> +   .cfg = {
> +   .id = V4L2_CID_HANTRO_HEVC_EXTRA_DECODE_PARAMS,
> +   .name = "HANTRO extra decode params",
> +   .type = V4L2_CTRL_TYPE_U8,
> +   .min = 0,
> +   .def = 0,
> +   .max = 255,
> +   .step = 1,
> +   .dims = { sizeof(struct 
> hantro_hevc_extra_decode_params) },
> +   .ops = _extra_ctrl_ops,
> +   },
> +   }, {
> +   .codec = HANTRO_JPEG_ENCODER | HANTRO_MPEG2_DECODER |
> +    HANTRO_VP8_DECODER | HANTRO_H264_DECODER |
> +    HANTRO_HEVC_DECODER,
> +   .cfg = {
> +   .id = V4L2_CID_USER_CLASS,

Are you sure you need to expose the V4L2_CID_USER_CLASS?
Maybe I'm missing something, but this looks odd.

Thanks,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [linux-sunxi] [PATCH v4 09/15] media: sunxi: Add support for the A31 MIPI CSI-2 controller

[Ezequiel Garcia]

Salut Paul,

Just a minor comment about the v4l2 async API.

On Thu, 31 Dec 2020 at 11:30, Paul Kocialkowski
 wrote:
>
> The A31 MIPI CSI-2 controller is a dedicated MIPI CSI-2 bridge
> found on Allwinner SoCs such as the A31 and V3/V3s.
>
> It is a standalone block, connected to the CSI controller on one side
> and to the MIPI D-PHY block on the other. It has a dedicated address
> space, interrupt line and clock.
>
> It is represented as a V4L2 subdev to the CSI controller and takes a
> MIPI CSI-2 sensor as its own subdev, all using the fwnode graph and
> media controller API.
>
> Only 8-bit and 10-bit Bayer formats are currently supported.
> While up to 4 internal channels to the CSI controller exist, only one
> is currently supported by this implementation.
>
> Signed-off-by: Paul Kocialkowski 
> ---
>  drivers/media/platform/sunxi/Kconfig  |   1 +
>  drivers/media/platform/sunxi/Makefile |   1 +
>  .../platform/sunxi/sun6i-mipi-csi2/Kconfig|  12 +
>  .../platform/sunxi/sun6i-mipi-csi2/Makefile   |   4 +
>  .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c   | 590 ++
>  .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h   | 117 
>  6 files changed, 725 insertions(+)
>  create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig
>  create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile
>  create mode 100644 
> drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
>  create mode 100644 
> drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h
>
[..]
> +static int sun6i_mipi_csi2_v4l2_setup(struct sun6i_mipi_csi2_dev *cdev)
> +{
> +   struct sun6i_mipi_csi2_video *video = >video;
> +   struct v4l2_subdev *subdev = >subdev;
> +   struct v4l2_async_notifier *notifier = >notifier;
> +   struct fwnode_handle *handle;
> +   struct v4l2_fwnode_endpoint *endpoint;
> +   struct v4l2_async_subdev *subdev_async;
> +   int ret;
> +
> +   /* Subdev */
> +
> +   v4l2_subdev_init(subdev, _mipi_csi2_subdev_ops);
> +   subdev->dev = cdev->dev;
> +   subdev->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +   strscpy(subdev->name, MODULE_NAME, sizeof(subdev->name));
> +   v4l2_set_subdevdata(subdev, cdev);
> +
> +   /* Entity */
> +
> +   subdev->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> +   subdev->entity.ops = _mipi_csi2_entity_ops;
> +
> +   /* Pads */
> +
> +   video->pads[0].flags = MEDIA_PAD_FL_SINK;
> +   video->pads[1].flags = MEDIA_PAD_FL_SOURCE;
> +
> +   ret = media_entity_pads_init(>entity, 2, video->pads);
> +   if (ret)
> +   return ret;
> +
> +   /* Endpoint */
> +
> +   handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(cdev->dev), 0, 0,
> +FWNODE_GRAPH_ENDPOINT_NEXT);
> +   if (!handle) {
> +   ret = -ENODEV;
> +   goto error_media_entity;
> +   }
> +
> +   endpoint = >endpoint;
> +   endpoint->bus_type = V4L2_MBUS_CSI2_DPHY;
> +
> +   ret = v4l2_fwnode_endpoint_parse(handle, endpoint);
> +   fwnode_handle_put(handle);

I think the _put should be...

> +   if (ret)
> +   goto error_media_entity;
> +
> +   /* Notifier */
> +
> +   v4l2_async_notifier_init(notifier);
> +
> +   subdev_async = >subdev_async;
> +   ret = v4l2_async_notifier_add_fwnode_remote_subdev(notifier, handle,
> +  subdev_async);

... here. See for instance drivers/media/platform/rcar-vin/rcar-csi2.c.

(Unless I've missed something, of course).

Cheers,
Ezequiel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] hantro: Format IOCTLs compliance fixes

[Ezequiel Garcia]

On Mon, 2021-01-11 at 13:22 +0100, Ricardo Ribalda wrote:
> Hi Ezequiel
> 
> On Mon, Jan 11, 2021 at 12:55 PM Ricardo Ribalda  wrote:
> > 
> > Hi Ezequiel
> > 
> > On Mon, Jan 11, 2021 at 12:48 PM Ezequiel Garcia  
> > wrote:
> > > 
> > > Hi Ricardo,
> > > 
> > > On Mon, 2021-01-11 at 12:35 +0100, Ricardo Ribalda wrote:
> > > > Clear the reserved fields.
> > > > 
> > > > Fixes:
> > > >   fail: v4l2-test-formats.cpp(482): pix_mp.plane_fmt[0].reserved not 
> > > > zeroed
> > > > test VIDIOC_TRY_FMT: FAIL
> > > >   fail: v4l2-test-formats.cpp(482): pix_mp.plane_fmt[0].reserved not 
> > > > zeroed
> > > > test VIDIOC_S_FMT: FAIL
> > > > 
> > > > Signed-off-by: Ricardo Ribalda 
> > > > ---
> > > >  drivers/staging/media/hantro/hantro_v4l2.c | 5 +
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c 
> > > > b/drivers/staging/media/hantro/hantro_v4l2.c
> > > > index b668a82d40ad..9b384fbffc93 100644
> > > > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > > > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > > > @@ -239,6 +239,7 @@ static int hantro_try_fmt(const struct hantro_ctx 
> > > > *ctx,
> > > >     const struct hantro_fmt *fmt, *vpu_fmt;
> > > >     bool capture = V4L2_TYPE_IS_CAPTURE(type);
> > > >     bool coded;
> > > > +   int i;
> > > > 
> > > >     coded = capture == ctx->is_encoder;
> > > > 
> > > > @@ -293,6 +294,10 @@ static int hantro_try_fmt(const struct hantro_ctx 
> > > > *ctx,
> > > >     pix_mp->width * pix_mp->height * fmt->max_depth;
> > > >     }
> > > > 
> > > > +   for (i = 0; i < pix_mp->num_planes; i++)
> > > > +   memset(pix_mp->plane_fmt[i].reserved, 0,
> > > > +  sizeof(pix_mp->plane_fmt[i].reserved));
> > > > +
> > > 
> > > This looks like something that should be handled at the core,
> > > probably in drivers/media/v4l2-core/v4l2-ioctl.c::v4l_try_fmt().
> > 
> > The core does clear the reserved field from v4l2_pix_format_mplane,
> 
> My bad, It is also clearing the per plane reserved field. I was
> testing in an old kernel.
> 

OK, cool.

> The grep it is still valid though. We can remove all the memsets in the 
> drivers.
> 

That would be very nice indeed.

Thanks,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] hantro: Format IOCTLs compliance fixes

[Ezequiel Garcia]

Hi Ricardo,

On Mon, 2021-01-11 at 12:35 +0100, Ricardo Ribalda wrote:
> Clear the reserved fields.
> 
> Fixes:
>   fail: v4l2-test-formats.cpp(482): pix_mp.plane_fmt[0].reserved not zeroed
> test VIDIOC_TRY_FMT: FAIL
>   fail: v4l2-test-formats.cpp(482): pix_mp.plane_fmt[0].reserved not zeroed
> test VIDIOC_S_FMT: FAIL
> 
> Signed-off-by: Ricardo Ribalda 
> ---
>  drivers/staging/media/hantro/hantro_v4l2.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c 
> b/drivers/staging/media/hantro/hantro_v4l2.c
> index b668a82d40ad..9b384fbffc93 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -239,6 +239,7 @@ static int hantro_try_fmt(const struct hantro_ctx *ctx,
> const struct hantro_fmt *fmt, *vpu_fmt;
> bool capture = V4L2_TYPE_IS_CAPTURE(type);
> bool coded;
> +   int i;
>  
> coded = capture == ctx->is_encoder;
>  
> @@ -293,6 +294,10 @@ static int hantro_try_fmt(const struct hantro_ctx *ctx,
> pix_mp->width * pix_mp->height * fmt->max_depth;
> }
>  
> +   for (i = 0; i < pix_mp->num_planes; i++)
> +   memset(pix_mp->plane_fmt[i].reserved, 0,
> +  sizeof(pix_mp->plane_fmt[i].reserved));
> +

This looks like something that should be handled at the core,
probably in drivers/media/v4l2-core/v4l2-ioctl.c::v4l_try_fmt().

Thanks,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: media: rkvdec: rkvdec.c: Use semicolon in place of comma

[Ezequiel Garcia]

On Sat, 2021-01-09 at 17:43 +0530, Sri Laasya Nutheti wrote:
> Line 146 had a comma in place of a semicolon. Fix it.
> 
> Signed-off-by: Sri Laasya Nutheti 
> ---
> v3: Corrected email recipients
>  drivers/staging/media/rkvdec/rkvdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c 
> b/drivers/staging/media/rkvdec/rkvdec.c
> index aa4f8c287618..d3eb81ee8dc2 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -143,7 +143,7 @@ static void rkvdec_reset_fmt(struct rkvdec_ctx *ctx, 
> struct v4l2_format *f,
> memset(f, 0, sizeof(*f));
> f->fmt.pix_mp.pixelformat = fourcc;
> f->fmt.pix_mp.field = V4L2_FIELD_NONE;
> -   f->fmt.pix_mp.colorspace = V4L2_COLORSPACE_REC709,
> +   f->fmt.pix_mp.colorspace = V4L2_COLORSPACE_REC709;
> f->fmt.pix_mp.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> f->fmt.pix_mp.quantization = V4L2_QUANTIZATION_DEFAULT;
> f->fmt.pix_mp.xfer_func = V4L2_XFER_FUNC_DEFAULT;

A fix was sent already for this:

https://patchwork.kernel.org/project/linux-rockchip/patch/20201204233743.GA8530@linuxmint-midtower-pc/

Thanks,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 -next] media: rkvdec: convert comma to semicolon

[Ezequiel Garcia]

On Fri, 2021-01-08 at 17:22 +0800, Zheng Yongjun wrote:
> Replace a comma between expression statements by a semicolon.
> 
> Signed-off-by: Zheng Yongjun 
> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c 
> b/drivers/staging/media/rkvdec/rkvdec.c
> index d25c4a37e2af..66572066e7a0 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -130,7 +130,7 @@ static void rkvdec_reset_fmt(struct rkvdec_ctx *ctx, 
> struct v4l2_format *f,
> memset(f, 0, sizeof(*f));
> f->fmt.pix_mp.pixelformat = fourcc;
> f->fmt.pix_mp.field = V4L2_FIELD_NONE;
> -   f->fmt.pix_mp.colorspace = V4L2_COLORSPACE_REC709,
> +   f->fmt.pix_mp.colorspace = V4L2_COLORSPACE_REC709;
> f->fmt.pix_mp.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> f->fmt.pix_mp.quantization = V4L2_QUANTIZATION_DEFAULT;
> f->fmt.pix_mp.xfer_func = V4L2_XFER_FUNC_DEFAULT;

Seems a fix was sent already for this:

https://patchwork.kernel.org/project/linux-rockchip/patch/20201204233743.GA8530@linuxmint-midtower-pc/

Thanks,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: rkvdec: silence ktest bot build warning

[Ezequiel Garcia]

On Thu, 2021-01-07 at 12:41 +0100, Boris Brezillon wrote:
> On Thu, 7 Jan 2021 10:13:43 +0100
> Hans Verkuil  wrote:
> 
> > On 08/12/2020 16:55, Adrian Ratiu wrote:
> > > Some configurations built by the ktest bot produce the following
> > > warn, so mark the struct as __maybe_unused to avoid unnecessary
> > > ML spam.
> > >   
> > > > > drivers/staging/media/rkvdec/rkvdec.c:967:34: warning: unused 
> > > > > variable 'of_rkvdec_match' [-Wunused-const-variable]  
> > >    static const struct of_device_id of_rkvdec_match[] = {
> > >     ^
> > >    1 warning generated.  
> > 
> > I suspect that this is because there is no 'depends on OF' in the Kconfig.
> > 
> > '__maybe_unused' isn't used for this anywhere else, so this does not seem 
> > like the
> > right approach.
> 
> It's not uncommon to do that, especially when you want the driver to be
> compile-tested (`git grep -C2 __maybe_unused|grep of_device_id` even
> reports 2 drivers in the media tree :P). A `depends on OF` or an
> `#ifdef CONFIG_OF` section surrounding the of_rkvdec_match declaration
> would also do the trick.
> 

I'm fine either way, __maybe_unused or depends on OF.

Thanks,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: [PATCH v3] media: cedrus: Add support for VP8 decoding

[Ezequiel Garcia]

On Fri, 2020-11-27 at 00:40 +0100, Jernej Škrabec wrote:
> Hi!
> 
> Dne petek, 27. november 2020 ob 00:21:11 CET je Ezequiel Garcia napisal(a):
> > Hi Jernej, Emmanuel,
> > 
> > Thanks for the patch.
> > 
> > On Tue, 2020-11-10 at 23:35 +0100, Jernej Skrabec wrote:
> > > VP8 in Cedrus shares same engine as H264.
> > > 
> > > Note that it seems necessary to call bitstream parsing functions,
> > > to parse frame header, otherwise decoded image is garbage. This is
> > > contrary to what is driver supposed to do. However, values are not
> > > really used, so this might be acceptable. It's possible that bitstream
> > > parsing functions set some internal VPU state, which is later necessary
> > > for proper decoding. Biggest suspect is "VP8 probs update" trigger.
> > > 
> > > Signed-off-by: Jernej Skrabec 
> > > [addressed issues from reviewer]
> > > Signed-off-by: Emmanuel Gil Peyrot 
> > > ---
> > > Changes in v3:
> > > - addressed comments from Ezequiel Garcia - new comments,
> > >   using new macros from VP8 UAPI, new function for waiting
> > >   on bit to be set
> > > Changes in v2:
> > > - rebased on top of current linux-media master branch
> > > 
> > > NOTE: This now depends on following patch:
> > > https://patchwork.linuxtv.org/project/linux-media/patch/
> 20201108202021.4187-1-linkma...@linkmauve.fr/
> > 
> > The patch looks fairly good, so let's wait and see
> > what Hans, Paul and Maxime think about it.
> > 
> > FWIW, my humble Reviewed-by: Ezequiel Garcia 
> 
> Thanks!
> 
> > It would be good to make sure this doesn't regress
> > v4l2-compliance, or cause some regression in decoding.
> 
> I didn't include v4l2-compliance here, but it was in previous revisions. This 
> revision has just cosmetics. Not sure how it could cause any regression since 
> it's pretty standalone.
> 

Yes, indeed.

> > Not really a blocker to merge this, but I'm thinking
> > that now that we have Fluster for conformance testing,
> > we could add the VP8 vectors and use them against
> > Cedrus and Hantro:
> > 
> > https://chromium.googlesource.com/webm/vp8-test-vectors/+/refs/heads/master
> 
> I tested VP8 test vectors with initial version of this decoder by hand and 
> all 
> videos were properly decoded as far as I can tell. But automated testing is 
> always welcome.
> 

More the reason to consider this safe to merge!

Thanks,
Ezequiel

> Best regards,
> Jernej
> 
> > Thanks,
> > Ezequiel
> > 
> > >  drivers/staging/media/sunxi/cedrus/Makefile   |   3 +-
> > >  drivers/staging/media/sunxi/cedrus/cedrus.c   |   8 +
> > >  drivers/staging/media/sunxi/cedrus/cedrus.h   |  24 +
> > >  .../staging/media/sunxi/cedrus/cedrus_dec.c   |   5 +
> > >  .../staging/media/sunxi/cedrus/cedrus_hw.c|   2 +
> > >  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  80 ++
> > >  .../staging/media/sunxi/cedrus/cedrus_video.c |   9 +
> > >  .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 907 ++
> > >  8 files changed, 1037 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > 
> 
> 


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] media: cedrus: Add support for VP8 decoding

[Ezequiel Garcia]

Hi Jernej, Emmanuel,

Thanks for the patch.

On Tue, 2020-11-10 at 23:35 +0100, Jernej Skrabec wrote:
> VP8 in Cedrus shares same engine as H264.
> 
> Note that it seems necessary to call bitstream parsing functions,
> to parse frame header, otherwise decoded image is garbage. This is
> contrary to what is driver supposed to do. However, values are not
> really used, so this might be acceptable. It's possible that bitstream
> parsing functions set some internal VPU state, which is later necessary
> for proper decoding. Biggest suspect is "VP8 probs update" trigger.
> 
> Signed-off-by: Jernej Skrabec 
> [addressed issues from reviewer]
> Signed-off-by: Emmanuel Gil Peyrot 
> ---
> Changes in v3:
> - addressed comments from Ezequiel Garcia - new comments,
>   using new macros from VP8 UAPI, new function for waiting
>   on bit to be set
> Changes in v2:
> - rebased on top of current linux-media master branch
> 
> NOTE: This now depends on following patch:
> https://patchwork.linuxtv.org/project/linux-media/patch/20201108202021.4187-1-linkma...@linkmauve.fr/
> 

The patch looks fairly good, so let's wait and see
what Hans, Paul and Maxime think about it.

FWIW, my humble Reviewed-by: Ezequiel Garcia 

It would be good to make sure this doesn't regress
v4l2-compliance, or cause some regression in decoding.

Not really a blocker to merge this, but I'm thinking
that now that we have Fluster for conformance testing,
we could add the VP8 vectors and use them against
Cedrus and Hantro:

https://chromium.googlesource.com/webm/vp8-test-vectors/+/refs/heads/master

Thanks,
Ezequiel

>  drivers/staging/media/sunxi/cedrus/Makefile   |   3 +-
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   8 +
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  24 +
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |   5 +
>  .../staging/media/sunxi/cedrus/cedrus_hw.c|   2 +
>  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  80 ++
>  .../staging/media/sunxi/cedrus/cedrus_video.c |   9 +
>  .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 907 ++
>  8 files changed, 1037 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 2/4] staging: media: Introduce NVIDIA Tegra video decoder driver

[Ezequiel Garcia]

On Sat, 21 Nov 2020 at 23:01, Dmitry Osipenko  wrote:
>
> 22.11.2020 04:02, Ezequiel Garcia пишет:
> > Hi Dmitry,
> >
> ...
> >> +++ b/drivers/staging/media/tegra-vde/TODO
> >> @@ -0,0 +1,4 @@
> >> +TODO:
> >> +   - Implement V4L2 API once it gains support for stateless decoders.
> >> +
> >> +Contact: Dmitry Osipenko 
> >
> > The API for H264 stateless decoding is ready.
> > See https://lkml.org/lkml/2020/11/18/795.
>
> Hello Ezequiel,
>
> Thank you for the notification! My last attempt at implementing V4L API
> support was about a year ago and it stopped once I realized that there
> is no userspace which uses that API. FFMPEG and chromium browser had
> some kind of V4L support, but it all was oriented at downstream driver
> stacks, and thus, not usable. Do you know what is the current status?
>

The bulk of the API, which relies on the stateless decoder interface [1],
and H264 stateless V4L2 controls has been ready for some time now,
and there are various implementations supporting it.

Chromium supports it [2], and I've tested it on chromebooks,
through chromeos builds. We haven't tried a non-chromeos build,
and I would say it's quite some work.

GStreamer support is available as well. See [3] which should
work for the latest H264 controls (the ones being moved out of staging).

LibreELEC developers maintain an Ffmpeg branch [4], I expect it will
be updated for the latest H264 controls soon, and hopefully merged
in mainline Ffmpeg.

GStreamer and Ffmpeg are relatively straightforward to build and test.

Thanks,
Ezequiel

[1] 
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-stateless-decoder.html
[2] https://github.com/chromium/chromium/tree/master/media/gpu/v4l2
[3] 
https://gitlab.freedesktop.org/ezequielgarcia/gst-plugins-bad/-/tree/h264_stable_uapi
[4] https://github.com/Kwiboo/FFmpeg/tree/v4l2-request-hwaccel-4.3.


> > One minor comment below.
> >
> ...
> >> +   // PPS
> >> +   __u8  pic_init_qp;
> >> +   __u8  deblocking_filter_control_present_flag;
> >> +   __u8  constrained_intra_pred_flag;
> >> +   __u8  chroma_qp_index_offset;
> >> +   __u8  pic_order_present_flag;
> >> +
> >
> > This seems to be bottom_field_pic_order_in_frame_present_flag,
> > as there is no "pic_order_present_flag" syntax element.
>
> Correct, looks like I borrowed that name from the libvdpau API.
>
> https://vdpau.pages.freedesktop.org/libvdpau/struct_vdp_picture_info_h264.html#a405f7ef26ea76bb2c446e151062fc001
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 2/4] staging: media: Introduce NVIDIA Tegra video decoder driver

[Ezequiel Garcia]

Hi Dmitry,

On Mon, 11 Dec 2017 at 21:27, Dmitry Osipenko  wrote:
>
> NVIDIA Tegra20/30/114/124/132 SoC's have video decoder engine that
> supports standard set of video formats like H.264 / MPEG-4 / WMV / VC1.
>
> Signed-off-by: Dmitry Osipenko 
> ---
>  MAINTAINERS |9 +
>  drivers/staging/media/Kconfig   |2 +
>  drivers/staging/media/Makefile  |1 +
>  drivers/staging/media/tegra-vde/Kconfig |7 +
>  drivers/staging/media/tegra-vde/Makefile|1 +
>  drivers/staging/media/tegra-vde/TODO|4 +
>  drivers/staging/media/tegra-vde/tegra-vde.c | 1213 
> +++
>  drivers/staging/media/tegra-vde/uapi.h  |   78 ++
>  8 files changed, 1315 insertions(+)
>  create mode 100644 drivers/staging/media/tegra-vde/Kconfig
>  create mode 100644 drivers/staging/media/tegra-vde/Makefile
>  create mode 100644 drivers/staging/media/tegra-vde/TODO
>  create mode 100644 drivers/staging/media/tegra-vde/tegra-vde.c
>  create mode 100644 drivers/staging/media/tegra-vde/uapi.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7d195739f892..7f7c24949a06 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8706,6 +8706,15 @@ T:   git git://linuxtv.org/media_tree.git
>  S: Maintained
>  F: drivers/media/dvb-frontends/stv6111*
>
> +MEDIA DRIVERS FOR NVIDIA TEGRA - VDE
> +M: Dmitry Osipenko 
> +L: linux-me...@vger.kernel.org
> +L: linux-te...@vger.kernel.org
> +T: git git://linuxtv.org/media_tree.git
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
> +F: drivers/staging/media/tegra-vde/
> +
>  MEDIA INPUT INFRASTRUCTURE (V4L/DVB)
>  M: Mauro Carvalho Chehab 
>  M: Mauro Carvalho Chehab 
> diff --git a/drivers/staging/media/Kconfig b/drivers/staging/media/Kconfig
> index 3a09140700e6..227437f22acf 100644
> --- a/drivers/staging/media/Kconfig
> +++ b/drivers/staging/media/Kconfig
> @@ -31,4 +31,6 @@ source "drivers/staging/media/imx/Kconfig"
>
>  source "drivers/staging/media/omap4iss/Kconfig"
>
> +source "drivers/staging/media/tegra-vde/Kconfig"
> +
>  endif
> diff --git a/drivers/staging/media/Makefile b/drivers/staging/media/Makefile
> index f25327163c67..59a47f69884f 100644
> --- a/drivers/staging/media/Makefile
> +++ b/drivers/staging/media/Makefile
> @@ -5,3 +5,4 @@ obj-$(CONFIG_VIDEO_IMX_MEDIA)   += imx/
>  obj-$(CONFIG_VIDEO_DM365_VPFE) += davinci_vpfe/
>  obj-$(CONFIG_VIDEO_OMAP4)  += omap4iss/
>  obj-$(CONFIG_INTEL_ATOMISP) += atomisp/
> +obj-$(CONFIG_TEGRA_VDE)+= tegra-vde/
> diff --git a/drivers/staging/media/tegra-vde/Kconfig 
> b/drivers/staging/media/tegra-vde/Kconfig
> new file mode 100644
> index ..ec3ebdaa
> --- /dev/null
> +++ b/drivers/staging/media/tegra-vde/Kconfig
> @@ -0,0 +1,7 @@
> +config TEGRA_VDE
> +   tristate "NVIDIA Tegra Video Decoder Engine driver"
> +   depends on ARCH_TEGRA || COMPILE_TEST
> +   select SRAM
> +   help
> +   Say Y here to enable support for the NVIDIA Tegra video decoder
> +   driver.
> diff --git a/drivers/staging/media/tegra-vde/Makefile 
> b/drivers/staging/media/tegra-vde/Makefile
> new file mode 100644
> index ..444c1d62daa1
> --- /dev/null
> +++ b/drivers/staging/media/tegra-vde/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_TEGRA_VDE)+= tegra-vde.o
> diff --git a/drivers/staging/media/tegra-vde/TODO 
> b/drivers/staging/media/tegra-vde/TODO
> new file mode 100644
> index ..31aaa3e66d80
> --- /dev/null
> +++ b/drivers/staging/media/tegra-vde/TODO
> @@ -0,0 +1,4 @@
> +TODO:
> +   - Implement V4L2 API once it gains support for stateless decoders.
> +
> +Contact: Dmitry Osipenko 

The API for H264 stateless decoding is ready.
See https://lkml.org/lkml/2020/11/18/795.

One minor comment below.

> diff --git a/drivers/staging/media/tegra-vde/uapi.h 
> b/drivers/staging/media/tegra-vde/uapi.h
> new file mode 100644
> index ..a50c7bcae057
> --- /dev/null
> +++ b/drivers/staging/media/tegra-vde/uapi.h
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (C) 2016-2017 Dmitry Osipenko 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#ifndef _UAPI_TEGRA_VDE_H_
> +#define _UAPI_TEGRA_VDE_H_
> +
> +#include 
> +#include 
> +
> +#define FLAG_B_FRAME   (1 << 0)
> +#define FLAG_REFERENCE (1 << 1)
> +
> +struct tegra_vde_h264_frame {
> +   __s32 y_fd;
> +   __s32 cb_fd;
> +   __s32 cr_fd;
> +   __s32 aux_fd;
> +   __u32 y_offset;
> +   __u32 cb_offset;
> +   __u32 cr_offset;
> +   __u32 aux_offset;
> +   __u32 frame_num;
> +   __u32 flags;
> +
> +   __u32 reserved;
> +} __attribute__((packed));
> +
> +struct 

Re: [RESEND PATCH 0/2] media: uapi: Expose VP8 probability lengths as defines

[Ezequiel Garcia]

Hi Emmanuel,

Thanks for the patch.

On Mon, 9 Nov 2020 at 15:37, Emmanuel Gil Peyrot  wrote:
>
> These values will be used by various drivers implementing the VP8
> stateless API.
>
> This had been suggested by Ezequiel Garcia for the Cedrus VP8 driver.
>
> The only driver using this API (until now) has also been updated to use
> these new defines.
>
> This is a resend because I forgot to include most maintainers, sorry for
> that.  It’s my very first patch to the kernel, I didn’t know about
> scripts/get_maintainers.pl
>

I haven't validated these two patches, but on a first look,
it seems it's a low-hanging fruit nice cleanup. Thanks for that!

Since it seems you are looking for interesting things to contribute,
note that the vp8-ctrls.h header is lacking some nice documentation
on each structure.

This should be done by looking at the VP8 syntax spec and documenting
things appropriately. See how it's done for H.264 and VP9:

https://patchwork.linuxtv.org/project/linux-media/patch/20200928201433.327068-1-ezequ...@collabora.com/
https://patchwork.kernel.org/project/linux-rockchip/patch/20201102190551.1223389-3-adrian.ra...@collabora.com/

Thanks,
Ezequiel

> Emmanuel Gil Peyrot (2):
>   media: uapi: Expose probability lengths as defines
>   media: hantro: Use VP8 lengths defined in uapi
>
>  drivers/staging/media/hantro/hantro_vp8.c | 4 ++--
>  include/media/vp8-ctrls.h | 6 --
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> --
> 2.29.2
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] media: cedrus: Propagate OUTPUT resolution to CAPTURE

[Ezequiel Garcia]

Hi Nicolas,

On Thu, 2020-09-17 at 20:43 -0400, Nicolas Dufresne wrote:
> Le jeudi 17 septembre 2020 à 20:27 -0400, Nicolas Dufresne a écrit :
> > As per spec, the CAPTURE resolution should be automatically set based on
> > the OTUPUT resolution. This patch properly propagate width/height to the
> > capture when the OUTPUT format is set and override the user provided
> > width/height with configured OUTPUT resolution when the CAPTURE fmt is
> > updated.
> > 
> > This also prevents userspace from selecting a CAPTURE resolution that is
> > too small, avoiding kernel oops.
> 
> Just in case it wasn't obvious, this is fully reproducible oops
> whenever you use GStreamer 1.18. Here's a copy of Ondrej report from
> today which thankfully allowed me to realized I had never completed
> this patch. Pretty much all kernel that includes Cedrus are to be
> affect, though is a staging driver on staging API of course.
> 
> ---
> 
> I tried testing cedrus with gstreamer 1.18 and it managed to crash the
> kernel on
> A64. I used:
> 
>   gst-launch-1.0 filesrc location=test.mkv ! matroskademux ! queue !
> h264parse ! v4l2slh264dec ! filesink location=aaa.dat
> 
> Unable to handle kernel paging request at virtual address
> 8080808080808088
> Mem abort info:
>   ESR = 0x9644
>   EC = 0x25: DABT (current EL), IL = 32 bits
>   SET = 0, FnV = 0
>   EA = 0, S1PTW = 0
> Data abort info:
>   ISV = 0, ISS = 0x0044
>   CM = 0, WnR = 1
> [8080808080808088] address between user and kernel address ranges
> Internal error: Oops: 9644 [#1] SMP
> Modules linked in: modem_power hci_uart btrtl bluetooth ecdh_generic
> ecc sunxi_cedrus(C) sun8i_ce crypto_engine snd_soc_bt_sco
> snd_soc_simple_card snd_soc_simple_card_utils snd_soc_simple_amplifier
> sun50i_codec_analog sun8i_codec sun8i_adda_pr_regmap snd_soc_ec25
> sun4i_i2s snd_soc_core snd_pcm_dmaengine snd_pcm snd_timer snd
> soundcore stk3310 inv_mpu6050_i2c inv_mpu6050 st_magn_i2c
> st_sensors_i2c st_magn st_sensors industrialio_triggered_buffer
> kfifo_buf regmap_i2c option usb_wwan usbserial anx7688 ohci_platform
> ohci_hcd ehci_platform ehci_hcd g_cdc usb_f_acm u_serial usb_f_ecm
> u_ether libcomposite sunxi phy_generic musb_hdrc udc_core usbcore
> sun8i_rotate v4l2_mem2mem gc2145 ov5640 sun6i_csi videobuf2_dma_contig
> v4l2_fwnode videobuf2_memops videobuf2_v4l2 videobuf2_common videodev
> mc 8723cs(C) cfg80211 rfkill lima gpu_sched goodix
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G C5.9.0-rc5-
> 00386-g4fe2ef82bd0b #62
> Hardware name: Pine64 PinePhone (1.2) (DT)
> pstate: 8085 (Nzcv daIf -PAN -UAO BTYPE=--)
> pc : v4l2_m2m_buf_remove+0x44/0x90 [v4l2_mem2mem]
> lr : v4l2_m2m_buf_remove+0x18/0x90 [v4l2_mem2mem]
> sp : ffc010c8be20
> x29: ffc010c8be20 x28: ffc010bb2fc0 
> x27: 0060 x26: ffc010935e58 
> x25: ffc010c06a5a x24: 0080 
> x23: 0005 x22: ffc010c8bf4c 
> x21: ff806ba0d088 x20: ff80687d1800 
> x19: ff8066c40298 x18:  
> x17:  x16:  
> x15: 01b66678fd80 x14: 0204 
> x13: 0001 x12: 0040 
> x11: ff806f0c0248 x10: ff806f0c024a 
> x9 : ffc010bbdac8 x8 : ff806f000270 
> x7 :  x6 : dead0100 
> x5 : dead0122 x4 :  
> x3 : 8080808080808080 x2 : 8080808080808080 
> x1 : ff80641327a8 x0 : 0080 
> Call trace:
>  v4l2_m2m_buf_remove+0x44/0x90 [v4l2_mem2mem]
>  v4l2_m2m_buf_done_and_job_finish+0x34/0x140 [v4l2_mem2mem]
>  cedrus_irq+0x8c/0xc0 [sunxi_cedrus]
>  __handle_irq_event_percpu+0x54/0x150
>  handle_irq_event+0x4c/0xec
>  handle_fasteoi_irq+0xbc/0x1c0
>  __handle_domain_irq+0x78/0xdc
>  gic_handle_irq+0x50/0xa0
>  el1_irq+0xb8/0x140
>  arch_cpu_idle+0x10/0x14
>  cpu_startup_entry+0x24/0x60
>  rest_init+0xb0/0xbc
>  arch_call_rest_init+0xc/0x14
>  start_kernel+0x690/0x6b0
> Code: f2fbd5a6 f2fbd5a5 5284 a9400823 (f9000462) 
> ---[ end trace 88233b9a76cdb261 ]---
> Kernel panic - not syncing: Fatal exception in interrupt
> 

Just FWIW, you could have included the panic backtrace and
the information about the bug in the patch description.

The driver is in staging, but still I'd say it's worth
to have:

Cc: sta...@vger.kernel.org
Fixes: 50e761516f2b ("media: platform: Add Cedrus VPU decoder driver")

Thanks,
Ezequiel

> > Signed-off-by: Nicolas Dufresne 
> > Reviewed-by: Ezequiel Garcia 
> > Acked-by: Paul Kocialkowski 
> > Tested-by: Ondrej Jirman 
> > ---
> >  .../staging/media/sunxi/cedrus/cedrus_video.c | 29 +--
> >  1 file changed, 27 insertio

Re: [PATCH v2] media: cedrus: Add support for VP8 decoding

[Ezequiel Garcia]

On Sun, 26 Jul 2020 at 16:16, Jernej Škrabec  wrote:
>
> Hi Ezequiel!
>
> Dne sobota, 25. julij 2020 ob 15:08:37 CEST je Ezequiel Garcia napisal(a):
> > Hi Jernej,
> >
> > As you know, I'm not familiar with this hardware,
> > but I've tried to take a detailed look anyway.
> >
>
> Thanks, any review is welcome.
>
> > The driver looks mostly good to me, I just have
> > some minor comments.
> >
> > More importantly, seems the current uAPI
> > control is supporting this platform nicely,
> > which gives us some confidence to mark it
> > as stable.
>
> Yes, it looks pretty good in that regard.
>
> >
> > Comments below.
> >
> > On Wed, 22 Jul 2020 at 17:35, Jernej Skrabec 
> wrote:
> > > VP8 in Cedrus shares same engine as H264.
> > >
> > > Note that it seems necessary to call bitstream parsing functions,
> > > to parse frame header, otherwise decoded image is garbage. This is
> > > contrary to what is driver supposed to do. However, values are not
> > > really used, so this might be acceptable. It's possible that bitstream
> > > parsing functions set some internal VPU state, which is later necessary
> > > for proper decoding. Biggest suspect is "VP8 probs update" trigger.
> >
> > I suggest that you also put this explanation here, as a comment
> > in the cedrus_vp8.c
>
> Ok.
>
> >
> > > Signed-off-by: Jernej Skrabec 
> > > ---
> > > Changes in v2:
> > > - rebased on top of current linux-media master branch
> > >
> > >  drivers/staging/media/sunxi/cedrus/Makefile   |   3 +-
> > >  drivers/staging/media/sunxi/cedrus/cedrus.c   |   8 +
> > >  drivers/staging/media/sunxi/cedrus/cedrus.h   |  15 +
> > >  .../staging/media/sunxi/cedrus/cedrus_dec.c   |   5 +
> > >  .../staging/media/sunxi/cedrus/cedrus_hw.c|   1 +
> > >  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  80 ++
> > >  .../staging/media/sunxi/cedrus/cedrus_video.c |   9 +
> > >  .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 699 ++
> > >  8 files changed, 819 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/Makefile
> > > b/drivers/staging/media/sunxi/cedrus/Makefile index
> > > 1bce49d3e7e2..a647b3690bf8 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/Makefile
> > > +++ b/drivers/staging/media/sunxi/cedrus/Makefile
> > > @@ -2,4 +2,5 @@
> > >
> > >  obj-$(CONFIG_VIDEO_SUNXI_CEDRUS) += sunxi-cedrus.o
> > >
> > >  sunxi-cedrus-y = cedrus.o cedrus_video.o cedrus_hw.o cedrus_dec.o \
> > >
> > > -cedrus_mpeg2.o cedrus_h264.o cedrus_h265.o
> > > +cedrus_mpeg2.o cedrus_h264.o cedrus_h265.o \
> > > +cedrus_vp8.o
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus.c index
> > > bc27f9430eeb..b2f5f03ad4a3 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > > @@ -135,6 +135,13 @@ static const struct cedrus_control cedrus_controls[]
> > > = {>
> > > .codec  = CEDRUS_CODEC_H265,
> > > .required   = false,
> > >
> > > },
> > >
> > > +   {
> > > +   .cfg = {
> > > +   .id =
> > > V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER, +   },
> > > +   .codec  = CEDRUS_CODEC_VP8,
> > > +   .required   = true,
> > > +   },
> > >
> > >  };
> > >
> > >  #define CEDRUS_CONTROLS_COUNT  ARRAY_SIZE(cedrus_controls)
> > >
> > > @@ -381,6 +388,7 @@ static int cedrus_probe(struct platform_device *pdev)
> > >
> > > dev->dec_ops[CEDRUS_CODEC_MPEG2] = _dec_ops_mpeg2;
> > > dev->dec_ops[CEDRUS_CODEC_H264] = _dec_ops_h264;
> > > dev->dec_ops[CEDRUS_CODEC_H265] = _dec_ops_h265;
> > >
> > > +   dev->dec_ops[CEDRUS_CODEC_VP8] = _dec_ops_vp8;
> > >
> > > mutex_init(>dev_mutex);
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> > > 9676ab8a..9f4605afa0f4 100644
> > &

Re: [PATCH v2] media: cedrus: Add support for VP8 decoding

[Ezequiel Garcia]

Hi Jernej,

As you know, I'm not familiar with this hardware,
but I've tried to take a detailed look anyway.

The driver looks mostly good to me, I just have
some minor comments.

More importantly, seems the current uAPI
control is supporting this platform nicely,
which gives us some confidence to mark it
as stable.

Comments below.

On Wed, 22 Jul 2020 at 17:35, Jernej Skrabec  wrote:
>
> VP8 in Cedrus shares same engine as H264.
>
> Note that it seems necessary to call bitstream parsing functions,
> to parse frame header, otherwise decoded image is garbage. This is
> contrary to what is driver supposed to do. However, values are not
> really used, so this might be acceptable. It's possible that bitstream
> parsing functions set some internal VPU state, which is later necessary
> for proper decoding. Biggest suspect is "VP8 probs update" trigger.
>

I suggest that you also put this explanation here, as a comment
in the cedrus_vp8.c

> Signed-off-by: Jernej Skrabec 
> ---
> Changes in v2:
> - rebased on top of current linux-media master branch
>
>  drivers/staging/media/sunxi/cedrus/Makefile   |   3 +-
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   8 +
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  15 +
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |   5 +
>  .../staging/media/sunxi/cedrus/cedrus_hw.c|   1 +
>  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  80 ++
>  .../staging/media/sunxi/cedrus/cedrus_video.c |   9 +
>  .../staging/media/sunxi/cedrus/cedrus_vp8.c   | 699 ++
>  8 files changed, 819 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
>
> diff --git a/drivers/staging/media/sunxi/cedrus/Makefile 
> b/drivers/staging/media/sunxi/cedrus/Makefile
> index 1bce49d3e7e2..a647b3690bf8 100644
> --- a/drivers/staging/media/sunxi/cedrus/Makefile
> +++ b/drivers/staging/media/sunxi/cedrus/Makefile
> @@ -2,4 +2,5 @@
>  obj-$(CONFIG_VIDEO_SUNXI_CEDRUS) += sunxi-cedrus.o
>
>  sunxi-cedrus-y = cedrus.o cedrus_video.o cedrus_hw.o cedrus_dec.o \
> -cedrus_mpeg2.o cedrus_h264.o cedrus_h265.o
> +cedrus_mpeg2.o cedrus_h264.o cedrus_h265.o \
> +cedrus_vp8.o
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c 
> b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index bc27f9430eeb..b2f5f03ad4a3 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -135,6 +135,13 @@ static const struct cedrus_control cedrus_controls[] = {
> .codec  = CEDRUS_CODEC_H265,
> .required   = false,
> },
> +   {
> +   .cfg = {
> +   .id = 
> V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER,
> +   },
> +   .codec  = CEDRUS_CODEC_VP8,
> +   .required   = true,
> +   },
>  };
>
>  #define CEDRUS_CONTROLS_COUNT  ARRAY_SIZE(cedrus_controls)
> @@ -381,6 +388,7 @@ static int cedrus_probe(struct platform_device *pdev)
> dev->dec_ops[CEDRUS_CODEC_MPEG2] = _dec_ops_mpeg2;
> dev->dec_ops[CEDRUS_CODEC_H264] = _dec_ops_h264;
> dev->dec_ops[CEDRUS_CODEC_H265] = _dec_ops_h265;
> +   dev->dec_ops[CEDRUS_CODEC_VP8] = _dec_ops_vp8;
>
> mutex_init(>dev_mutex);
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h 
> b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index 9676ab8a..9f4605afa0f4 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -35,6 +35,7 @@ enum cedrus_codec {
> CEDRUS_CODEC_MPEG2,
> CEDRUS_CODEC_H264,
> CEDRUS_CODEC_H265,
> +   CEDRUS_CODEC_VP8,
> CEDRUS_CODEC_LAST,
>  };
>
> @@ -75,6 +76,10 @@ struct cedrus_h265_run {
> const struct v4l2_ctrl_hevc_slice_params*slice_params;
>  };
>
> +struct cedrus_vp8_run {
> +   const struct v4l2_ctrl_vp8_frame_header *slice_params;

I don't think VP8 has any concept of slice, as H264 does.
I think it's misleading to call this parameter as slice_params.

> +};
> +
>  struct cedrus_run {
> struct vb2_v4l2_buffer  *src;
> struct vb2_v4l2_buffer  *dst;
> @@ -83,6 +88,7 @@ struct cedrus_run {
> struct cedrus_h264_run  h264;
> struct cedrus_mpeg2_run mpeg2;
> struct cedrus_h265_run  h265;
> +   struct cedrus_vp8_run   vp8;
> };
>  };
>
> @@ -134,6 +140,14 @@ struct cedrus_ctx {
> void*neighbor_info_buf;
> dma_addr_t  neighbor_info_buf_addr;
> } h265;
> +   struct {
> +   unsigned intlast_frame_p_type;
> +   unsigned intlast_filter_type;
> +   unsigned intlast_sharpness_level;
> +
> +   u8  *entropy_probs_buf;
> +   

Re: [PATCH] media: cedrus: Propagate OUTPUT resolution to CAPTURE

[Ezequiel Garcia]

It seems this one felt thru the cracks. Sorry for the delay.

On Thu, 2020-05-14 at 11:39 -0400, Nicolas Dufresne wrote:
> As per spec, the CAPTURE resolution should be automatically set based on
> the OTUPUT resolution. This patch properly propagate width/height to the
> capture when the OUTPUT format is set and override the user provided
> width/height with configured OUTPUT resolution when the CAPTURE fmt is
> updated.
> 
> This also prevents userspace from selecting a CAPTURE resolution that is
> too small, avoiding unwanted page faults.
> 
> Signed-off-by: Nicolas Dufresne 

This looks correct.

Reviewed-by: Ezequiel Garcia 

Thanks,
Ezequiel

> ---
>  drivers/staging/media/sunxi/cedrus/cedrus_video.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c 
> b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index 16d82309e7b6..a6d6b15adc2e 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -247,6 +247,8 @@ static int cedrus_try_fmt_vid_cap(struct file *file, void 
> *priv,
>   return -EINVAL;
>  
>   pix_fmt->pixelformat = fmt->pixelformat;
> + pix_fmt->width = ctx->src_fmt.width;
> + pix_fmt->height = ctx->src_fmt.height;
>   cedrus_prepare_format(pix_fmt);
>  
>   return 0;
> @@ -319,11 +321,14 @@ static int cedrus_s_fmt_vid_out(struct file *file, void 
> *priv,
>   break;
>   }
>  
> - /* Propagate colorspace information to capture. */
> + /* Propagate format information to capture. */
>   ctx->dst_fmt.colorspace = f->fmt.pix.colorspace;
>   ctx->dst_fmt.xfer_func = f->fmt.pix.xfer_func;
>   ctx->dst_fmt.ycbcr_enc = f->fmt.pix.ycbcr_enc;
>   ctx->dst_fmt.quantization = f->fmt.pix.quantization;
> + ctx->dst_fmt.width = ctx->src_fmt.width;
> + ctx->dst_fmt.height = ctx->src_fmt.height;
> + cedrus_prepare_format(>dst_fmt);
>  
>   return 0;
>  }
> -- 
> 2.26.2
> 
> 


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] media: uapi: h264: update reference lists

[Ezequiel Garcia]

Hello Jernej,

I'd like to post a new H264 uAPI cleanup series soon,
would you mind resending this, or otherwise do you
mind if I include this patch in the series?

See below for a tiny comment.

On Thu, 4 Jun 2020 at 15:55, Jernej Skrabec  wrote:
>
> When dealing with with interlaced frames, reference lists must tell if
> each particular reference is meant for top or bottom field. This info
> is currently not provided at all in the H264 related controls.
>
> Make reference lists hold a structure which will also hold flags along
> index into DPB array. Flags will tell if reference is meant for top or
> bottom field.
>
> Currently the only user of these lists is Cedrus which is just compile
> fixed here. Actual usage of newly introduced flags will come in
> following commit.
>
> Signed-off-by: Jernej Skrabec 
> ---
>  .../media/v4l/ext-ctrls-codec.rst | 40 ++-
>  .../staging/media/sunxi/cedrus/cedrus_h264.c  |  6 +--
>  include/media/h264-ctrls.h| 12 +-
>  3 files changed, 51 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst 
> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index d0d506a444b1..6c36d298db20 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1843,10 +1843,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>  * - __u32
>- ``slice_group_change_cycle``
>-
> -* - __u8
> +* - struct :c:type:`v4l2_h264_reference`
>- ``ref_pic_list0[32]``
>- Reference picture list after applying the per-slice modifications
> -* - __u8
> +* - struct :c:type:`v4l2_h264_reference`
>- ``ref_pic_list1[32]``
>- Reference picture list after applying the per-slice modifications
>  * - __u32
> @@ -1926,6 +1926,42 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>- ``chroma_offset[32][2]``
>-
>
> +``Picture Reference``
> +
> +.. c:type:: v4l2_h264_reference
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: struct v4l2_h264_reference
> +:header-rows:  0
> +:stub-columns: 0
> +:widths:   1 1 2
> +
> +* - __u16
> +  - ``flags``
> +  - See :ref:`Picture Reference Flags `
> +* - __u8
> +  - ``index``
> +  -
> +
> +.. _h264_reference_flags:
> +
> +``Picture Reference Flags``
> +
> +.. cssclass:: longtable
> +
> +.. flat-table::
> +:header-rows:  0
> +:stub-columns: 0
> +:widths:   1 1 2
> +
> +* - ``V4L2_H264_REFERENCE_FLAG_TOP_FIELD``
> +  - 0x0001
> +  -
> +* - ``V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD``
> +  - 0x0002
> +  -
> +
>  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS (struct)``
>  Specifies the decode parameters (as extracted from the bitstream)
>  for the associated H264 slice data. This includes the necessary
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c 
> b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index 54ee2aa423e2..cce527bbdf86 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -166,8 +166,8 @@ static void cedrus_write_frame_list(struct cedrus_ctx 
> *ctx,
>
>  static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>struct cedrus_run *run,
> -  const u8 *ref_list, u8 num_ref,
> -  enum cedrus_h264_sram_off sram)
> +  const struct v4l2_h264_reference *ref_list,
> +  u8 num_ref, enum cedrus_h264_sram_off sram)
>  {
> const struct v4l2_ctrl_h264_decode_params *decode = 
> run->h264.decode_params;
> struct vb2_queue *cap_q;
> @@ -188,7 +188,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
> int buf_idx;
> u8 dpb_idx;
>
> -   dpb_idx = ref_list[i];
> +   dpb_idx = ref_list[i].index;
> dpb = >dpb[dpb_idx];
>
> if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index 080fd1293c42..9b1cbc9bc38e 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -140,6 +140,14 @@ struct v4l2_h264_pred_weight_table {
>  #define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED0x04
>  #define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH 0x08
>
> +#define V4L2_H264_REFERENCE_FLAG_TOP_FIELD 0x01
> +#define V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD  0x02
> +
> +struct v4l2_h264_reference {
> +   __u8 flags;
> +   __u8 index;
> +};
> +
>  struct v4l2_ctrl_h264_slice_params {
> /* Size in bytes, including header */
> __u32 size;
> @@ -182,8 +190,8 @@ struct v4l2_ctrl_h264_slice_params {
>  * Entries on each 

Re: [PATCH 0/3] media: uapi: cedrus: Fix decoding interlaced H264 content

[Ezequiel Garcia]

Hi Jernej,

On Thu, 4 Jun 2020 at 15:55, Jernej Skrabec  wrote:
>
> Currently H264 interlaced content it's not properly decoded on Cedrus.
> There are two reasons for this:
> 1. slice parameters control doesn't provide enough information
> 2. bug in frame list construction in Cedrus driver
>
> As described in commit message in patch 1, references stored in
> reference lists should tell if reference targets top or bottom field.
> However, this information is currently not provided. Patch 1 adds
> it in form of flags which are set for each reference. Patch 2 then
> uses those flags in Cedrus driver.
>
> Frame list construction is fixed in patch 3.
>
> This solution was extensively tested using Kodi on LibreELEC with A64,
> H3, H5 and H6 SoCs in slightly different form (flags were transmitted
> in MSB bits in index).
>

So, if I understand correctly the field needs to be passed per-reference,
and the current per-DPB entry is not good?

If you could point at the userspace code for this, it would be interesting
to take a look.

> Note: I'm not 100% sure if flags for both, top and bottom fields are
> needed. Any input here would be welcome.
>

Given enum v4l2_field is already part of the uAPI, perhaps it makes
sense to just reuse that for the field type? Maybe it's an overkill,
but it would make sense to reuse the concepts and types that
already exist.

We can still add a reserved field to make this new reference type
extensive.

Thanks,
Ezequiel


> Please take a look.
>
> Best regards,
> Jernej
>
> Jernej Skrabec (3):
>   media: uapi: h264: update reference lists
>   media: cedrus: h264: Properly configure reference field
>   media: cedrus: h264: Fix frame list construction
>
>  .../media/v4l/ext-ctrls-codec.rst | 40 ++-
>  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 27 +++--
>  include/media/h264-ctrls.h| 12 +-
>  3 files changed, 62 insertions(+), 17 deletions(-)
>
> --
> 2.27.0
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: cedrus: Add support for additional output formats

[Ezequiel Garcia]

Hello Jernej,

Thanks for the patch.

On Wed, 20 May 2020 at 14:12, Jernej Skrabec  wrote:
>
> If VPU supports untiled output, it actually supports several different
> YUV 4:2:0 layouts, namely NV12, NV21, YUV420 and YVU420.
>
> Add support for all of them.
>
> Signed-off-by: Jernej Skrabec 
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus_hw.c | 18 +-
>  .../staging/media/sunxi/cedrus/cedrus_video.c  | 18 ++
>  2 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c 
> b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> index daf5f244f93b..c119fd8c4b92 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> @@ -83,9 +83,25 @@ void cedrus_dst_format_set(struct cedrus_dev *dev,
>
> switch (fmt->pixelformat) {
> case V4L2_PIX_FMT_NV12:
> +   case V4L2_PIX_FMT_NV21:
> +   case V4L2_PIX_FMT_YUV420:
> +   case V4L2_PIX_FMT_YVU420:
> chroma_size = ALIGN(width, 16) * ALIGN(height, 16) / 2;
>
> -   reg = VE_PRIMARY_OUT_FMT_NV12;
> +   switch (fmt->pixelformat) {
> +   case V4L2_PIX_FMT_NV12:
> +   reg = VE_PRIMARY_OUT_FMT_NV12;
> +   break;
> +   case V4L2_PIX_FMT_NV21:
> +   reg = VE_PRIMARY_OUT_FMT_NV21;
> +   break;
> +   case V4L2_PIX_FMT_YUV420:
> +   reg = VE_PRIMARY_OUT_FMT_YU12;
> +   break;
> +   case V4L2_PIX_FMT_YVU420:
> +   reg = VE_PRIMARY_OUT_FMT_YV12;
> +   break;
> +   }
> cedrus_write(dev, VE_PRIMARY_OUT_FMT, reg);
>

I think it would result in a cleaner code if you extend
cedrus_format to include the hw_format.

Something along these lines (not a complete patch):

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index 15cf1f10221b..618daaa65a82 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -48,10 +48,12 @@ static struct cedrus_format cedrus_formats[] = {
},
{
.pixelformat= V4L2_PIX_FMT_SUNXI_TILED_NV12,
+   .hw_format  = VE_PRIMARY_OUT_FMT_TILED_32_NV12,
.directions = CEDRUS_DECODE_DST,
},
{
.pixelformat= V4L2_PIX_FMT_NV12,
+   .hw_format  = VE_PRIMARY_OUT_FMT_NV12,
.directions = CEDRUS_DECODE_DST,
.capabilities   = CEDRUS_CAPABILITY_UNTILED,
},
@@ -274,6 +276,7 @@ static int cedrus_s_fmt_vid_cap(struct file *file,
void *priv,
 {
struct cedrus_ctx *ctx = cedrus_file2ctx(file);
struct cedrus_dev *dev = ctx->dev;
+   struct cedrus_format *fmt;
struct vb2_queue *vq;
int ret;

@@ -287,7 +290,10 @@ static int cedrus_s_fmt_vid_cap(struct file
*file, void *priv,

ctx->dst_fmt = f->fmt.pix;

-   cedrus_dst_format_set(dev, >dst_fmt);
+   fmt = cedrus_find_format(ctx->dst_fmt.pixelformat,
+CEDRUS_DECODE_DST,
+dev->capabilities);
+   cedrus_dst_format_set(dev, fmt);

return 0;
 }

So then in cedrus_dst_format_set() you can just
write VE_PRIMARY_OUT_FMT with fmt->hw_format.

> reg = chroma_size / 2;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c 
> b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index 15cf1f10221b..016021d71df2 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -55,6 +55,21 @@ static struct cedrus_format cedrus_formats[] = {
> .directions = CEDRUS_DECODE_DST,
> .capabilities   = CEDRUS_CAPABILITY_UNTILED,
> },
> +   {
> +   .pixelformat= V4L2_PIX_FMT_NV21,
> +   .directions = CEDRUS_DECODE_DST,
> +   .capabilities   = CEDRUS_CAPABILITY_UNTILED,
> +   },
> +   {
> +   .pixelformat= V4L2_PIX_FMT_YUV420,
> +   .directions = CEDRUS_DECODE_DST,
> +   .capabilities   = CEDRUS_CAPABILITY_UNTILED,
> +   },
> +   {
> +   .pixelformat= V4L2_PIX_FMT_YVU420,
> +   .directions = CEDRUS_DECODE_DST,
> +   .capabilities   = CEDRUS_CAPABILITY_UNTILED,
> +   },
>  };
>
>  #define CEDRUS_FORMATS_COUNT   ARRAY_SIZE(cedrus_formats)
> @@ -130,6 +145,9 @@ void cedrus_prepare_format(struct v4l2_pix_format 
> *pix_fmt)
> break;
>
> case V4L2_PIX_FMT_NV12:
> +   case V4L2_PIX_FMT_NV21:
> +   case V4L2_PIX_FMT_YUV420:
> +   case V4L2_PIX_FMT_YVU420:
> /* 16-aligned stride. */

Re: [PATCH v2 8/9] arm64: dts: rockchip: add rx0 mipi-phy for rk3399

[Ezequiel Garcia]

Hi Heiko,

On Fri, 2020-04-03 at 13:15 -0300, Helen Koike wrote:
> From: Shunqian Zheng 
> 
> Designware MIPI D-PHY, used for ISP0 in rk3399.
> 
> Verified with:
> make ARCH=arm64 dtbs_check 
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> 
> Signed-off-by: Shunqian Zheng 
> Signed-off-by: Jacob Chen 
> Signed-off-by: Helen Koike 
> 
> ---
> 
> Changes in v2:
> - fix alignment of clocks
> 
> V1:
> This patchset came from the original ISP series from Rockchip:
> 
> https://patchwork.kernel.org/patch/10267409/
> 

Can you take the devicetree changes (patches 8 and 9) ?

Thanks,
Ezequiel

> The only difference is:
> - add phy-cells
> - update compatible to "rockchip,rk3399-mipi-dphy-rx0"
> - commit message
> ---
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi 
> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index 33cc21fcf4c10..6b3380b10e596 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -1394,6 +1394,17 @@ io_domains: io-domains {
>   status = "disabled";
>   };
>  
> + mipi_dphy_rx0: mipi-dphy-rx0 {
> + compatible = "rockchip,rk3399-mipi-dphy-rx0";
> + clocks = < SCLK_MIPIDPHY_REF>,
> +  < SCLK_DPHY_RX0_CFG>,
> +  < PCLK_VIO_GRF>;
> + clock-names = "dphy-ref", "dphy-cfg", "grf";
> + power-domains = < RK3399_PD_VIO>;
> + #phy-cells = <0>;
> + status = "disabled";
> + };
> +
>   u2phy0: usb2-phy@e450 {
>   compatible = "rockchip,rk3399-usb2phy";
>   reg = <0xe450 0x10>;
> -- 
> 2.26.0
> 
> 


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 7/9] media: MAINTAINERS: rkisp1: add path to dt-bindings

[Ezequiel Garcia]

Hi Hans,

On Fri, 2020-04-17 at 09:18 +0200, Hans Verkuil wrote:
> On 03/04/2020 18:15, Helen Koike wrote:
> > The Rockchip ISP bindings was moved out of staging.
> > Update MAINTAINERS file with the new path.
> 
> Shouldn't there be a reference to 
> Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> as well in MAINTAINERS?
> 

Indeed, and I can take care of that later. I was planning to move
this PHY driver out of staging, but was waiting on patches 4 and 5
of this series.

They seem fine, would you mind picking them?

Thanks!
Ezequiel

> Regards,
> 
>   Hans
> 
> > Suggested-by: Johan Jonker 
> > Signed-off-by: Helen Koike 
> > ---
> > 
> > V2:
> > - This is a new patch in the series
> > ---
> >  MAINTAINERS | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index d66ac41ef5872..726044b84cf23 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -14303,6 +14303,7 @@ M:  Helen Koike 
> >  L: linux-me...@vger.kernel.org
> >  S: Maintained
> >  F: drivers/staging/media/rkisp1/
> > +F: Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> >  
> >  ROCKCHIP RASTER 2D GRAPHIC ACCELERATION UNIT DRIVER
> >  M: Jacob Chen 
> > 
> 
> 


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: staging: rkisp1 Kconfig: depends on OF

[Ezequiel Garcia]

Hi Mauro, Randy,

On Mon, 20 Apr 2020 at 13:45, Mauro Carvalho Chehab
 wrote:
>
> building it with a random config causes a warning:
>
> WARNING: unmet direct dependencies detected for PHY_ROCKCHIP_DPHY_RX0
>   Depends on [n]: STAGING [=y] && STAGING_MEDIA [=y] && MEDIA_SUPPORT [=y] && 
> (ARCH_ROCKCHIP || COMPILE_TEST [=y]) && OF [=n]
>   Selected by [y]:
>   - VIDEO_ROCKCHIP_ISP1 [=y] && STAGING [=y] && STAGING_MEDIA [=y] && 
> MEDIA_SUPPORT [=y] && VIDEO_V4L2 [=y] && (ARCH_ROCKCHIP || COMPILE_TEST [=y])
>
> Cc: Stephen Rothwell 
> Reported-by: Randy Dunlap 
> Signed-off-by: Mauro Carvalho Chehab 

Thanks for the patch. Please note this warning (plus another one),
is already fixed by a couple patches in this series:

https://patchwork.linuxtv.org/project/linux-media/list/?series=2094

Also, Arnd sent a similar fix recently:

https://lkml.org/lkml/2020/4/8/596

The series I posted is acked by Helen and should fix all issues,
so perhaps you can merge it before more people keep finding this :-)

Thanks,
Ezequiel

> ---
>  drivers/staging/media/rkisp1/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/rkisp1/Kconfig 
> b/drivers/staging/media/rkisp1/Kconfig
> index 5ecbefa0f5ec..07e4a6e4458e 100644
> --- a/drivers/staging/media/rkisp1/Kconfig
> +++ b/drivers/staging/media/rkisp1/Kconfig
> @@ -2,7 +2,7 @@
>
>  config VIDEO_ROCKCHIP_ISP1
> tristate "Rockchip Image Signal Processing v1 Unit driver"
> -   depends on VIDEO_V4L2
> +   depends on VIDEO_V4L2 && OF
> depends on ARCH_ROCKCHIP || COMPILE_TEST
> select MEDIA_CONTROLLER
> select VIDEO_V4L2_SUBDEV_API
> --
> 2.25.2
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: staging: rkisp1: avoid unused variable warning

[Ezequiel Garcia]

On Wed, 2020-04-08 at 21:08 +0200, Arnd Bergmann wrote:
> On Wed, Apr 8, 2020 at 7:56 PM Ezequiel Garcia  wrote:
> > On Wed, 2020-04-08 at 17:52 +0200, Arnd Bergmann wrote:
> > > When compile-testing with CONFIG_OF disabled, we get a warning
> > > about an unused variable, and about inconsistent Kconfig dependencies:
> > > 
> > > WARNING: unmet direct dependencies detected for PHY_ROCKCHIP_DPHY_RX0
> > >   Depends on [n]: STAGING [=y] && STAGING_MEDIA [=y] && MEDIA_SUPPORT 
> > > [=m] && (ARCH_ROCKCHIP [=n] || COMPILE_TEST [=y]) && OF [=n]
> > >   Selected by [m]:
> > >   - VIDEO_ROCKCHIP_ISP1 [=m] && STAGING [=y] && STAGING_MEDIA [=y] && 
> > > MEDIA_SUPPORT [=m] && VIDEO_V4L2 [=m] && VIDEO_V4L2_SUBDEV_API [=y] &&
> > > (ARCH_ROCKCHIP [=n] || COMPILE_TEST [=y])
> > > 
> > > drivers/staging/media/rkisp1/rkisp1-dev.c: In function 'rkisp1_probe':
> > > drivers/staging/media/rkisp1/rkisp1-dev.c:457:22: error: unused variable 
> > > 'node' [-Werror=unused-variable]
> > >   457 |  struct device_node *node = pdev->dev.of_node;
> > > 
> > > Simply open-coding the pointer dereference in the only place
> > > the variable is used avoids the warning in all configurations,
> > > so we can allow compile-testing as well.
> > > 
> > 
> > Hello Arnd,
> > 
> > Thanks for your patch.
> > 
> > I believe this is already fixed here:
> > 
> > https://patchwork.linuxtv.org/patch/62774/
> > https://patchwork.linuxtv.org/patch/62775/
> 
> Ok, sorry for the duplicate. I only tested on mainline from a few days ago,
> so I must have missed it getting merged in the meantime.
> 

No worries!

Those were was sent very recently, and won't be
merged any time soon :-)

Ezequiel


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: staging: rkisp1: avoid unused variable warning

[Ezequiel Garcia]

On Wed, 2020-04-08 at 17:52 +0200, Arnd Bergmann wrote:
> When compile-testing with CONFIG_OF disabled, we get a warning
> about an unused variable, and about inconsistent Kconfig dependencies:
> 
> WARNING: unmet direct dependencies detected for PHY_ROCKCHIP_DPHY_RX0
>   Depends on [n]: STAGING [=y] && STAGING_MEDIA [=y] && MEDIA_SUPPORT [=m] && 
> (ARCH_ROCKCHIP [=n] || COMPILE_TEST [=y]) && OF [=n]
>   Selected by [m]:
>   - VIDEO_ROCKCHIP_ISP1 [=m] && STAGING [=y] && STAGING_MEDIA [=y] && 
> MEDIA_SUPPORT [=m] && VIDEO_V4L2 [=m] && VIDEO_V4L2_SUBDEV_API [=y] &&
> (ARCH_ROCKCHIP [=n] || COMPILE_TEST [=y])
> 
> drivers/staging/media/rkisp1/rkisp1-dev.c: In function 'rkisp1_probe':
> drivers/staging/media/rkisp1/rkisp1-dev.c:457:22: error: unused variable 
> 'node' [-Werror=unused-variable]
>   457 |  struct device_node *node = pdev->dev.of_node;
> 
> Simply open-coding the pointer dereference in the only place
> the variable is used avoids the warning in all configurations,
> so we can allow compile-testing as well.
> 

Hello Arnd,

Thanks for your patch.

I believe this is already fixed here:

https://patchwork.linuxtv.org/patch/62774/
https://patchwork.linuxtv.org/patch/62775/


Cheers,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/4] dt-bindings: phy: phy-rockchip-dphy-rx0: move rockchip dphy rx0 bindings out of staging

[Ezequiel Garcia]

(+Kishon)

Hi Helen,

I was wondering if we couldn't also move the phy driver out of staging.

Thanks,
Ezequiel
 
On Wed, 2020-04-01 at 21:02 -0300, Helen Koike wrote:
> Move phy-rockchip-dphy-rx0 bindings to Documentation/devicetree/bindings/phy
> 
> Signed-off-by: Helen Koike 
> ---
>  .../devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml   | 0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  rename {drivers/staging/media/phy-rockchip-dphy-rx0/Documentation => 
> Documentation}/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml (100%)
> 
> diff --git 
> a/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> b/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> similarity index 100%
> rename from 
> drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> rename to Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] media: hantro: Use standard luma quantization table

[Ezequiel Garcia]

Hi Andrzej,

On Mon, 2020-01-27 at 15:30 +0100, Andrzej Pietrasiewicz wrote:
> The table is actually different in the document than in this file, so align
> this file with the document.
> 
> Signed-off-by: Andrzej Pietrasiewicz 
> ---
>  drivers/staging/media/hantro/hantro_jpeg.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_jpeg.c 
> b/drivers/staging/media/hantro/hantro_jpeg.c
> index 125eb41f2ede..d3b381d00b23 100644
> --- a/drivers/staging/media/hantro/hantro_jpeg.c
> +++ b/drivers/staging/media/hantro/hantro_jpeg.c
> @@ -23,17 +23,17 @@
>  #define HUFF_CHROMA_AC_OFF   409
>  
>  /* Default tables from JPEG ITU-T.81
> - * (ISO/IEC 10918-1) Annex K.3, I
> + * (ISO/IEC 10918-1) Annex K, tables K.1 and K.2
>   */

I wonder if we shouldn't just have these tables
in decimal instead of hexa, so they look exactly
like the ones in the spec.

Thanks,
Ezequiel

>  static const unsigned char luma_q_table[] = {
> - 0x10, 0x0b, 0x0a, 0x10, 0x7c, 0x8c, 0x97, 0xa1,
> - 0x0c, 0x0c, 0x0e, 0x13, 0x7e, 0x9e, 0xa0, 0x9b,
> - 0x0e, 0x0d, 0x10, 0x18, 0x8c, 0x9d, 0xa9, 0x9c,
> - 0x0e, 0x11, 0x16, 0x1d, 0x97, 0xbb, 0xb4, 0xa2,
> - 0x12, 0x16, 0x25, 0x38, 0xa8, 0x6d, 0x67, 0xb1,
> - 0x18, 0x23, 0x37, 0x40, 0xb5, 0x68, 0x71, 0xc0,
> + 0x10, 0x0b, 0x0a, 0x10, 0x18, 0x28, 0x33, 0x3d,
> + 0x0c, 0x0c, 0x0e, 0x13, 0x1a, 0x3a, 0x3c, 0x37,
> + 0x0e, 0x0d, 0x10, 0x18, 0x28, 0x39, 0x45, 0x38,
> + 0x0e, 0x11, 0x16, 0x1d, 0x33, 0x57, 0x50, 0x3e,
> + 0x12, 0x16, 0x25, 0x38, 0x44, 0x6d, 0x67, 0x4d,
> + 0x18, 0x23, 0x37, 0x40, 0x51, 0x68, 0x71, 0x5c,
>   0x31, 0x40, 0x4e, 0x57, 0x67, 0x79, 0x78, 0x65,
> - 0x48, 0x5c, 0x5f, 0x62, 0x70, 0x64, 0x67, 0xc7,
> + 0x48, 0x5c, 0x5f, 0x62, 0x70, 0x64, 0x67, 0x63
>  };
>  
>  static const unsigned char chroma_q_table[] = {
> -- 
> 2.17.1
> 
> 


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/4] media: hantro: Read be32 words starting at every fourth byte

[Ezequiel Garcia]

On Mon, 2020-01-27 at 15:30 +0100, Andrzej Pietrasiewicz wrote:
> Since (luma/chroma)_qtable is an array of unsigned char, indexing it
> returns consecutive byte locations, but we are supposed to read the arrays
> in four-byte words. Consequently, we should be pointing
> get_unaligned_be32() at consecutive word locations instead.
> 

Ouch!

Seems we were too fast on that cleanup. Please add:

Cc: sta...@vger.kernel.org
Fixes: 00c30f42c7595f "media: rockchip vpu: remove some unused vars"
Reviewed-by: Ezequiel Garcia 

Thanks,
Ezequiel

> Signed-off-by: Andrzej Pietrasiewicz 
> ---
>  drivers/staging/media/hantro/hantro_h1_jpeg_enc.c | 9 +++--
>  drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c | 9 +++--
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c 
> b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
> index 938b48d4d3d9..be787a045c7e 100644
> --- a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
> +++ b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c
> @@ -67,12 +67,17 @@ hantro_h1_jpeg_enc_set_qtable(struct hantro_dev *vpu,
> unsigned char *chroma_qtable)
>  {
>   u32 reg, i;
> + __be32 *luma_qtable_p;
> + __be32 *chroma_qtable_p;
> +
> + luma_qtable_p = (__be32 *)luma_qtable;
> + chroma_qtable_p = (__be32 *)chroma_qtable;
>  
>   for (i = 0; i < H1_JPEG_QUANT_TABLE_COUNT; i++) {
> - reg = get_unaligned_be32(_qtable[i]);
> + reg = get_unaligned_be32(_qtable_p[i]);
>   vepu_write_relaxed(vpu, reg, H1_REG_JPEG_LUMA_QUAT(i));
>  
> - reg = get_unaligned_be32(_qtable[i]);
> + reg = get_unaligned_be32(_qtable_p[i]);
>   vepu_write_relaxed(vpu, reg, H1_REG_JPEG_CHROMA_QUAT(i));
>   }
>  }
> diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c 
> b/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
> index 067892345b5d..bdb95652d6a8 100644
> --- a/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
> +++ b/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c
> @@ -98,12 +98,17 @@ rk3399_vpu_jpeg_enc_set_qtable(struct hantro_dev *vpu,
>  unsigned char *chroma_qtable)
>  {
>   u32 reg, i;
> + __be32 *luma_qtable_p;
> + __be32 *chroma_qtable_p;
> +
> + luma_qtable_p = (__be32 *)luma_qtable;
> + chroma_qtable_p = (__be32 *)chroma_qtable;
>  
>   for (i = 0; i < VEPU_JPEG_QUANT_TABLE_COUNT; i++) {
> - reg = get_unaligned_be32(_qtable[i]);
> + reg = get_unaligned_be32(_qtable_p[i]);
>   vepu_write_relaxed(vpu, reg, VEPU_REG_JPEG_LUMA_QUAT(i));
>  
> - reg = get_unaligned_be32(_qtable[i]);
> + reg = get_unaligned_be32(_qtable_p[i]);
>   vepu_write_relaxed(vpu, reg, VEPU_REG_JPEG_CHROMA_QUAT(i));
>   }
>  }
> -- 
> 2.17.1
> 
> 


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/4] Hantro VPU JPEG encoder fixes

[Ezequiel Garcia]

Hi Andrzej,

Thanks a lot for the fixes!

On Mon, 2020-01-27 at 15:30 +0100, Andrzej Pietrasiewicz wrote:
> This series addresses quality issues in encoded JPEG images.
> 
> The first patch actually restores the intention of the original submission
> of this driver: due to a typo the helper variables were unused and then
> have been removed in some cleanup done by Mauro.
> 
> The second patch aligns the driver's luma quantization table with
> the one in the ITU-T.81 standard.
> 
> The third patch changes the order in which quantization tables are
> written to the resulting file and to the hardware. The file expects
> a zig-zag order, while the hardware wants some special order, neither
> linear nor zig-zag. In other words, hardware-wise it rearranges which
> parts of quantization tables go into which 4-byte registers - in a hardware
> specific order rather than linear or zig-zag. It also affects rk3288 and
> hasn't been tested with it.
> 
> The fourth patch then rearranges the sequence of register writes.
> The whole luma quantization table must be written first, and then the
> chroma quantization is written. In other words, while patch 3/4
> changes what goes into which register, this patch changes when each
> register is written to. It also affects rk3288 and hasn't been
> tested with it.
> 

I've just tested RK3288, and this series is indeed fixing
these issues. So for all patches:

Tested-by: Ezequiel Garcia 

Thanks,
Ezequiel

> Andrzej Pietrasiewicz (4):
>   media: hantro: Read be32 words starting at every fourth byte
>   media: hantro: Use standard luma quantization table
>   media: hantro: Write the quantization tables in proper order
>   media: hantro: Write quantization table registers in increasing
> addresses order
> 
>  .../staging/media/hantro/hantro_h1_jpeg_enc.c | 19 -
>  drivers/staging/media/hantro/hantro_jpeg.c| 76 ++-
>  drivers/staging/media/hantro/hantro_jpeg.h|  2 +-
>  .../media/hantro/rk3399_vpu_hw_jpeg_enc.c | 24 --
>  4 files changed, 89 insertions(+), 32 deletions(-)
> 
> -- 
> 2.17.1
> 
> 


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: hantro: Support H264 profile control

[Ezequiel Garcia]

Hello Hirokazu, Hans,

On Fri, 2020-01-10 at 13:31 +0100, Hans Verkuil wrote:
> On 11/29/19 1:16 AM, Tomasz Figa wrote:
> > On Sat, Nov 23, 2019 at 1:52 AM Nicolas Dufresne  
> > wrote:
> > > Le samedi 23 novembre 2019 à 01:00 +0900, Tomasz Figa a écrit :
> > > > On Sat, Nov 23, 2019 at 12:09 AM Nicolas Dufresne 
> > > >  wrote:
> > > > > Le vendredi 22 novembre 2019 à 14:16 +0900, Hirokazu Honda a écrit :
> > > > > > The Hantro G1 decoder supports H.264 profiles from Baseline to 
> > > > > > High, with
> > > > > > the exception of the Extended profile.
> > > > > > 
> > > > > > Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE control, so that the
> > > > > > applications can query the driver for the list of supported 
> > > > > > profiles.
> > > > > 
> > > > > Thanks for this patch. Do you think we could also add the LEVEL 
> > > > > control
> > > > > so the profile/level enumeration becomes complete ?
> > > > > 
> > > > > I'm thinking it would be nice if the v4l2 compliance test make sure
> > > > > that codecs do implement these controls (both stateful and stateless),
> > > > > it's essential for stack with software fallback, or multiple capable
> > > > > codec hardware but with different capabilities.
> > > > > 
> > > > 
> > > > Level is a difficult story, because it also specifies the number of
> > > > macroblocks per second, but for decoders like this the number of
> > > > macroblocks per second it can handle depends on things the driver
> > > > might be not aware of - clock frequencies, DDR throughput, system
> > > > load, etc.
> > > > 
> > > > My take on this is that the decoder driver should advertise the
> > > > highest resolution the decoder can handle due to hardware constraints.
> > > > Performance related things depend on the integration details and
> > > > should be managed elsewhere. For example Android and Chrome OS manage
> > > > expected decoding performance in per-board configuration files.
> > > 
> > > When you read datasheet, the HW is always rated to maximum level (and
> > > it's a range) with the assumption of a single stream. It seems much
> > > easier to expose this as-is, statically then to start doing some math
> > > with data that isn't fully exposed to the user. This is about filtering
> > > of multiple CODEC instances, it does not need to be rocket science,
> > > specially that the amount of missing data is important (e.g. usage of
> > > tiles, compression, IPP all have an impact on the final performance).
> > > All we want to know in userspace is if this HW is even possibly capable
> > > of LEVEL X, and if not, we'll look for another one.
> > > 
> > 
> > Agreed, one could potentially define it this way, but would it be
> > really useful for the userspace and the users? I guess it could enable
> > slightly faster fallback to software decoding in the extreme case of
> > the hardware not supporting the level at all, but I suspect that the
> > majority of cases would be the hardware just being unusably slow.
> > 
> > Also, as I mentioned before, we already return the range of supported
> > resolutions, which in practice should map to the part of the level
> > that may depend on hardware capabilities rather than performance, so
> > exposing levels as well would add redundancy to the information
> > exposed.
> 
> There is a lot of discussion here, but it all revolves around a potential
> LEVEL control.
> 
> So I gather everyone is OK with merging this patch?
> 

I'm fine with this.

[..]
> > > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c 
> > > > > > b/drivers/staging/media/hantro/hantro_drv.c
> > > > > > index 6d9d41170832..9387619235d8 100644
> > > > > > --- a/drivers/staging/media/hantro/hantro_drv.c
> > > > > > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > > > > > @@ -355,6 +355,16 @@ static const struct hantro_ctrl controls[] = {
> > > > > >   .def = 
> > > > > > V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
> > > > > >   .max = 
> > > > > > V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
> > > > > >   },
> > > > > > + }, {
> > > > > > + .codec = HANTRO_H264_DECODER,
> > > > > > + .cfg = {
> > > > > > + .id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> > > > > > + .min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,

I'd like to see the .def here ^^^ for consistency
with the other controls.

But I know this is my OCD speaking, so it's fine
as-is as well.

Reviewed-by: Ezequiel Garcia 

> > > > > > + .max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
> > > > > > + .menu_skip_mask =
> > > > > > + BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
> > > > > > + .def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,

Thanks,
Eze


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: hantro: remove a pointless NULL check

[Ezequiel Garcia]

Hi Dan,

Thanks for the patch.

On Wed, 2020-01-08 at 08:35 +0300, Dan Carpenter wrote:
> This can't be NULL and we've already dereferenced it so let's remove
> the check.
> 
> Signed-off-by: Dan Carpenter 
> ---
>  drivers/staging/media/hantro/hantro_v4l2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c 
> b/drivers/staging/media/hantro/hantro_v4l2.c
> index 85af1b96fd34..0198bcda26b7 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -688,7 +688,7 @@ static int hantro_start_streaming(struct vb2_queue *q, 
> unsigned int count)
>   return ret;
>  
>  err_codec_exit:
> - if (ctx->codec_ops && ctx->codec_ops->exit)
> + if (ctx->codec_ops->exit)

Since you are here, can you remove the other unneeded
checks in the driver?

We are assuming ctx->codec_op is non-NULL, so perhaps
a check in .probe, to check it explicitly would be better.

Thanks,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/7] media: cedrus: Remove dst_bufs from context

[Ezequiel Garcia]

On Thu, 2019-05-30 at 23:15 +0200, Jernej Skrabec wrote:
> This array is just duplicated capture buffer queue. Remove it and adjust
> code to look into capture buffer queue instead.
> 
> Signed-off-by: Jernej Skrabec 
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  4 +---
>  .../staging/media/sunxi/cedrus/cedrus_h264.c  |  4 ++--
>  .../staging/media/sunxi/cedrus/cedrus_video.c | 22 ---
>  3 files changed, 3 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h 
> b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index 3f476d0fd981..d8e6777e5e27 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -100,8 +100,6 @@ struct cedrus_ctx {
>   struct v4l2_ctrl_handlerhdl;
>   struct v4l2_ctrl**ctrls;
>  
> - struct vb2_buffer   *dst_bufs[VIDEO_MAX_FRAME];
> -
>   union {
>   struct {
>   void*mv_col_buf;
> @@ -187,7 +185,7 @@ static inline dma_addr_t cedrus_dst_buf_addr(struct 
> cedrus_ctx *ctx,
>   if (index < 0)
>   return 0;
>  
> - buf = ctx->dst_bufs[index];
> + buf = ctx->fh.m2m_ctx->cap_q_ctx.q.bufs[index];

I think you can use v4l2_m2m_get_dst_vq() to access the queue,
and vb2_get_buffer() to access buffers in a vb2 queue.

>   return buf ? cedrus_buf_addr(buf, >dst_fmt, plane) : 0;
>  }
>  
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c 
> b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index d0ee3f90ff46..b2290f98d81a 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -119,7 +119,7 @@ static void cedrus_write_frame_list(struct cedrus_ctx 
> *ctx,
>   if (buf_idx < 0)
>   continue;
>  
> - cedrus_buf = vb2_to_cedrus_buffer(ctx->dst_bufs[buf_idx]);
> + cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);

Ditto about vb2_get_buffer.

>   position = cedrus_buf->codec.h264.position;
>   used_dpbs |= BIT(position);
>  
> @@ -194,7 +194,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>   if (buf_idx < 0)
>   continue;
>  
> -     ref_buf = to_vb2_v4l2_buffer(ctx->dst_bufs[buf_idx]);
> + ref_buf = to_vb2_v4l2_buffer(cap_q->bufs[buf_idx]);

Ditto about vb2_get_buffer.

With those changes:

Reviewed-by: Ezequiel Garcia 

Thanks,
Ezequiel

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/7] media: cedrus: Disable engine after each slice decoding

[Ezequiel Garcia]

Hi Jernej,

On Mon, 2019-06-03 at 13:38 +0200, Maxime Ripard wrote:
> Hi,
> 
> On Thu, May 30, 2019 at 11:15:10PM +0200, Jernej Skrabec wrote:
> > libvdpau-sunxi always disables engine after each decoded slice.
> > Do same in Cedrus driver.
> > 
> > Presumably this also lowers power consumption which is always nice.
> > 
> > Signed-off-by: Jernej Skrabec 
> 
> Is it fixing anything though?
> 
> I indeed saw that cedar did disable it everytime, but I couldn't find
> a reason why.
> 
> Also, the power management improvement would need to be measured, it
> can even create the opposite situation where the device will draw more
> current from being woken up than if it had just remained disabled.
> 

While reviewing this, I'm noticing that cedrus_engine_disable can
be marked for static storage (with or without this patch).

Regards,
Eze

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [REGRESSION] Xorg segfaults on Asus Chromebook CP101 with Linux v5.2 (was Asus C101P Chromeboot fails to boot with Linux 5.2)

[Ezequiel Garcia]

On Sat, 13 Jul 2019 at 13:43, Alex Dewar  wrote:
>
> On 13/07/2019 16:17, Heiko Stuebner wrote:
> > Hi,
> >
> > Am Samstag, 13. Juli 2019, 13:38:45 CEST schrieb Alex Dewar:
> >> I initially thought my machine was failing to boot entirely, but it
> >> turns out it was just failing to start the display manager. I managed to
> >> escape to a tty by hammering the keyboard a bit.
> >>
> >> I suspect the culprit is the rockchip_vpu driver (in staging/media),
> >> which has been renamed to hantro in this merge window. When I run startx
> >> from a terminal, X fails to start and Xorg segfaults (log here:
> >> http://users.sussex.ac.uk/~ad374/xorg.log). X seems to work without any
> >> issues in v5.1.
> >
> > 5.2 also has support for Panfrost (Mali-Midgard GPUs) but I'm not
> > sure if it already can support X11 yet and your X11 log mentions
> > libglamoregl in the segfault stack trace.
> >
> > Apart from it bisect that Greg suggested you could also just try
> > blacklisting either panfrost or vpu kernel modules
> > /etc/udev/somewhere . This would prevent them from loading
> >
> > Hope that helps
> > Heiko
> >
> >
>
> Hi Heiko,
>
> Thanks for this. I blacklisted the panfrost driver and X magically
> started working again.
>
> I'll try to do a bisect later to find the offending commit though.
>
> In related news, it also seems that the sound and wifi drivers aren't
> working either in 5.2 (although I need to do a bit more testing to
> confirm the latter).
>

Adding myself and Tomeu.

Perhaps we need to disable Panfrost from defconfig from now?

Regards,
Eze
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] FBTFT: fbtft-bus: Fix code style problems

[Ezequiel Garcia]

On Mon, 11 Mar 2019 at 18:40, Sam Ravnborg  wrote:
>
> Hi Eze
> >
> > Why is this driver still here? I thought we migrated everyhing to
> > tinydrm already.
> Some have been ported, some are waiting for a user to do the port.
> If you looks at tinydrm you will see:
> ili9225.c  ili9341.c
>
> And we also have under panel:
> panel-ilitek-ili9881c.c
> panel-ilitek-ili9322.c
>
> But in staging there are more panels than just these.
>
> So we have not yet ported all.
> And there is today no list of what is missing.
>

Right.

Perhaps the TODO file on fbtft should be updated stating clearly that
we don't care about cleaning this one.
This is important given staging is used for Outreachy and other
introductory programs.

We can also add some big fat warning message (in case there isn't one
already) asking users of staging/fbtft to help porting/testing the
port to tinydrm.

Thanks,
Eze
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] FBTFT: fbtft-bus: Fix code style problems

[Ezequiel Garcia]

Hi everyone,

Dante: please avoid top-posting. In other words, put your replies in-line,
like I'm gonna do now. See below (and see how discussion goes on other threads).

On Mon, 11 Mar 2019 at 17:34, DANTE JAVIER PAZ  wrote:
>
> Hello Sam, thank you very much for your comments,
> As I told Dan (my email did not reach the mailing list) this is my
> first attempt to contribute,
> So I'm learning a lot from your advice and corrections.
>
> I will look for TODO lists to see if there are more useful
> contributions to make, all suggestions are also welcome.
>
> Thanks again for the patience of all of you.
> Best,
> Dante
>
>
> El lun., 11 mar. 2019 a las 13:25, Sam Ravnborg () 
> escribió:
> >
> > Hi Dante
> >
> > Thanks for the patch.
> > On Sat, Mar 09, 2019 at 06:48:52PM -0300, Dante Paz wrote:
> > > From: Dante Paz 
> > >
> > > Style and coding function issues were corrected, by avoiding macro 
> > > functions with a conflicting coding style.
> > > Signed-off-by: Dante Paz 
> >
> > But it raised a few comments.
> >
> > The staging/fbtft is a dumping of a set of drivers that
> > in the end will be migrated to DRM.
> > And there is not much gained trying to do coding style changes to these
> > drivers.
> > So please conmsider finding a drver where this is more relevant.
> >

Sam,

Why is this driver still here? I thought we migrated everyhing to
tinydrm already.

Maybe there's a list of panels still not migrated?

Thanks,
Eze
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC 2/4] media: v4l2-mem2mem: Add an optional job_done operation

[Ezequiel Garcia]

On Mon, 2019-01-14 at 14:38 +0100, Paul Kocialkowski wrote:
> Introduce a new optional job_done operation, which allows calling back
> to the driver when a job is done. Since the job might be completed
> from interrupt context where some operations are not available, having
> a callback from non-atomic context allows performing these operations
> upon completion of a job. This is particularly useful for releasing
> access to a reference buffer, which cannot be done in atomic context.
> 

I'm not exactly sure it makes a lot of sense to review this patch,
since the approach could change.

However, let me point out a few fundamental issues here.
 
> Use the already existing v4l2_m2m_device_run_work work queue for that
> and clear the M2M device current context after calling job_done in the
> worker thread, so that the private data can be passed to the operation.
> 
> Delaying the current context clearing should not be a problem since the
> next call to v4l2_m2m_try_run happens right after that.
> 

Careful here. It's misleading to think an event will happen
"right after". I'd say it's either synchronously, or asynchronously.

It's quite the opposite I'd say, the clearing will happen
"who-knows-when the scheduler picks the thread to run" :-)

Before this patch the curr_ctx was cleared in v4l2_m2m_job_finish,
atomically with the ctx job flags clearing and before waking up
threads waiting in v4l2_m2m_cancel_job.

You are now changing this, by clearing curr_ctx in a worker.
It's perfectly possible that v4l2_m2m_try_schedule will run
before the worker, trying to run with the old context, which
apparently would be safely refused.

> Signed-off-by: Paul Kocialkowski 
> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 8 ++--
>  include/media/v4l2-mem2mem.h   | 4 
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c 
> b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 631f4e2aa942..d5bccb0192f9 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -376,6 +376,11 @@ static void v4l2_m2m_device_run_work(struct work_struct 
> *work)
>   struct v4l2_m2m_dev *m2m_dev =
>   container_of(work, struct v4l2_m2m_dev, job_work);
>  
> + if (m2m_dev->m2m_ops->job_done && m2m_dev->curr_ctx)
> + m2m_dev->m2m_ops->job_done(m2m_dev->curr_ctx->priv);
> +
> + m2m_dev->curr_ctx = NULL;
> +

I don't think you can access this without taking the job spinlock.

>   v4l2_m2m_try_run(m2m_dev);
>  }
> 

Aside from this, it seems we might need this hook sooner or later.

Thanks,
Eze

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] media: rockchip vpu: remove some unused vars

[Ezequiel Garcia]

On Wed, 2018-12-05 at 19:01 -0500, Mauro Carvalho Chehab wrote:
> As complained by gcc:
> 
>   drivers/staging/media/rockchip/vpu/rk3288_vpu_hw_jpeg_enc.c: In 
> function 'rk3288_vpu_jpeg_enc_set_qtable':
>   drivers/staging/media/rockchip/vpu/rk3288_vpu_hw_jpeg_enc.c:70:10: 
> warning: variable 'chroma_qtable_p' set but not used [-Wunused-but-set-
> variable]
> __be32 *chroma_qtable_p;
> ^~~
>   drivers/staging/media/rockchip/vpu/rk3288_vpu_hw_jpeg_enc.c:69:10: 
> warning: variable 'luma_qtable_p' set but not used [-Wunused-but-set-
> variable]
> __be32 *luma_qtable_p;
> ^
>   drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c: In 
> function 'rk3399_vpu_jpeg_enc_set_qtable':
>   drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c:101:10: 
> warning: variable 'chroma_qtable_p' set but not used [-Wunused-but-set-
> variable]
> __be32 *chroma_qtable_p;
> ^~~
>   drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c:100:10: 
> warning: variable 'luma_qtable_p' set but not used [-Wunused-but-set-
> variable]
> __be32 *luma_qtable_p;
> ^
>   drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c: In function 
> 'rockchip_vpu_queue_setup':
>   drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c:522:33: warning: 
> variable 'vpu_fmt' set but not used [-Wunused-but-set-variable]
> const struct rockchip_vpu_fmt *vpu_fmt;
>^~~
>   drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c: In function 
> 'rockchip_vpu_buf_prepare':
>   drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c:560:33: warning: 
> variable 'vpu_fmt' set but not used [-Wunused-but-set-variable]
> const struct rockchip_vpu_fmt *vpu_fmt;
>    ^~~
> 
> Signed-off-by: Mauro Carvalho Chehab 

Acked-by: Ezequiel Garcia 

Thanks!


> ---
>  drivers/staging/media/rockchip/vpu/rk3288_vpu_hw_jpeg_enc.c | 5 -
>  drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c | 5 -
>  drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c   | 6 --
>  3 files changed, 16 deletions(-)
> 
> diff --git a/drivers/staging/media/rockchip/vpu/rk3288_vpu_hw_jpeg_enc.c 
> b/drivers/staging/media/rockchip/vpu/rk3288_vpu_hw_jpeg_enc.c
> index e27c10855de5..5282236d1bb1 100644
> --- a/drivers/staging/media/rockchip/vpu/rk3288_vpu_hw_jpeg_enc.c
> +++ b/drivers/staging/media/rockchip/vpu/rk3288_vpu_hw_jpeg_enc.c
> @@ -66,13 +66,8 @@ rk3288_vpu_jpeg_enc_set_qtable(struct rockchip_vpu_dev 
> *vpu,
>  unsigned char *luma_qtable,
>  unsigned char *chroma_qtable)
>  {
> - __be32 *luma_qtable_p;
> - __be32 *chroma_qtable_p;
>   u32 reg, i;
>  
> - luma_qtable_p = (__be32 *)luma_qtable;
> - chroma_qtable_p = (__be32 *)chroma_qtable;
> -
>   for (i = 0; i < VEPU_JPEG_QUANT_TABLE_COUNT; i++) {
>   reg = get_unaligned_be32(_qtable[i]);
>   vepu_write_relaxed(vpu, reg, VEPU_REG_JPEG_LUMA_QUAT(i));
> diff --git a/drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c 
> b/drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c
> index 5f75e4d11d76..dbc86d95fe3b 100644
> --- a/drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c
> +++ b/drivers/staging/media/rockchip/vpu/rk3399_vpu_hw_jpeg_enc.c
> @@ -97,13 +97,8 @@ rk3399_vpu_jpeg_enc_set_qtable(struct rockchip_vpu_dev 
> *vpu,
>  unsigned char *luma_qtable,
>  unsigned char *chroma_qtable)
>  {
> - __be32 *luma_qtable_p;
> - __be32 *chroma_qtable_p;
>   u32 reg, i;
>  
> - luma_qtable_p = (__be32 *)luma_qtable;
> - chroma_qtable_p = (__be32 *)chroma_qtable;
> -
>   for (i = 0; i < VEPU_JPEG_QUANT_TABLE_COUNT; i++) {
>   reg = get_unaligned_be32(_qtable[i]);
>   vepu_write_relaxed(vpu, reg, VEPU_REG_JPEG_LUMA_QUAT(i));
> diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c 
> b/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c
> index 038a7136d5d1..ab0fb2053620 100644
> --- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c
> +++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_enc.c
> @@ -519,17 +519,14 @@ rockchip_vpu_queue_setup(struct vb2_queue *vq,
>struct device *alloc_devs[])
>  {
>   struct rockchip_vpu_ctx *ctx = vb2_get_drv_priv(vq);
> - const struct rockchip_vpu_fmt *vpu_fmt;
>   struct v4l2_pix_format_mplane *pixfmt;
>   int 

Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver

[Ezequiel Garcia]

On Tue, 2018-08-28 at 09:34 +0200, Paul Kocialkowski wrote:
> This introduces the Cedrus VPU driver that supports the VPU found in
> Allwinner SoCs, also known as Video Engine. It is implemented through
> a v4l2 m2m decoder device and a media device (used for media requests).
> So far, it only supports MPEG2 decoding.
> 
> Since this VPU is stateless, synchronization with media requests is
> required in order to ensure consistency between frame headers that
> contain metadata about the frame to process and the raw slice data that
> is used to generate the frame.
> 
> This driver was made possible thanks to the long-standing effort
> carried out by the linux-sunxi community in the interest of reverse
> engineering, documenting and implementing support for Allwinner VPU.
> 
> Signed-off-by: Paul Kocialkowski 
> ---
>  MAINTAINERS   |   7 +
>  drivers/staging/media/Kconfig |   2 +
>  drivers/staging/media/Makefile|   1 +
>  drivers/staging/media/sunxi/Kconfig   |  15 +
>  drivers/staging/media/sunxi/Makefile  |   1 +
>  drivers/staging/media/sunxi/cedrus/Kconfig|  14 +
>  drivers/staging/media/sunxi/cedrus/Makefile   |   3 +
>  drivers/staging/media/sunxi/cedrus/cedrus.c   | 420 +
>  drivers/staging/media/sunxi/cedrus/cedrus.h   | 167 +
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   | 116 
>  .../staging/media/sunxi/cedrus/cedrus_dec.h   |  28 +
>  .../staging/media/sunxi/cedrus/cedrus_hw.c| 322 ++
>  .../staging/media/sunxi/cedrus/cedrus_hw.h|  30 +
>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 235 +++
>  .../staging/media/sunxi/cedrus/cedrus_regs.h  | 233 +++
>  .../staging/media/sunxi/cedrus/cedrus_video.c | 574 ++
>  .../staging/media/sunxi/cedrus/cedrus_video.h |  32 +
>  17 files changed, 2200 insertions(+)
>  create mode 100644 drivers/staging/media/sunxi/Kconfig
>  create mode 100644 drivers/staging/media/sunxi/Makefile
>  create mode 100644 drivers/staging/media/sunxi/cedrus/Kconfig
>  create mode 100644 drivers/staging/media/sunxi/cedrus/Makefile
>  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus.c
>  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus.h
>  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_dec.c
>  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_dec.h
>  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_hw.c
>  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_hw.h
>  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
>  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_regs.h
>  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_video.c
>  create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_video.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 435e6c08c694..08065d53c69d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -656,6 +656,13 @@ L:   linux-cry...@vger.kernel.org
>  S:   Maintained
>  F:   drivers/crypto/sunxi-ss/
>  
> +ALLWINNER VPU DRIVER
> +M:   Maxime Ripard 
> +M:   Paul Kocialkowski 
> +L:   linux-me...@vger.kernel.org
> +S:   Maintained
> +F:   drivers/staging/media/sunxi/cedrus/
> +
>  ALPHA PORT
>  M:   Richard Henderson 
>  M:   Ivan Kokshaysky 
> diff --git a/drivers/staging/media/Kconfig b/drivers/staging/media/Kconfig
> index db5cf67047ad..b3620a8f2d9f 100644
> --- a/drivers/staging/media/Kconfig
> +++ b/drivers/staging/media/Kconfig
> @@ -31,6 +31,8 @@ source "drivers/staging/media/mt9t031/Kconfig"
>  
>  source "drivers/staging/media/omap4iss/Kconfig"
>  
> +source "drivers/staging/media/sunxi/Kconfig"
> +
>  source "drivers/staging/media/tegra-vde/Kconfig"
>  
>  source "drivers/staging/media/zoran/Kconfig"
> diff --git a/drivers/staging/media/Makefile b/drivers/staging/media/Makefile
> index 503fbe47fa58..42948f805548 100644
> --- a/drivers/staging/media/Makefile
> +++ b/drivers/staging/media/Makefile
> @@ -5,5 +5,6 @@ obj-$(CONFIG_SOC_CAMERA_IMX074)   += imx074/
>  obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031/
>  obj-$(CONFIG_VIDEO_DM365_VPFE)   += davinci_vpfe/
>  obj-$(CONFIG_VIDEO_OMAP4)+= omap4iss/
> +obj-$(CONFIG_VIDEO_SUNXI)+= sunxi/
>  obj-$(CONFIG_TEGRA_VDE)  += tegra-vde/
>  obj-$(CONFIG_VIDEO_ZORAN)+= zoran/
> diff --git a/drivers/staging/media/sunxi/Kconfig 
> b/drivers/staging/media/sunxi/Kconfig
> new file mode 100644
> index ..c78d92240ceb
> --- /dev/null
> +++ b/drivers/staging/media/sunxi/Kconfig
> @@ -0,0 +1,15 @@
> +config VIDEO_SUNXI
> + bool "Allwinner sunXi family Video Devices"
> + depends on ARCH_SUNXI || COMPILE_TEST
> + help
> +   If you have an Allwinner SoC based on the sunXi family, say Y.
> +
> +   Note that this option doesn't include new drivers in the
> +   kernel: saying N will just cause Kconfig to skip all the
> +   questions about Allwinner media devices.
> +
> +if 

Re: [PATCH v7 0/8] Cedrus driver for the Allwinner Video Engine, using media requests

[Ezequiel Garcia]

Hey Paul,

On Thu, 2018-08-09 at 11:04 +0200, Paul Kocialkowski wrote:
> This is the seventh iteration of the updated Cedrus driver,
> that supports the Video Engine found on most Allwinner SoCs, starting
> with the A10. It was tested on the A13, A20, A33 and H3.
> 
> The initial version of this driver[0] was originally written and
> submitted by Florent Revest using a previous version of the request API
> that is necessary to provide coherency between controls and the buffers
> they apply to.
> 
> The driver was adapted to use the latest version of the media request
> API[1], as submitted by Hans Verkuil. Media request API support is a
> hard requirement for the Cedrus driver.
> 
> The driver itself currently only supports MPEG2 and more codecs will be
> added eventually. The default output frame format provided by the Video
> Engine is a multi-planar tiled YUV format (based on NV12). A specific
> format is introduced in the V4L2 API to describe it. Starting with the
> A33, the Video Engine can also output untiled YUV formats.
> 
> This implementation is based on the significant work that was conducted
> by various members of the linux-sunxi community for understanding and
> documenting the Video Engine's innards.
> 
> In addition to the media requests API, the following series are required
> for Cedrus:
> * vicodec: the Virtual Codec driver
> * allwinner: a64: add SRAM controller / system control
> * SRAM patches from the Cedrus VPU driver series version 5
> 
> Changes since v6:
> * Reworked MPEG2 controls to stick closer to the bitstream;
> * Updated controls documentation accordingly and added requested fixes;
> * Renamed tiled format to V4L2_PIX_FMT_SUNXI_TILED_NV12;
> * Added various minor driver fixes based on Hans' feedback;
> * Fixed dst frame alignment based on Jernej's feedback and tests;
> * Removed set bits for the disabled secondary output.
> 
> Changes since v5:
> * Added MPEG2 quantization matrices definitions and support;
> * Cleaned up registers definitions;
> * Moved the driver to staging as requested;
> 

I tried to find the reason for moving this driver to staging,
but couldn't find it in the discussion.

If there's a legitimate reason, shouldn't you add a TODO file?

Thanks,
Eze
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 4/8] media: platform: Add Cedrus VPU decoder driver

[Ezequiel Garcia]

On Wed, 2018-07-25 at 12:02 +0200, Paul Kocialkowski wrote:
> This introduces the Cedrus VPU driver that supports the VPU found in
> Allwinner SoCs, also known as Video Engine. It is implemented through
> a v4l2 m2m decoder device and a media device (used for media requests).
> So far, it only supports MPEG2 decoding.
> 
> Since this VPU is stateless, synchronization with media requests is
> required in order to ensure consistency between frame headers that
> contain metadata about the frame to process and the raw slice data that
> is used to generate the frame.
> 
> This driver was made possible thanks to the long-standing effort
> carried out by the linux-sunxi community in the interest of reverse
> engineering, documenting and implementing support for Allwinner VPU.
> 
> Signed-off-by: Paul Kocialkowski 
[..]
> +static int cedrus_probe(struct platform_device *pdev)
> +{
> + struct cedrus_dev *dev;
> + struct video_device *vfd;
> + int ret;
> +
> + dev = devm_kzalloc(>dev, sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return -ENOMEM;
> +
> + dev->dev = >dev;
> + dev->pdev = pdev;
> +
> + ret = cedrus_hw_probe(dev);
> + if (ret) {
> + dev_err(>dev, "Failed to probe hardware\n");
> + return ret;
> + }
> +
> + dev->dec_ops[CEDRUS_CODEC_MPEG2] = _dec_ops_mpeg2;
> +
> + mutex_init(>dev_mutex);
> + spin_lock_init(>irq_lock);
> +

A minor thing.

I believe this spinlock is not needed. All the data structures
it's accessing are already protected, and some operations
(stop_streaming) are guaranteed to not run at the same
time as a job.

Regards,
Eze
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel