mbox series

[v1,00/18] Add HANTRO G2/HEVC decoder support for IMX8MQ

Message ID 20210217080306.157876-1-benjamin.gaignard@collabora.com
Headers show
Series Add HANTRO G2/HEVC decoder support for IMX8MQ | expand

Message

Benjamin Gaignard Feb. 17, 2021, 8:02 a.m. UTC
The IMX8MQ got two VPUs but until now only G1 has been enabled.
This series aim to add the second VPU (aka G2) and provide basic 
HEVC decoding support.

To be able to decode HEVC it is needed to add/update some of the
structures in the uapi. In addition of them one HANTRO dedicated
control is required to inform the driver of the numbre of bits to skip
at the beginning of the slice header.
The hardware require to allocate few auxiliary buffers to store the
references frame, scaling list or tile size data.

The driver has been tested with fluster test suite stream.
For example with this command: ./fluster.py run -ts JCT-VC-HEVC_V1 -d GStreamer-H.265-V4L2SL-Gst1.0
 
This series depends of the reset rework posted here: https://www.spinics.net/lists/arm-kernel/msg875766.html

Finally the both VPUs will have a node the device-tree and be
independent from v4l2 point of view.

./v4l2-compliance -m 1 
v4l2-compliance 1.21.0-4705, 64 bits, 64-bit time_t
v4l2-compliance SHA: 733f7a54f79d 2021-02-03 08:25:49

Compliance test for hantro-vpu device /dev/media1:

Media Driver Info:
	Driver name      : hantro-vpu
	Model            : hantro-vpu
	Serial           : 
	Bus info         : platform: hantro-vpu
	Media version    : 5.11.0
	Hardware revision: 0x00000000 (0)
	Driver version   : 5.11.0

Required ioctls:
	test MEDIA_IOC_DEVICE_INFO: OK
	test invalid ioctls: OK

Allow for multiple opens:
	test second /dev/media1 open: OK
	test MEDIA_IOC_DEVICE_INFO: OK
	test for unlimited opens: OK

Media Controller ioctls:
	test MEDIA_IOC_G_TOPOLOGY: OK
	Entities: 3 Interfaces: 1 Pads: 4 Links: 4
	test MEDIA_IOC_ENUM_ENTITIES/LINKS: OK
	test MEDIA_IOC_SETUP_LINK: OK

Total for hantro-vpu device /dev/media1: 8, Succeeded: 8, Failed: 0, Warnings: 0
--------------------------------------------------------------------------------
Compliance test for hantro-vpu device /dev/video1:

Driver Info:
	Driver name      : hantro-vpu
	Card type        : nxp,imx8mq-vpu-g2-dec
	Bus info         : platform: hantro-vpu
	Driver version   : 5.11.0
	Capabilities     : 0x84204000
		Video Memory-to-Memory Multiplanar
		Streaming
		Extended Pix Format
		Device Capabilities
	Device Caps      : 0x04204000
		Video Memory-to-Memory Multiplanar
		Streaming
		Extended Pix Format
Media Driver Info:
	Driver name      : hantro-vpu
	Model            : hantro-vpu
	Serial           : 
	Bus info         : platform: hantro-vpu
	Media version    : 5.11.0
	Hardware revision: 0x00000000 (0)
	Driver version   : 5.11.0
Interface Info:
	ID               : 0x0300000c
	Type             : V4L Video
Entity Info:
	ID               : 0x00000001 (1)
	Name             : nxp,imx8mq-vpu-g2-dec-source
	Function         : V4L2 I/O
	Pad 0x01000002   : 0: Source
	  Link 0x02000008: to remote pad 0x1000004 of entity 'nxp,imx8mq-vpu-g2-dec-proc': Data, Enabled, Immutable

Required ioctls:
	test MC information (see 'Media Driver Info' above): OK
	test VIDIOC_QUERYCAP: OK
	test invalid ioctls: OK

