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

From: Benjamin Gaignard <benjamin.gaignard@collabora.com>
To: Ezequiel Garcia <ezequiel@collabora.com>,
	p.zabel@pengutronix.de, mchehab@kernel.org, robh+dt@kernel.org,
	shawnguo@kernel.org, s.hauer@pengutronix.de,
	kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
	gregkh@linuxfoundation.org, mripard@kernel.org,
	paul.kocialkowski@bootlin.com, wens@csie.org,
	jernej.skrabec@siol.net, peng.fan@nxp.com,
	hverkuil-cisco@xs4all.nl, dan.carpenter@oracle.com
Cc: linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, kernel@collabora.com
Subject: Re: [PATCH v4 05/11] media: hantro: Add a field to distinguish the hardware versions
Date: Fri, 5 Mar 2021 10:27:44 +0100	[thread overview]
Message-ID: <23f62276-237d-1161-259a-84748db7365b@collabora.com> (raw)
In-Reply-To: <32899bc605ae7173c29b25a396e21d7fad32d4bf.camel@collabora.com>

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