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

[Ezequiel Garcia <ezequiel@collabora.com>]

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;