Allow for multiple opens:
	test second /dev/video1 open: OK
	test VIDIOC_QUERYCAP: OK
	test VIDIOC_G/S_PRIORITY: OK
	test for unlimited opens: OK

Debug ioctls:
	test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
	test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls:
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
	test VIDIOC_QUERYCTRL: OK
	test VIDIOC_G/S_CTRL: OK
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 10 Private Controls: 1

Format ioctls:
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
	test VIDIOC_G/S_PARM: OK (Not Supported)
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK
	test VIDIOC_TRY_FMT: OK
	test VIDIOC_S_FMT: OK
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK (Not Supported)

Codec ioctls:
	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
	test VIDIOC_EXPBUF: OK
	test Requests: OK

Total for hantro-vpu device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0

Grand Total for hantro-vpu device /dev/media1: 54, Succeeded: 54, Failed: 0, Warnings: 0

Benjamin
 
Benjamin Gaignard (18):
  include: media: hevc: Add scaling and decode params controls
  media: hantro: Define HEVC codec profiles and supported features
  arm64: dts: imx8mq-evk: add reserve memory node for CMA region
  media: hevc: add structures for hevc codec
  media: controls: Add control for HEVC codec
  media: hantro: Make sure that ctx->codex_ops is set
  media: hantro: Add a field to distinguish the hardware versions
  media: hantro: Add HEVC structures
  media: hantro: move hantro_needs_postproc function
  media: hantro: Add helper functions for buffer information
  media: hantro: Add helper function for auxiliary buffers allocation
  media: uapi: Add a control for HANTRO driver
  media: hantro: Introduce G2/HEVC decoder
  media: hantro: add G2 support to postproc
  media: hantro: handle V4L2_PIX_FMT_HEVC_SLICE control
  media: hantro: IMX8M: add variant for G2/HEVC codec
  dt-bindings: media: nxp,imx8mq-vpu: Update bindings
  arm64: dts: imx8mq: Add node to G2 hardware

 .../bindings/media/nxp,imx8mq-vpu.yaml        |  54 +-
 arch/arm64/boot/dts/freescale/imx8mq-evk.dts  |  15 +
 arch/arm64/boot/dts/freescale/imx8mq.dtsi     |  43 +-
 drivers/media/v4l2-core/v4l2-ctrls.c          |  36 +-
 drivers/staging/media/hantro/Makefile         |   2 +
 drivers/staging/media/hantro/hantro.h         |  58 +-
 drivers/staging/media/hantro/hantro_drv.c     | 110 ++-
 .../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      |  50 ++
 .../staging/media/hantro/hantro_postproc.c    |  52 +-
 drivers/staging/media/hantro/hantro_v4l2.c    |   3 +-
 drivers/staging/media/hantro/imx8m_vpu_hw.c   |  95 ++-
 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  |   6 +-
 include/media/hevc-ctrls.h                    |  56 +-
 include/uapi/linux/hantro-v4l2-controls.h     |  20 +
 include/uapi/linux/v4l2-controls.h            |   5 +
 21 files changed, 1654 insertions(+), 69 deletions(-)
 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
 create mode 100644 include/uapi/linux/hantro-v4l2-controls.h

Comments

Greg Kroah-Hartman Feb. 17, 2021, 8:36 a.m. UTC | #1
On Wed, Feb 17, 2021 at 09:28:09AM +0100, Benjamin Gaignard wrote:
> 
> Le 17/02/2021 à 09:08, Greg KH a écrit :
> > On Wed, Feb 17, 2021 at 09:02:48AM +0100, Benjamin Gaignard wrote:
> > > The IMX8MQ got two VPUs but until now only G1 has been enabled.
> > > This series aim to add the second VPU (aka G2) and provide basic
> > > HEVC decoding support.
> > Why are you adding this directly to drivers/staging/media/ and not
> > drivers/media/?  Why can't this just go to the main location and not
> > live in staging?
> 
> G2/HEVC is added inside the already exiting Hantro driver, it is "just"
> an other codec from Hantro driver point of view.
> In addition of that v4l2-hevc uAPI is still unstable.
> One goal of this series is to have one more consumer of this v4l2-hevc
> uAPI so maybe we can claim it to be stable enough to move away from staging
> and then do the same for Hantro driver. That would be a great achievement !

