| Message ID | 20210217080306.157876-1-benjamin.gaignard@collabora.com |
|---|---|
| Headers | show |
| Series | Add HANTRO G2/HEVC decoder support for IMX8MQ | expand |
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
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.
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
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);
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
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 >>
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); > >