| Message ID | 20210303113952.178519-6-benjamin.gaignard@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add HANTRO G2/HEVC decoder support for IMX8MQ | expand |
On Wed, 2021-03-03 at 12:39 +0100, Benjamin Gaignard wrote: > Decoders hardware blocks could exist in multiple versions: add > a field to distinguish them at runtime. > G2 hardware block doesn't have postprocessor hantro_needs_postproc > function should always returns false in for this hardware. > hantro_needs_postproc function becoming to much complex to > stay inline in .h file move it to .c file. > Note that I already questioned this patch before: https://lkml.org/lkml/2021/2/17/722 I think it's better to rely on of_device_id.data for this type of thing. In particular, I was expecting that just using hantro_variant.postproc_regs would be enough. Can you try if that works and avoid reading swreg(0) and probing the hardware core? Thanks! Ezequiel > Keep the default behavoir to be G1 hardware. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > drivers/staging/media/hantro/hantro.h | 13 +++++++------ > drivers/staging/media/hantro/hantro_drv.c | 2 ++ > drivers/staging/media/hantro/hantro_postproc.c | 17 +++++++++++++++++ > 3 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h > index a76a0d79db9f..05876e426419 100644 > --- a/drivers/staging/media/hantro/hantro.h > +++ b/drivers/staging/media/hantro/hantro.h > @@ -37,6 +37,9 @@ struct hantro_codec_ops; > #define HANTRO_HEVC_DECODER BIT(19) > #define HANTRO_DECODERS 0xffff0000 > > +#define HANTRO_G1_REV 0x6731 > +#define HANTRO_G2_REV 0x6732 > + > /** > * struct hantro_irq - irq handler and name > * > @@ -171,6 +174,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. > @@ -190,6 +194,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; > @@ -412,12 +417,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_drv.c b/drivers/staging/media/hantro/hantro_drv.c > index f0b68e16fcc0..e3e6df28f470 100644 > --- a/drivers/staging/media/hantro/hantro_drv.c > +++ b/drivers/staging/media/hantro/hantro_drv.c > @@ -836,6 +836,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; > > ret = dma_set_coherent_mask(vpu->dev, DMA_BIT_MASK(32)); > if (ret) { > diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c > index 6d2a8f2a8f0b..050880f720d6 100644 > --- a/drivers/staging/media/hantro/hantro_postproc.c > +++ b/drivers/staging/media/hantro/hantro_postproc.c > @@ -50,6 +50,23 @@ 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->core_hw_dec_rev == HANTRO_G1_REV):q > + return fmt->fourcc != V4L2_PIX_FMT_NV12; > + > + if (vpu->core_hw_dec_rev == HANTRO_G2_REV) > + return false; > + > + return false; > +} > + > void hantro_postproc_enable(struct hantro_ctx *ctx) > { > struct hantro_dev *vpu = ctx->dev;
Le 03/03/2021 à 23:05, Ezequiel Garcia a écrit : > On Wed, 2021-03-03 at 12:39 +0100, Benjamin Gaignard wrote: >> Decoders hardware blocks could exist in multiple versions: add >> a field to distinguish them at runtime. >> G2 hardware block doesn't have postprocessor hantro_needs_postproc >> function should always returns false in for this hardware. >> hantro_needs_postproc function becoming to much complex to >> stay inline in .h file move it to .c file. >> > Note that I already questioned this patch before: > > https://lkml.org/lkml/2021/2/17/722 > > I think it's better to rely on of_device_id.data for this > type of thing. > > In particular, I was expecting that just using > hantro_variant.postproc_regs would be enough. > > Can you try if that works and avoid reading swreg(0) > and probing the hardware core? I have found a way to remove this: if the variant doesn't define post processor formats, needs_postproc function will always returns false and that what the only useful usage of this version field. Benjamin > > Thanks! > Ezequiel > >> Keep the default behavoir to be G1 hardware. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> drivers/staging/media/hantro/hantro.h | 13 +++++++------ >> drivers/staging/media/hantro/hantro_drv.c | 2 ++ >> drivers/staging/media/hantro/hantro_postproc.c | 17 +++++++++++++++++ >> 3 files changed, 26 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h >> index a76a0d79db9f..05876e426419 100644 >> --- a/drivers/staging/media/hantro/hantro.h >> +++ b/drivers/staging/media/hantro/hantro.h >> @@ -37,6 +37,9 @@ struct hantro_codec_ops; >> #define HANTRO_HEVC_DECODER BIT(19) >> #define HANTRO_DECODERS 0xffff0000 >> >> +#define HANTRO_G1_REV 0x6731 >> +#define HANTRO_G2_REV 0x6732 >> + >> /** >> * struct hantro_irq - irq handler and name >> * >> @@ -171,6 +174,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. >> @@ -190,6 +194,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; >> @@ -412,12 +417,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_drv.c b/drivers/staging/media/hantro/hantro_drv.c >> index f0b68e16fcc0..e3e6df28f470 100644 >> --- a/drivers/staging/media/hantro/hantro_drv.c >> +++ b/drivers/staging/media/hantro/hantro_drv.c >> @@ -836,6 +836,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; >> >> ret = dma_set_coherent_mask(vpu->dev, DMA_BIT_MASK(32)); >> if (ret) { >> diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c >> index 6d2a8f2a8f0b..050880f720d6 100644 >> --- a/drivers/staging/media/hantro/hantro_postproc.c >> +++ b/drivers/staging/media/hantro/hantro_postproc.c >> @@ -50,6 +50,23 @@ 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->core_hw_dec_rev == HANTRO_G1_REV):q >> + return fmt->fourcc != V4L2_PIX_FMT_NV12; >> + >> + if (vpu->core_hw_dec_rev == HANTRO_G2_REV) >> + return false; >> + >> + return false; >> +} >> + >> void hantro_postproc_enable(struct hantro_ctx *ctx) >> { >> struct hantro_dev *vpu = ctx->dev; > >
diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h index a76a0d79db9f..05876e426419 100644 --- a/drivers/staging/media/hantro/hantro.h +++ b/drivers/staging/media/hantro/hantro.h @@ -37,6 +37,9 @@ struct hantro_codec_ops; #define HANTRO_HEVC_DECODER BIT(19) #define HANTRO_DECODERS 0xffff0000 +#define HANTRO_G1_REV 0x6731 +#define HANTRO_G2_REV 0x6732 + /** * struct hantro_irq - irq handler and name * @@ -171,6 +174,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. @@ -190,6 +194,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; @@ -412,12 +417,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_drv.c b/drivers/staging/media/hantro/hantro_drv.c index f0b68e16fcc0..e3e6df28f470 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c @@ -836,6 +836,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; ret = dma_set_coherent_mask(vpu->dev, DMA_BIT_MASK(32)); if (ret) { diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c index 6d2a8f2a8f0b..050880f720d6 100644 --- a/drivers/staging/media/hantro/hantro_postproc.c +++ b/drivers/staging/media/hantro/hantro_postproc.c @@ -50,6 +50,23 @@ 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->core_hw_dec_rev == HANTRO_G1_REV) + return fmt->fourcc != V4L2_PIX_FMT_NV12; + + if (vpu->core_hw_dec_rev == HANTRO_G2_REV) + return false; + + return false; +} + void hantro_postproc_enable(struct hantro_ctx *ctx) { struct hantro_dev *vpu = ctx->dev;
Decoders hardware blocks could exist in multiple versions: add a field to distinguish them at runtime. G2 hardware block doesn't have postprocessor hantro_needs_postproc function should always returns false in for this hardware. hantro_needs_postproc function becoming to much complex to stay inline in .h file move it to .c file. Keep the default behavoir to be G1 hardware. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- drivers/staging/media/hantro/hantro.h | 13 +++++++------ drivers/staging/media/hantro/hantro_drv.c | 2 ++ drivers/staging/media/hantro/hantro_postproc.c | 17 +++++++++++++++++ 3 files changed, 26 insertions(+), 6 deletions(-)