I know I do not like seeing new additions/features/whatever being added
to staging drivers as that encourages people to do new stuff on them
without doing the real work needed to get them out of staging.

So what is preventing the existing driver from getting out of staging
now?

And how are you all creating new userspace apis for staging drivers to
the v4l layer?  What happens when you export something new and then
userspace starts to rely on it and then you change it?

Anyway, the media staging drivers are on their own, I don't touch them,
it just feels odd to me...

thanks,

greg k-h
Hans Verkuil Feb. 17, 2021, 9:10 a.m. UTC | #2
On 17/02/2021 09:36, Greg KH wrote:
> On Wed, Feb 17, 2021 at 09:28:09AM +0100, Benjamin Gaignard wrote:
>>
>> Le 17/02/2021 à 09:08, Greg KH a écrit :
>>> On Wed, Feb 17, 2021 at 09:02:48AM +0100, Benjamin Gaignard wrote:
>>>> The IMX8MQ got two VPUs but until now only G1 has been enabled.
>>>> This series aim to add the second VPU (aka G2) and provide basic
>>>> HEVC decoding support.
>>> Why are you adding this directly to drivers/staging/media/ and not
>>> drivers/media/?  Why can't this just go to the main location and not
>>> live in staging?
>>
>> G2/HEVC is added inside the already exiting Hantro driver, it is "just"
>> an other codec from Hantro driver point of view.
>> In addition of that v4l2-hevc uAPI is still unstable.
>> One goal of this series is to have one more consumer of this v4l2-hevc
>> uAPI so maybe we can claim it to be stable enough to move away from staging
>> and then do the same for Hantro driver. That would be a great achievement !
> 
> I know I do not like seeing new additions/features/whatever being added
> to staging drivers as that encourages people to do new stuff on them
> without doing the real work needed to get them out of staging.

In order to support a specific codec (MPEG-2, H.264, HEVC, VP8, etc.) for
stateless codec hardware like the hantro, V4L2 controls need to be defined.
The contents of these controls is derived directly from the underlying codec
standards, but it is quite difficult to get this right with the first attempt,
since these standards are very complex.

So we went for the strategy of keeping these drivers in staging to make it
easy to work on, while keeping the APIs for each codec private (i.e., they are
not exposed in include/uapi/linux).

Once we have sufficient confidence in the API for a specific codec we move
it to uapi and thus fix the API. We also renumber the control IDs at that
time to avoid any confusion between the staging version and the final version.

We did that for H.264 and I hope we can soon do the same for MPEG-2 and VP8.

HEVC is definitely not ready for that yet.

The key phrase is 'sufficient confidence': one requirement is that it is supported
by at least two drivers to be reasonably certain the API doesn't contain any HW
specific stuff, and it passes test suites and review by codec experts.

All this is actively being worked on, so this is very much alive, but it is
complex and time consuming.

> So what is preventing the existing driver from getting out of staging
> now?

Once MPEG-2 and VP8 are finalized it is probably time to move these drivers
out of staging, while still keeping the HEVC API part private.

> 
> And how are you all creating new userspace apis for staging drivers to
> the v4l layer?  What happens when you export something new and then
> userspace starts to rely on it and then you change it?

Nothing is exported. So if userspace want to use it they have to manually
copy headers from include/media to their application.

> 
> Anyway, the media staging drivers are on their own, I don't touch them,
> it just feels odd to me...

It's an unusual situation. But putting the drivers in staging and keeping
the codec API headers private turns out to be the most effective way to
develop this.

Regards,

	Hans

