Re: [PATCH v4 05/11] media: hantro: Add a field to distinguish the hardware versions

[Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > ---
> >   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;