diff mbox series

[v2,05/12] staging: media: rkvdec: Add SPS structure to internal context

Message ID 20230101-patch-series-v2-6-2-rc1-v2-5-fa1897efac14@collabora.com (mailing list archive)
State New, archived
Headers show
Series RkVDEC HEVC driver | expand

Commit Message

Sebastian Fricke Jan. 12, 2023, 12:56 p.m. UTC
Prepare storing the SPS structure for HEVC & H264 in the internal
context of the rkvdec instance. This structure is used to figure out
which capture queue format is appropriate for decoding.

Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Ezequiel Garcia Jan. 12, 2023, 3:02 p.m. UTC | #1
Hi Sebastian,

On Thu, Jan 12, 2023 at 9:56 AM Sebastian Fricke
<sebastian.fricke@collabora.com> wrote:
>
> Prepare storing the SPS structure for HEVC & H264 in the internal
> context of the rkvdec instance. This structure is used to figure out
> which capture queue format is appropriate for decoding.
>
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> index 633335ebb9c4..332126e7b812 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.h
> +++ b/drivers/staging/media/rkvdec/rkvdec.h
> @@ -105,6 +105,7 @@ struct rkvdec_ctx {
>         struct v4l2_ctrl_handler ctrl_hdl;
>         struct rkvdec_dev *dev;
>         void *priv;
> +       void *sps;

I don't really like re-caching the SPS in the context,
since all the controls are already stored in the context,
via the ctrl_handler.

See hantro_get_ctrl().

Duplicating state can lead to problems and even if we get it
right this time, will be hard to maintain.

Thanks,
Ezequiel

>  };
>
>  static inline struct rkvdec_ctx *fh_to_rkvdec_ctx(struct v4l2_fh *fh)
>
> --
> 2.25.1
Andrzej Pietrasiewicz Jan. 16, 2023, 10 a.m. UTC | #2
Hi Sebastian,

W dniu 12.01.2023 o 16:02, Ezequiel Garcia pisze:
> Hi Sebastian,
> 
> On Thu, Jan 12, 2023 at 9:56 AM Sebastian Fricke
> <sebastian.fricke@collabora.com> wrote:
>>
>> Prepare storing the SPS structure for HEVC & H264 in the internal
>> context of the rkvdec instance. This structure is used to figure out
>> which capture queue format is appropriate for decoding.
>>
>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>> ---
>>   drivers/staging/media/rkvdec/rkvdec.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>> index 633335ebb9c4..332126e7b812 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>> @@ -105,6 +105,7 @@ struct rkvdec_ctx {
>>          struct v4l2_ctrl_handler ctrl_hdl;
>>          struct rkvdec_dev *dev;
>>          void *priv;
>> +       void *sps;
> 
> I don't really like re-caching the SPS in the context,
> since all the controls are already stored in the context,
> via the ctrl_handler.
> 
> See hantro_get_ctrl().
> 
> Duplicating state can lead to problems and even if we get it
> right this time, will be hard to maintain.
> 

Also, just looking at this patch is seems a bit too fine-grained
a change to add a member to a driver-internal struct and have no
users at the same time. OTOH Ezequiel has easily spotted it :D

Andrzej

> Thanks,
> Ezequiel
> 
>>   };
>>
>>   static inline struct rkvdec_ctx *fh_to_rkvdec_ctx(struct v4l2_fh *fh)
>>
>> --
>> 2.25.1
diff mbox series

Patch

diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
index 633335ebb9c4..332126e7b812 100644
--- a/drivers/staging/media/rkvdec/rkvdec.h
+++ b/drivers/staging/media/rkvdec/rkvdec.h
@@ -105,6 +105,7 @@  struct rkvdec_ctx {
 	struct v4l2_ctrl_handler ctrl_hdl;
 	struct rkvdec_dev *dev;
 	void *priv;
+	void *sps;
 };
 
 static inline struct rkvdec_ctx *fh_to_rkvdec_ctx(struct v4l2_fh *fh)