PS: stateful vs stateless decoders: stateful decoders are fully supported
today: you just feed the decoder the compressed stream and the decoded frames
are produced by the firmware/hardware. I.e. the HW takes care of the decoder
state. Stateless decoders require you to pass the compressed frame + decoder
state to the hardware, so they do not keep track of the decoder state, that
needs to be done in software. And that requires structures to be created that
store the state, which luckily can be derived from the codec standards.
Ezequiel Garcia Feb. 17, 2021, 8:15 p.m. UTC | #3
On Wed, 2021-02-17 at 09:02 +0100, Benjamin Gaignard wrote:
> Decoders hardware blocks could exist in multiple versions: add
> a field to distinguish them at runtime.
> Keep the default behavoir to be G1 hardware.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  drivers/staging/media/hantro/hantro.h     | 5 +++++
>  drivers/staging/media/hantro/hantro_drv.c | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
> index bde65231f22f..2a566dfc2fe3 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -36,6 +36,9 @@ struct hantro_codec_ops;
>  #define HANTRO_H264_DECODER    BIT(18)
>  #define HANTRO_DECODERS                0xffff0000
>  
> +#define HANTRO_G1_REV          0x6731
> +#define HANTRO_G2_REV          0x6732
> +
>  /**
>   * struct hantro_irq - irq handler and name
>   *
> @@ -170,6 +173,7 @@ hantro_vdev_to_func(struct video_device *vdev)
>   * @enc_base:          Mapped address of VPU encoder register for convenience.
>   * @dec_base:          Mapped address of VPU decoder register for convenience.
>   * @ctrl_base:         Mapped address of VPU control block.
> + * @core_hw_dec_rev    Runtime detected HW decoder core revision
>   * @vpu_mutex:         Mutex to synchronize V4L2 calls.
>   * @irqlock:           Spinlock to synchronize access to data structures
>   *                     shared with interrupt handlers.
> @@ -189,6 +193,7 @@ struct hantro_dev {
>         void __iomem *enc_base;
>         void __iomem *dec_base;
>         void __iomem *ctrl_base;
> +       u32 core_hw_dec_rev;
>  
>         struct mutex vpu_mutex; /* video_device lock */
>         spinlock_t irqlock;
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 0570047c7fa0..e1443c394f62 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -840,6 +840,8 @@ static int hantro_probe(struct platform_device *pdev)
>         }
>         vpu->enc_base = vpu->reg_bases[0] + vpu->variant->enc_offset;
>         vpu->dec_base = vpu->reg_bases[0] + vpu->variant->dec_offset;
> +       /* by default decoder is G1 */
> +       vpu->core_hw_dec_rev = HANTRO_G1_REV;
>  

What's the use of this field? Can't we simply rely on the compatible string?

Thanks,
Ezequiel
Ezequiel Garcia Feb. 17, 2021, 8:42 p.m. UTC | #4
Hi Benjamin,

On Wed, 2021-02-17 at 09:02 +0100, Benjamin Gaignard wrote:
> Add helper functions to allocate and free auxiliary buffers.
> These buffers aren't for frames but are needed by the hardware
> to store scaling matrix, tiles size, border filters etc...
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  drivers/staging/media/hantro/hantro.h | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
> index a9b80b2c9124..7f842edbc341 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -446,6 +446,30 @@ hantro_get_dec_buf(struct hantro_ctx *ctx, struct vb2_buffer *vb)
>         return vb2_plane_vaddr(vb, 0);
>  }
>  
> +static inline int
> +hantro_aux_buf_alloc(struct hantro_dev *vpu,
> +                    struct hantro_aux_buf *buf, size_t size)
> +{

Can you add convert the dma_alloc_ calls in the driver,
and squash it in this patch?

I.e. hantro_h264_dec_init, hantro_vp8_dec_init, etc.

Thanks!
Ezequiel

> +       buf->cpu = dma_alloc_coherent(vpu->dev, size, &buf->dma, GFP_KERNEL);
> +       if (!buf->cpu)
> +               return -ENOMEM;
> +
> +       buf->size = size;
> +       return 0;
> +}
> +
> +static inline void
> +hantro_aux_buf_free(struct hantro_dev *vpu,
> +                   struct hantro_aux_buf *buf)
> +{
> +       if (buf->cpu)
> +               dma_free_coherent(vpu->dev, buf->size, buf->cpu, buf->dma);
> +
> +       buf->cpu = NULL;
> +       buf->dma = 0;
> +       buf->size = 0;
> +}
> +
>  void hantro_postproc_disable(struct hantro_ctx *ctx);
>  void hantro_postproc_enable(struct hantro_ctx *ctx);
>  void hantro_postproc_free(struct hantro_ctx *ctx);
Dan Carpenter Feb. 18, 2021, 10:45 a.m. UTC | #5
On Wed, Feb 17, 2021 at 04:39:49PM -0300, Ezequiel Garcia wrote:
> Hi Benjamin,
> 
> On Wed, 2021-02-17 at 09:02 +0100, Benjamin Gaignard wrote:
> > Define allocation range for the default CMA region.
> > 
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> 
> Despite it seems like I signed-off this one...
> 

