diff mbox series

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

Message ID 20210303113952.178519-6-benjamin.gaignard@collabora.com (mailing list archive)
State New
Headers show
Series Add HANTRO G2/HEVC decoder support for IMX8MQ | expand

Commit Message

Benjamin Gaignard March 3, 2021, 11:39 a.m. UTC
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(-)

Comments

Ezequiel Garcia March 3, 2021, 10:05 p.m. UTC | #1
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;
Benjamin Gaignard March 5, 2021, 9:27 a.m. UTC | #2
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 mbox series

Patch

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;