I've been puzzled by this as well.  :P

Signed-off-by means you either wrote the patch or you handled it in some
way.  And it is intended as a legally binding document that you didn't
sneak in any copyrighted code from SCO UNIXWARE (etc).  So like maybe
the authors snuck some in or maybe a maintainer took the patch and
sneaked some unixware code in.

Obviously if you sign the code, that counts as an Ack and Review as well
because maintainers are going to only merge stuff if they've looked it
over a bit.  But the main thing is that it means you didn't didn't
violate any copyrights.

regards,
dan carpenter
Benjamin Gaignard Feb. 18, 2021, 2:48 p.m. UTC | #6
Le 17/02/2021 à 23:48, Rob Herring a écrit :
> On Wed, Feb 17, 2021 at 09:03:05AM +0100, Benjamin Gaignard wrote:
>> The introduction on HEVC decoder lead to update the bindings
>> to support it.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
>> ---
>>   .../bindings/media/nxp,imx8mq-vpu.yaml        | 54 ++++++++++++-------
>>   1 file changed, 36 insertions(+), 18 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
>> index 762be3f96ce9..468435c70eef 100644
>> --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
>> @@ -15,24 +15,25 @@ description:
>>   
>>   properties:
>>     compatible:
>> -    const: nxp,imx8mq-vpu
>> +    enum:
>> +      - nxp,imx8mq-vpu
>> +      - nxp,imx8mq-vpu-g2
>>   
>>     reg:
>> -    maxItems: 3
>> +    maxItems: 1
>>   
>>     reg-names:
>> -    items:
>> -      - const: g1
>> -      - const: g2
>> -      - const: ctrl
>> +    enum:
>> +      - g1
>> +      - g2
> This isn't a compatible change. You need to state why that's okay if it
> is okay.

I will change the commit message to this in the next version:
The current bindings seem to make the assumption that the
two VPUs hardware blocks (G1 and G2) are only one set of
registers.
After implementing the VPU reset driver and G2 decoder driver
it shows that all the VPUs are independent and don't need to
know about the registers of the other blocks.
Remove from the bindings the need to set all blocks register
but keep reg-names property because removing it from the driver
may affect other variants.

Benjamin

>
>>   
>>     interrupts:
>> -    maxItems: 2
>> +    maxItems: 1
>>   
>>     interrupt-names:
>> -    items:
>> -      - const: g1
>> -      - const: g2
>> +    enum:
>> +      - g1
>> +      - g2
>>   
>>     clocks:
>>       maxItems: 3
>> @@ -46,6 +47,9 @@ properties:
>>     power-domains:
>>       maxItems: 1
>>   
>> +  resets:
>> +    maxItems: 1
>> +
>>   required:
>>     - compatible
>>     - reg
>> @@ -54,6 +58,7 @@ required:
>>     - interrupt-names
>>     - clocks
>>     - clock-names
>> +  - resets
>>   
>>   additionalProperties: false
>>   
>> @@ -61,19 +66,32 @@ examples:
>>     - |
>>           #include <dt-bindings/clock/imx8mq-clock.h>
>>           #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +        #include <dt-bindings/reset/imx8mq-vpu-reset.h>
>>   
>> -        vpu: video-codec@38300000 {
>> +        vpu_g1: video-codec@38300000 {
>>                   compatible = "nxp,imx8mq-vpu";
>> -                reg = <0x38300000 0x10000>,
>> -                      <0x38310000 0x10000>,
>> -                      <0x38320000 0x10000>;
>> -                reg-names = "g1", "g2", "ctrl";
>> -                interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
>> -                             <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
>> -                interrupt-names = "g1", "g2";
>> +                reg = <0x38300000 0x10000>;
>> +                reg-names = "g1";
>> +                interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
>> +                interrupt-names = "g1";
>> +                clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
>> +                         <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
>> +                         <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
>> +                clock-names = "g1", "g2", "bus";
>> +                power-domains = <&pgc_vpu>;
>> +                resets = <&vpu_reset IMX8MQ_RESET_VPU_RESET_G1>;
>> +        };
>> +
>> +        vpu_g2: video-codec@38310000 {
>> +                compatible = "nxp,imx8mq-vpu-g2";
>> +                reg = <0x38310000 0x10000>;
>> +                reg-names = "g2";
>> +                interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
>> +                interrupt-names = "g2";
>>                   clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
>>                            <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
>>                            <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
>>                   clock-names = "g1", "g2", "bus";
>>                   power-domains = <&pgc_vpu>;
>> +                resets = <&vpu_reset IMX8MQ_RESET_VPU_RESET_G2>;
>>           };
>> -- 
>> 2.25.1
>>
Benjamin Gaignard Feb. 18, 2021, 2:51 p.m. UTC | #7
Le 17/02/2021 à 21:42, Ezequiel Garcia a écrit :
> Hi Benjamin,
>
> On Wed, 2021-02-17 at 09:02 +0100, Benjamin Gaignard wrote:
>> Add helper functions to allocate and free auxiliary buffers.
>> These buffers aren't for frames but are needed by the hardware
>> to store scaling matrix, tiles size, border filters etc...
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
>> ---
>>   drivers/staging/media/hantro/hantro.h | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
>> index a9b80b2c9124..7f842edbc341 100644
>> --- a/drivers/staging/media/hantro/hantro.h
>> +++ b/drivers/staging/media/hantro/hantro.h
>> @@ -446,6 +446,30 @@ hantro_get_dec_buf(struct hantro_ctx *ctx, struct vb2_buffer *vb)
>>          return vb2_plane_vaddr(vb, 0);
>>   }
>>   
>> +static inline int
>> +hantro_aux_buf_alloc(struct hantro_dev *vpu,
>> +                    struct hantro_aux_buf *buf, size_t size)
>> +{
> Can you add convert the dma_alloc_ calls in the driver,
> and squash it in this patch?
>
> I.e. hantro_h264_dec_init, hantro_vp8_dec_init, etc.

Sure I will that in v2.
Benjamin

>
> Thanks!
> Ezequiel
>
>> +       buf->cpu = dma_alloc_coherent(vpu->dev, size, &buf->dma, GFP_KERNEL);
>> +       if (!buf->cpu)
>> +               return -ENOMEM;
>> +
>> +       buf->size = size;
>> +       return 0;
>> +}
>> +
>> +static inline void
>> +hantro_aux_buf_free(struct hantro_dev *vpu,
>> +                   struct hantro_aux_buf *buf)
>> +{
>> +       if (buf->cpu)
>> +               dma_free_coherent(vpu->dev, buf->size, buf->cpu, buf->dma);
>> +
>> +       buf->cpu = NULL;
>> +       buf->dma = 0;
>> +       buf->size = 0;
>> +}
>> +
>>   void hantro_postproc_disable(struct hantro_ctx *ctx);
>>   void hantro_postproc_enable(struct hantro_ctx *ctx);
>>   void hantro_postproc_free(struct hantro_ctx *ctx);
>
>