[RFC PATCH 0/5] Add enum_fmt flag for coded formats with dynamic resolution switching

[RFC PATCH 0/5] Add enum_fmt flag for coded formats with dynamic resolution switching

[Maxime Jourdan]

Hello,

This RFC proposes a new format flag - V4L2_FMT_FLAG_DYN_RESOLUTION - used
to tag coded formats for which the device supports dynamic resolution
switching, via V4L2_EVENT_SOURCE_CHANGE.
This includes the initial "source change" where the device is able to
tell userspace about the coded resolution and the DPB size (which
sometimes translates to V4L2_CID_MIN_BUFFERS_FOR_CAPTURE).
This flag is mainly aimed at stateful decoder drivers.

This RFC is motivated by my development on the amlogic video decoder
driver, which does not support dynamic resolution switching for older
coded formats (MPEG 1/2, MPEG 4 part II, H263). It does however support
it for the newer formats (H264, HEVC, VP9).

The specification regarding stateful video decoders should be amended
to include that, in the absence of this flag for a certain format,
userspace is expected to extract the coded resolution and allocate
a sufficient amount of capture buffers on its own.
I understand that this point may be tricky, since older kernels with
close-to-spec drivers would not have this flag available, yet would
fully support dynamic resolution switching.
However, with the spec not merged in yet, I wanted to have your opinion
on this late addition.

The RFC patches also adds support for this flag for the 4 following
stateful decoder drivers:
 - venus
 - s5p-mfc
 - mtk-vcodec
 - vicodec

Maxime Jourdan (5):
  media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION
  media: venus: vdec: flag OUTPUT formats with
    V4L2_FMT_FLAG_DYN_RESOLUTION
  media: s5p_mfc_dec: flag OUTPUT formats with
    V4L2_FMT_FLAG_DYN_RESOLUTION
  media: mtk-vcodec: flag OUTPUT formats with
    V4L2_FMT_FLAG_DYN_RESOLUTION
  media: vicodec: flag vdec/stateful OUTPUT formats with
    V4L2_FMT_FLAG_DYN_RESOLUTION

 Documentation/media/uapi/v4l/vidioc-enum-fmt.rst   |  7 +++++++
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c |  4 ++++
 drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h |  1 +
 drivers/media/platform/qcom/venus/core.h           |  1 +
 drivers/media/platform/qcom/venus/vdec.c           | 11 +++++++++++
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h    |  1 +
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c       | 13 +++++++++++++
 drivers/media/platform/vicodec/vicodec-core.c      |  2 ++
 include/uapi/linux/videodev2.h                     |  5 +++--
 9 files changed, 43 insertions(+), 2 deletions(-)

-- 
2.21.0

[RFC PATCH 1/5] media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION

[Maxime Jourdan]

Add a enum_fmt format flag to specifically tag coded formats where
dynamic resolution switching is supported by the device.

This is useful for some codec drivers that can't support dynamic
resolution switching for all their listed coded formats. It allows
userspace to know whether it should extract the video parameters itself,
or if it can rely on the device to send V4L2_EVENT_SOURCE_CHANGE when
such changes are detected.

Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com>
---
 Documentation/media/uapi/v4l/vidioc-enum-fmt.rst | 7 +++++++
 include/uapi/linux/videodev2.h                   | 5 +++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
index 822d6730e7d2..92ddd4ddbce2 100644
--- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
+++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
@@ -127,6 +127,13 @@ one until ``EINVAL`` is returned.
       - This format is not native to the device but emulated through
 	software (usually libv4l2), where possible try to use a native
 	format instead for better performance.
+    * - ``V4L2_FMT_FLAG_DYN_RESOLUTION``
+      - 0x0004
+      - Dynamic resolution switching is supported by the device for this
+	coded format. It will notify the user via the event
+	``V4L2_EVENT_SOURCE_CHANGE`` when changes in the video parameters
+	are detected.
+
 
 
 Return Value
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 1050a75fb7ef..834550e20ee7 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -768,8 +768,9 @@ struct v4l2_fmtdesc {
 	__u32		    reserved[4];
 };
 
-#define V4L2_FMT_FLAG_COMPRESSED 0x0001
-#define V4L2_FMT_FLAG_EMULATED   0x0002
+#define V4L2_FMT_FLAG_COMPRESSED	0x0001
+#define V4L2_FMT_FLAG_EMULATED		0x0002
+#define V4L2_FMT_FLAG_DYN_RESOLUTION	0x0004
 
 	/* Frame Size and frame rate enumeration */
 /*
-- 
2.21.0

Re: [RFC PATCH 1/5] media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION

[Tomasz Figa]

Hi Maxime,

On Sun, Jun 9, 2019 at 11:38 PM Maxime Jourdan <mjourdan@baylibre.com> wrote:
>
> Add a enum_fmt format flag to specifically tag coded formats where
> dynamic resolution switching is supported by the device.
>
> This is useful for some codec drivers that can't support dynamic
> resolution switching for all their listed coded formats. It allows
> userspace to know whether it should extract the video parameters itself,
> or if it can rely on the device to send V4L2_EVENT_SOURCE_CHANGE when
> such changes are detected.
>

First of all, thanks for the patch!

Given the aspect of compatibility and also the general preference for
the drivers to actually handle dynamic resolution changes, I'd suggest
inverting the meaning of this flag. With something like
"V4L2_FMT_FLAG_STATIC_RESOLUTION" it would be more of an exception
rather than the default behavior.

Best regards,
Tomasz

Re: [RFC PATCH 1/5] media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION

[Hans Verkuil]

On 6/10/19 5:48 AM, Tomasz Figa wrote:
> Hi Maxime,
> 
> On Sun, Jun 9, 2019 at 11:38 PM Maxime Jourdan <mjourdan@baylibre.com> wrote:
>>
>> Add a enum_fmt format flag to specifically tag coded formats where
>> dynamic resolution switching is supported by the device.
>>
>> This is useful for some codec drivers that can't support dynamic
>> resolution switching for all their listed coded formats. It allows
>> userspace to know whether it should extract the video parameters itself,
>> or if it can rely on the device to send V4L2_EVENT_SOURCE_CHANGE when
>> such changes are detected.
>>
> 
> First of all, thanks for the patch!
> 
> Given the aspect of compatibility and also the general preference for
> the drivers to actually handle dynamic resolution changes, I'd suggest
> inverting the meaning of this flag. With something like
> "V4L2_FMT_FLAG_STATIC_RESOLUTION" it would be more of an exception
> rather than the default behavior.

We had a discussion about that, see this thread: https://lkml.org/lkml/2019/5/31/256

Regards,

	Hans

Re: [RFC PATCH 1/5] media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION

[Maxime Jourdan]

Hi Tomasz,
On Mon, Jun 10, 2019 at 5:48 AM Tomasz Figa <tfiga@chromium.org> wrote:
>
> Hi Maxime,
>
> On Sun, Jun 9, 2019 at 11:38 PM Maxime Jourdan <mjourdan@baylibre.com> wrote:
> >
> > Add a enum_fmt format flag to specifically tag coded formats where
> > dynamic resolution switching is supported by the device.
> >
> > This is useful for some codec drivers that can't support dynamic
> > resolution switching for all their listed coded formats. It allows
> > userspace to know whether it should extract the video parameters itself,
> > or if it can rely on the device to send V4L2_EVENT_SOURCE_CHANGE when
> > such changes are detected.
> >
>
> First of all, thanks for the patch!
>
> Given the aspect of compatibility and also the general preference for
> the drivers to actually handle dynamic resolution changes, I'd suggest
> inverting the meaning of this flag. With something like
> "V4L2_FMT_FLAG_STATIC_RESOLUTION" it would be more of an exception
> rather than the default behavior.
>

This is actually what I did to begin with [0], with the same
reasoning: not supporting dynamic resolution for a certain coded
format is more of an exception than the norm (for decoders).
The patch was ultimately dropped from the meson vdec series after
discussing with Hans, see [0] or the lkml link Hans provided in his
answer.

We have the chance today that stateful decoders in the kernel either
support V4L2_EVENT_SOURCE_CHANGE and dynamic resolution switching for
all their formats, or they don't expose V4L2_EVENT_SOURCE_CHANGE at
all.
While this flag would change the spec, it wouldn't break existing
userspace using close-to-spec drivers like s5p-mfc or mtk-vcodec.

Cheers,
Maxime

[0] https://patchwork.kernel.org/patch/10969829/

> Best regards,
> Tomasz

Re: [RFC PATCH 1/5] media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION

[Tomasz Figa]

On Wed, Jun 12, 2019 at 1:46 AM Maxime Jourdan <mjourdan@baylibre.com> wrote:
>
> Hi Tomasz,
> On Mon, Jun 10, 2019 at 5:48 AM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > Hi Maxime,
> >
> > On Sun, Jun 9, 2019 at 11:38 PM Maxime Jourdan <mjourdan@baylibre.com> wrote:
> > >
> > > Add a enum_fmt format flag to specifically tag coded formats where
> > > dynamic resolution switching is supported by the device.
> > >
> > > This is useful for some codec drivers that can't support dynamic
> > > resolution switching for all their listed coded formats. It allows
> > > userspace to know whether it should extract the video parameters itself,
> > > or if it can rely on the device to send V4L2_EVENT_SOURCE_CHANGE when
> > > such changes are detected.
> > >
> >
> > First of all, thanks for the patch!
> >
> > Given the aspect of compatibility and also the general preference for
> > the drivers to actually handle dynamic resolution changes, I'd suggest
> > inverting the meaning of this flag. With something like
> > "V4L2_FMT_FLAG_STATIC_RESOLUTION" it would be more of an exception
> > rather than the default behavior.
> >
>
> This is actually what I did to begin with [0], with the same
> reasoning: not supporting dynamic resolution for a certain coded
> format is more of an exception than the norm (for decoders).
> The patch was ultimately dropped from the meson vdec series after
> discussing with Hans, see [0] or the lkml link Hans provided in his
> answer.
>
> We have the chance today that stateful decoders in the kernel either
> support V4L2_EVENT_SOURCE_CHANGE and dynamic resolution switching for
> all their formats, or they don't expose V4L2_EVENT_SOURCE_CHANGE at
> all.
> While this flag would change the spec, it wouldn't break existing
> userspace using close-to-spec drivers like s5p-mfc or mtk-vcodec.

Fair enough. Feel free to add my Reviewed-by, after Hans's comments are fixed.

Best regards,
Tomasz

>
> Cheers,
> Maxime
>
> [0] https://patchwork.kernel.org/patch/10969829/
>
> > Best regards,
> > Tomasz

Re: [RFC PATCH 1/5] media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION

[Hans Verkuil]

On 6/9/19 4:38 PM, Maxime Jourdan wrote:
> Add a enum_fmt format flag to specifically tag coded formats where
> dynamic resolution switching is supported by the device.
> 
> This is useful for some codec drivers that can't support dynamic

can't -> can

> resolution switching for all their listed coded formats. It allows
> userspace to know whether it should extract the video parameters itself,
> or if it can rely on the device to send V4L2_EVENT_SOURCE_CHANGE when
> such changes are detected.
> 
> Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com>
> ---
>  Documentation/media/uapi/v4l/vidioc-enum-fmt.rst | 7 +++++++
>  include/uapi/linux/videodev2.h                   | 5 +++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> index 822d6730e7d2..92ddd4ddbce2 100644
> --- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> @@ -127,6 +127,13 @@ one until ``EINVAL`` is returned.
>        - This format is not native to the device but emulated through
>  	software (usually libv4l2), where possible try to use a native
>  	format instead for better performance.
> +    * - ``V4L2_FMT_FLAG_DYN_RESOLUTION``
> +      - 0x0004
> +      - Dynamic resolution switching is supported by the device for this
> +	coded format. It will notify the user via the event

I'd say 'compressed bitstream format (aka coded format)'.

Also mention that this flag can only be used in combination with the
COMPRESSED flag, since this applies to compressed formats only.

Regards,

	Hans

> +	``V4L2_EVENT_SOURCE_CHANGE`` when changes in the video parameters
> +	are detected.
> +
>  
>  
>  Return Value
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 1050a75fb7ef..834550e20ee7 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -768,8 +768,9 @@ struct v4l2_fmtdesc {
>  	__u32		    reserved[4];
>  };
>  
> -#define V4L2_FMT_FLAG_COMPRESSED 0x0001
> -#define V4L2_FMT_FLAG_EMULATED   0x0002
> +#define V4L2_FMT_FLAG_COMPRESSED	0x0001
> +#define V4L2_FMT_FLAG_EMULATED		0x0002
> +#define V4L2_FMT_FLAG_DYN_RESOLUTION	0x0004
>  
>  	/* Frame Size and frame rate enumeration */
>  /*
> 

[RFC PATCH 2/5] media: venus: vdec: flag OUTPUT formats with V4L2_FMT_FLAG_DYN_RESOLUTION

[Maxime Jourdan]

Tag all the coded formats where the venus vdec supports dynamic
resolution switching.

Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com>
---
 drivers/media/platform/qcom/venus/core.h |  1 +
 drivers/media/platform/qcom/venus/vdec.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 7a3feb5cee00..74eb42668627 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -55,6 +55,7 @@ struct venus_format {
 	u32 pixfmt;
 	unsigned int num_planes;
 	u32 type;
+	u32 flags;
 };
 
 #define MAX_PLANES		4
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 2a47b9b8c5bc..8aabc23966b8 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -46,42 +46,52 @@ static const struct venus_format vdec_formats[] = {
 		.pixfmt = V4L2_PIX_FMT_MPEG4,
 		.num_planes = 1,
 		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	}, {
 		.pixfmt = V4L2_PIX_FMT_MPEG2,
 		.num_planes = 1,
 		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	}, {
 		.pixfmt = V4L2_PIX_FMT_H263,
 		.num_planes = 1,
 		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	}, {
 		.pixfmt = V4L2_PIX_FMT_VC1_ANNEX_G,
 		.num_planes = 1,
 		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	}, {
 		.pixfmt = V4L2_PIX_FMT_VC1_ANNEX_L,
 		.num_planes = 1,
 		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	}, {
 		.pixfmt = V4L2_PIX_FMT_H264,
 		.num_planes = 1,
 		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	}, {
 		.pixfmt = V4L2_PIX_FMT_VP8,
 		.num_planes = 1,
 		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	}, {
 		.pixfmt = V4L2_PIX_FMT_VP9,
 		.num_planes = 1,
 		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	}, {
 		.pixfmt = V4L2_PIX_FMT_XVID,
 		.num_planes = 1,
 		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	}, {
 		.pixfmt = V4L2_PIX_FMT_HEVC,
 		.num_planes = 1,
 		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 };
 
@@ -360,6 +370,7 @@ static int vdec_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
 		return -EINVAL;
 
 	f->pixelformat = fmt->pixfmt;
+	f->flags = fmt->flags;
 
 	return 0;
 }
-- 
2.21.0

Re: [RFC PATCH 2/5] media: venus: vdec: flag OUTPUT formats with V4L2_FMT_FLAG_DYN_RESOLUTION

[Hans Verkuil]

On 6/9/19 4:38 PM, Maxime Jourdan wrote:
> Tag all the coded formats where the venus vdec supports dynamic
> resolution switching.
> 
> Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com>
> ---
>  drivers/media/platform/qcom/venus/core.h |  1 +
>  drivers/media/platform/qcom/venus/vdec.c | 11 +++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 7a3feb5cee00..74eb42668627 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -55,6 +55,7 @@ struct venus_format {
>  	u32 pixfmt;
>  	unsigned int num_planes;
>  	u32 type;
> +	u32 flags;
>  };
>  
>  #define MAX_PLANES		4
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 2a47b9b8c5bc..8aabc23966b8 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -46,42 +46,52 @@ static const struct venus_format vdec_formats[] = {
>  		.pixfmt = V4L2_PIX_FMT_MPEG4,
>  		.num_planes = 1,
>  		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> +		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
>  	}, {
>  		.pixfmt = V4L2_PIX_FMT_MPEG2,
>  		.num_planes = 1,
>  		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> +		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
>  	}, {
>  		.pixfmt = V4L2_PIX_FMT_H263,
>  		.num_planes = 1,
>  		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> +		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
>  	}, {
>  		.pixfmt = V4L2_PIX_FMT_VC1_ANNEX_G,
>  		.num_planes = 1,
>  		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> +		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
>  	}, {
>  		.pixfmt = V4L2_PIX_FMT_VC1_ANNEX_L,
>  		.num_planes = 1,
>  		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> +		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
>  	}, {
>  		.pixfmt = V4L2_PIX_FMT_H264,
>  		.num_planes = 1,
>  		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> +		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
>  	}, {
>  		.pixfmt = V4L2_PIX_FMT_VP8,
>  		.num_planes = 1,
>  		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> +		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
>  	}, {
>  		.pixfmt = V4L2_PIX_FMT_VP9,
>  		.num_planes = 1,
>  		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> +		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
>  	}, {
>  		.pixfmt = V4L2_PIX_FMT_XVID,
>  		.num_planes = 1,
>  		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> +		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
>  	}, {
>  		.pixfmt = V4L2_PIX_FMT_HEVC,
>  		.num_planes = 1,
>  		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
> +		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
>  	},
>  };
>  
> @@ -360,6 +370,7 @@ static int vdec_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>  		return -EINVAL;
>  
>  	f->pixelformat = fmt->pixfmt;
> +	f->flags = fmt->flags;

Ah, there is a bug in v4l_fill_fmtdesc() in v4l2-ioctl.c: at the end
fmt->flags overwritten, so the V4L2_FMT_FLAG_DYN_RESOLUTION is cleared
again. That line in v4l_fill_fmtdesc() should be |=.

Regards,

	Hans

>  
>  	return 0;
>  }
> 

[RFC PATCH 3/5] media: s5p_mfc_dec: flag OUTPUT formats with V4L2_FMT_FLAG_DYN_RESOLUTION

[Maxime Jourdan]

Tag all the coded formats where the s5p_mfc decoder supports dynamic
resolution switching.

Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |  1 +
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index 24148020baa9..59a279abb4a7 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -728,6 +728,7 @@ struct s5p_mfc_fmt {
 	enum s5p_mfc_fmt_type type;
 	u32 num_planes;
 	u32 versions;
+	u32 flags;
 };
 
 /**
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 51ab2e38a270..fd0bb04245a9 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -71,6 +71,7 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_DEC,
 		.num_planes	= 1,
 		.versions	= MFC_V5PLUS_BITS,
+		.flags		= V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.name		= "H264/MVC Encoded Stream",
@@ -79,6 +80,7 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_DEC,
 		.num_planes	= 1,
 		.versions	= MFC_V6PLUS_BITS,
+		.flags		= V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.name		= "H263 Encoded Stream",
@@ -87,6 +89,7 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_DEC,
 		.num_planes	= 1,
 		.versions	= MFC_V5PLUS_BITS,
+		.flags		= V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.name		= "MPEG1 Encoded Stream",
@@ -95,6 +98,7 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_DEC,
 		.num_planes	= 1,
 		.versions	= MFC_V5PLUS_BITS,
+		.flags		= V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.name		= "MPEG2 Encoded Stream",
@@ -103,6 +107,7 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_DEC,
 		.num_planes	= 1,
 		.versions	= MFC_V5PLUS_BITS,
+		.flags		= V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.name		= "MPEG4 Encoded Stream",
@@ -111,6 +116,7 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_DEC,
 		.num_planes	= 1,
 		.versions	= MFC_V5PLUS_BITS,
+		.flags		= V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.name		= "XviD Encoded Stream",
@@ -119,6 +125,7 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_DEC,
 		.num_planes	= 1,
 		.versions	= MFC_V5PLUS_BITS,
+		.flags		= V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.name		= "VC1 Encoded Stream",
@@ -127,6 +134,7 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_DEC,
 		.num_planes	= 1,
 		.versions	= MFC_V5PLUS_BITS,
+		.flags		= V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.name		= "VC1 RCV Encoded Stream",
@@ -135,6 +143,7 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_DEC,
 		.num_planes	= 1,
 		.versions	= MFC_V5PLUS_BITS,
+		.flags		= V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.name		= "VP8 Encoded Stream",
@@ -143,6 +152,7 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_DEC,
 		.num_planes	= 1,
 		.versions	= MFC_V6PLUS_BITS,
+		.flags		= V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.fourcc		= V4L2_PIX_FMT_HEVC,
@@ -150,6 +160,7 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_DEC,
 		.num_planes	= 1,
 		.versions	= MFC_V10_BIT,
+		.flags		= V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.fourcc		= V4L2_PIX_FMT_VP9,
@@ -157,6 +168,7 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_DEC,
 		.num_planes	= 1,
 		.versions	= MFC_V10_BIT,
+		.flags		= V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 };
 
@@ -303,6 +315,7 @@ static int vidioc_enum_fmt(struct file *file, struct v4l2_fmtdesc *f,
 	fmt = &formats[i];
 	strscpy(f->description, fmt->name, sizeof(f->description));
 	f->pixelformat = fmt->fourcc;
+	f->flags = fmt->flags;
 	return 0;
 }
 
-- 
2.21.0

[RFC PATCH 4/5] media: mtk-vcodec: flag OUTPUT formats with V4L2_FMT_FLAG_DYN_RESOLUTION

[Maxime Jourdan]

Tag all the coded formats where the mtk-vcodec decoder supports dynamic
resolution switching.

Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com>
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 4 ++++
 drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
index aee1ddc35de4..0a7e75de56cd 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
@@ -24,16 +24,19 @@ static const struct mtk_video_fmt mtk_video_formats[] = {
 		.fourcc = V4L2_PIX_FMT_H264,
 		.type = MTK_FMT_DEC,
 		.num_planes = 1,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.fourcc = V4L2_PIX_FMT_VP8,
 		.type = MTK_FMT_DEC,
 		.num_planes = 1,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.fourcc = V4L2_PIX_FMT_VP9,
 		.type = MTK_FMT_DEC,
 		.num_planes = 1,
+		.flags = V4L2_FMT_FLAG_DYN_RESOLUTION,
 	},
 	{
 		.fourcc = V4L2_PIX_FMT_MT21C,
@@ -943,6 +946,7 @@ static int vidioc_enum_fmt(struct v4l2_fmtdesc *f, bool output_queue)
 
 	fmt = &mtk_video_formats[i];
 	f->pixelformat = fmt->fourcc;
+	f->flags = fmt->flags;
 
 	return 0;
 }
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
index 7306aeabe229..e9b5322973e1 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
@@ -94,6 +94,7 @@ struct mtk_video_fmt {
 	u32	fourcc;
 	enum mtk_fmt_type	type;
 	u32	num_planes;
+	u32	flags;
 };
 
 /**
-- 
2.21.0

[RFC PATCH 5/5] media: vicodec: flag vdec/stateful OUTPUT formats with V4L2_FMT_FLAG_DYN_RESOLUTION

[Maxime Jourdan]

Tag all the coded formats where the vicodec stateful decoder supports
dynamic resolution switching.

Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com>
---
 drivers/media/platform/vicodec/vicodec-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index 72c56756e45b..8e425c347332 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -729,6 +729,8 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
 			return -EINVAL;
 		f->pixelformat = ctx->is_stateless ?
 			V4L2_PIX_FMT_FWHT_STATELESS : V4L2_PIX_FMT_FWHT;
+		if (!ctx->is_enc && !ctx->is_stateless)
+			f->flags = V4L2_FMT_FLAG_DYN_RESOLUTION;
 	}
 	return 0;
 }
-- 
2.21.0

Re: [RFC PATCH 0/5] Add enum_fmt flag for coded formats with dynamic resolution switching

[Hans Verkuil]

On 6/9/19 4:38 PM, Maxime Jourdan wrote:
> Hello,
> 
> This RFC proposes a new format flag - V4L2_FMT_FLAG_DYN_RESOLUTION - used
> to tag coded formats for which the device supports dynamic resolution
> switching, via V4L2_EVENT_SOURCE_CHANGE.
> This includes the initial "source change" where the device is able to
> tell userspace about the coded resolution and the DPB size (which
> sometimes translates to V4L2_CID_MIN_BUFFERS_FOR_CAPTURE).

Shouldn't the initial source change still be there? The amlogic decoder
is capable of determining the resolution of the stream, right? It just
can't handle mid-stream changes.

Regards,

	Hans

> This flag is mainly aimed at stateful decoder drivers.
> 
> This RFC is motivated by my development on the amlogic video decoder
> driver, which does not support dynamic resolution switching for older
> coded formats (MPEG 1/2, MPEG 4 part II, H263). It does however support
> it for the newer formats (H264, HEVC, VP9).
> 
> The specification regarding stateful video decoders should be amended
> to include that, in the absence of this flag for a certain format,
> userspace is expected to extract the coded resolution and allocate
> a sufficient amount of capture buffers on its own.
> I understand that this point may be tricky, since older kernels with
> close-to-spec drivers would not have this flag available, yet would
> fully support dynamic resolution switching.
> However, with the spec not merged in yet, I wanted to have your opinion
> on this late addition.
> 
> The RFC patches also adds support for this flag for the 4 following
> stateful decoder drivers:
>  - venus
>  - s5p-mfc
>  - mtk-vcodec
>  - vicodec
> 
> Maxime Jourdan (5):
>   media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION
>   media: venus: vdec: flag OUTPUT formats with
>     V4L2_FMT_FLAG_DYN_RESOLUTION
>   media: s5p_mfc_dec: flag OUTPUT formats with
>     V4L2_FMT_FLAG_DYN_RESOLUTION
>   media: mtk-vcodec: flag OUTPUT formats with
>     V4L2_FMT_FLAG_DYN_RESOLUTION
>   media: vicodec: flag vdec/stateful OUTPUT formats with
>     V4L2_FMT_FLAG_DYN_RESOLUTION
> 
>  Documentation/media/uapi/v4l/vidioc-enum-fmt.rst   |  7 +++++++
>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c |  4 ++++
>  drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h |  1 +
>  drivers/media/platform/qcom/venus/core.h           |  1 +
>  drivers/media/platform/qcom/venus/vdec.c           | 11 +++++++++++
>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h    |  1 +
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c       | 13 +++++++++++++
>  drivers/media/platform/vicodec/vicodec-core.c      |  2 ++
>  include/uapi/linux/videodev2.h                     |  5 +++--
>  9 files changed, 43 insertions(+), 2 deletions(-)
> 

Re: [RFC PATCH 0/5] Add enum_fmt flag for coded formats with dynamic resolution switching

[Maxime Jourdan]

On Tue, Jun 11, 2019 at 10:13 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 6/9/19 4:38 PM, Maxime Jourdan wrote:
> > Hello,
> >
> > This RFC proposes a new format flag - V4L2_FMT_FLAG_DYN_RESOLUTION - used
> > to tag coded formats for which the device supports dynamic resolution
> > switching, via V4L2_EVENT_SOURCE_CHANGE.
> > This includes the initial "source change" where the device is able to
> > tell userspace about the coded resolution and the DPB size (which
> > sometimes translates to V4L2_CID_MIN_BUFFERS_FOR_CAPTURE).
>
> Shouldn't the initial source change still be there? The amlogic decoder
> is capable of determining the resolution of the stream, right? It just
> can't handle mid-stream changes.
>

Well, no, not for older compressed formats like MPEG2. Sorry that this
wasn't clear all along. If it did, then the meson vdec wouldn't fail
current v4l2 compliance on the dqevent(V4L2_EVENT_SOURCE_CHANGE).
Userspace is expected to S_FMT the coded resolution before starting up
the capture. If the bitstream has a different resolution than the
format set, then you end up with cropped images. There's no risk for
dma overflow since those are configured to write at most up to buffer
capacity.

To be more precise: the firmware does report back the coded
resolution, but by that time it's too late as it has already begun
consuming the bitstream and decoding to buffers regardless.

For newer compressed formats (H264, HEVC, VP9), the firmware will
pause and notify a source change only, but with the older formats it's
more like "Hey, I decoded frames, by the way here is the coded
resolution".

Regards,
Maxime

> Regards,
>
>         Hans
>
> > This flag is mainly aimed at stateful decoder drivers.
> >
> > This RFC is motivated by my development on the amlogic video decoder
> > driver, which does not support dynamic resolution switching for older
> > coded formats (MPEG 1/2, MPEG 4 part II, H263). It does however support
> > it for the newer formats (H264, HEVC, VP9).
> >
> > The specification regarding stateful video decoders should be amended
> > to include that, in the absence of this flag for a certain format,
> > userspace is expected to extract the coded resolution and allocate
> > a sufficient amount of capture buffers on its own.
> > I understand that this point may be tricky, since older kernels with
> > close-to-spec drivers would not have this flag available, yet would
> > fully support dynamic resolution switching.
> > However, with the spec not merged in yet, I wanted to have your opinion
> > on this late addition.
> >
> > The RFC patches also adds support for this flag for the 4 following
> > stateful decoder drivers:
> >  - venus
> >  - s5p-mfc
> >  - mtk-vcodec
> >  - vicodec
> >
> > Maxime Jourdan (5):
> >   media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION
> >   media: venus: vdec: flag OUTPUT formats with
> >     V4L2_FMT_FLAG_DYN_RESOLUTION
> >   media: s5p_mfc_dec: flag OUTPUT formats with
> >     V4L2_FMT_FLAG_DYN_RESOLUTION
> >   media: mtk-vcodec: flag OUTPUT formats with
> >     V4L2_FMT_FLAG_DYN_RESOLUTION
> >   media: vicodec: flag vdec/stateful OUTPUT formats with
> >     V4L2_FMT_FLAG_DYN_RESOLUTION
> >
> >  Documentation/media/uapi/v4l/vidioc-enum-fmt.rst   |  7 +++++++
> >  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c |  4 ++++
> >  drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h |  1 +
> >  drivers/media/platform/qcom/venus/core.h           |  1 +
> >  drivers/media/platform/qcom/venus/vdec.c           | 11 +++++++++++
> >  drivers/media/platform/s5p-mfc/s5p_mfc_common.h    |  1 +
> >  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c       | 13 +++++++++++++
> >  drivers/media/platform/vicodec/vicodec-core.c      |  2 ++
> >  include/uapi/linux/videodev2.h                     |  5 +++--
> >  9 files changed, 43 insertions(+), 2 deletions(-)
> >
>

Re: [RFC PATCH 0/5] Add enum_fmt flag for coded formats with dynamic resolution switching

[Hans Verkuil]

On 6/11/19 10:13 AM, Hans Verkuil wrote:
> On 6/9/19 4:38 PM, Maxime Jourdan wrote:
>> Hello,
>>
>> This RFC proposes a new format flag - V4L2_FMT_FLAG_DYN_RESOLUTION - used
>> to tag coded formats for which the device supports dynamic resolution
>> switching, via V4L2_EVENT_SOURCE_CHANGE.
>> This includes the initial "source change" where the device is able to
>> tell userspace about the coded resolution and the DPB size (which
>> sometimes translates to V4L2_CID_MIN_BUFFERS_FOR_CAPTURE).
> 
> Shouldn't the initial source change still be there? The amlogic decoder
> is capable of determining the resolution of the stream, right? It just
> can't handle mid-stream changes.

I've been thinking about this a bit more: there are three different HW capabilities:

1) The hardware cannot parse the resolution at all and userspace has to tell it
via S_FMT.

2) The hardware can parse the initial resolution, but is not able to handle
mid-stream resolution changes.

3) The hardware can parse the initial resolution and all following mid-stream
resolution changes.

We can consider 2 the default situation.

In case of 1 the SOURCE_CHANGE event is absent and userspace cannot subscribe
to it. Question: do we want to flag this with the format as well? I.e. with a
V4L2_FMT_FLAG_MANUAL_RESOLUTION? I think just not implementing the SOURCE_CHANGE
event (and documenting this) is sufficient.

In case of 3 the format sets the V4L2_FMT_FLAG_DYN_RESOLUTION flag.

What do you think?

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>> This flag is mainly aimed at stateful decoder drivers.
>>
>> This RFC is motivated by my development on the amlogic video decoder
>> driver, which does not support dynamic resolution switching for older
>> coded formats (MPEG 1/2, MPEG 4 part II, H263). It does however support
>> it for the newer formats (H264, HEVC, VP9).
>>
>> The specification regarding stateful video decoders should be amended
>> to include that, in the absence of this flag for a certain format,
>> userspace is expected to extract the coded resolution and allocate
>> a sufficient amount of capture buffers on its own.
>> I understand that this point may be tricky, since older kernels with
>> close-to-spec drivers would not have this flag available, yet would
>> fully support dynamic resolution switching.
>> However, with the spec not merged in yet, I wanted to have your opinion
>> on this late addition.
>>
>> The RFC patches also adds support for this flag for the 4 following
>> stateful decoder drivers:
>>  - venus
>>  - s5p-mfc
>>  - mtk-vcodec
>>  - vicodec
>>
>> Maxime Jourdan (5):
>>   media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION
>>   media: venus: vdec: flag OUTPUT formats with
>>     V4L2_FMT_FLAG_DYN_RESOLUTION
>>   media: s5p_mfc_dec: flag OUTPUT formats with
>>     V4L2_FMT_FLAG_DYN_RESOLUTION
>>   media: mtk-vcodec: flag OUTPUT formats with
>>     V4L2_FMT_FLAG_DYN_RESOLUTION
>>   media: vicodec: flag vdec/stateful OUTPUT formats with
>>     V4L2_FMT_FLAG_DYN_RESOLUTION
>>
>>  Documentation/media/uapi/v4l/vidioc-enum-fmt.rst   |  7 +++++++
>>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c |  4 ++++
>>  drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h |  1 +
>>  drivers/media/platform/qcom/venus/core.h           |  1 +
>>  drivers/media/platform/qcom/venus/vdec.c           | 11 +++++++++++
>>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h    |  1 +
>>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c       | 13 +++++++++++++
>>  drivers/media/platform/vicodec/vicodec-core.c      |  2 ++
>>  include/uapi/linux/videodev2.h                     |  5 +++--
>>  9 files changed, 43 insertions(+), 2 deletions(-)
>>
> 

Re: [RFC PATCH 0/5] Add enum_fmt flag for coded formats with dynamic resolution switching

[Maxime Jourdan]

On Mon, Jul 15, 2019 at 2:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 6/11/19 10:13 AM, Hans Verkuil wrote:
> > On 6/9/19 4:38 PM, Maxime Jourdan wrote:
> >> Hello,
> >>
> >> This RFC proposes a new format flag - V4L2_FMT_FLAG_DYN_RESOLUTION - used
> >> to tag coded formats for which the device supports dynamic resolution
> >> switching, via V4L2_EVENT_SOURCE_CHANGE.
> >> This includes the initial "source change" where the device is able to
> >> tell userspace about the coded resolution and the DPB size (which
> >> sometimes translates to V4L2_CID_MIN_BUFFERS_FOR_CAPTURE).
> >
> > Shouldn't the initial source change still be there? The amlogic decoder
> > is capable of determining the resolution of the stream, right? It just
> > can't handle mid-stream changes.
>
> I've been thinking about this a bit more: there are three different HW capabilities:
>
> 1) The hardware cannot parse the resolution at all and userspace has to tell it
> via S_FMT.
>
> 2) The hardware can parse the initial resolution, but is not able to handle
> mid-stream resolution changes.
>
> 3) The hardware can parse the initial resolution and all following mid-stream
> resolution changes.
>
> We can consider 2 the default situation.
>
> In case of 1 the SOURCE_CHANGE event is absent and userspace cannot subscribe
> to it. Question: do we want to flag this with the format as well? I.e. with a
> V4L2_FMT_FLAG_MANUAL_RESOLUTION? I think just not implementing the SOURCE_CHANGE
> event (and documenting this) is sufficient.
>

I think that not implementing SOURCE_CHANGE is sufficient as well. The
issue (in my case), is that the amlogic decoder _does_ support the
event (case 3) for anything recent (H264, HEVC, VP9), but not for e.g
MPEG 1/2 (case 1).

A possible solution would be to create 2 separate devices, one
implementing the event, the other not. Do you think this is reasonable
? This would discard the need for all the proposed flags, unless there
are other decoder drivers that fall in case 2.

> In case of 3 the format sets the V4L2_FMT_FLAG_DYN_RESOLUTION flag.
>
> What do you think?
>
> Regards,
>
>         Hans
>
> >
> > Regards,
> >
> >       Hans
> >
> >> This flag is mainly aimed at stateful decoder drivers.
> >>
> >> This RFC is motivated by my development on the amlogic video decoder
> >> driver, which does not support dynamic resolution switching for older
> >> coded formats (MPEG 1/2, MPEG 4 part II, H263). It does however support
> >> it for the newer formats (H264, HEVC, VP9).
> >>
> >> The specification regarding stateful video decoders should be amended
> >> to include that, in the absence of this flag for a certain format,
> >> userspace is expected to extract the coded resolution and allocate
> >> a sufficient amount of capture buffers on its own.
> >> I understand that this point may be tricky, since older kernels with
> >> close-to-spec drivers would not have this flag available, yet would
> >> fully support dynamic resolution switching.
> >> However, with the spec not merged in yet, I wanted to have your opinion
> >> on this late addition.
> >>
> >> The RFC patches also adds support for this flag for the 4 following
> >> stateful decoder drivers:
> >>  - venus
> >>  - s5p-mfc
> >>  - mtk-vcodec
> >>  - vicodec
> >>
> >> Maxime Jourdan (5):
> >>   media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION
> >>   media: venus: vdec: flag OUTPUT formats with
> >>     V4L2_FMT_FLAG_DYN_RESOLUTION
> >>   media: s5p_mfc_dec: flag OUTPUT formats with
> >>     V4L2_FMT_FLAG_DYN_RESOLUTION
> >>   media: mtk-vcodec: flag OUTPUT formats with
> >>     V4L2_FMT_FLAG_DYN_RESOLUTION
> >>   media: vicodec: flag vdec/stateful OUTPUT formats with
> >>     V4L2_FMT_FLAG_DYN_RESOLUTION
> >>
> >>  Documentation/media/uapi/v4l/vidioc-enum-fmt.rst   |  7 +++++++
> >>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c |  4 ++++
> >>  drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h |  1 +
> >>  drivers/media/platform/qcom/venus/core.h           |  1 +
> >>  drivers/media/platform/qcom/venus/vdec.c           | 11 +++++++++++
> >>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h    |  1 +
> >>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c       | 13 +++++++++++++
> >>  drivers/media/platform/vicodec/vicodec-core.c      |  2 ++
> >>  include/uapi/linux/videodev2.h                     |  5 +++--
> >>  9 files changed, 43 insertions(+), 2 deletions(-)
> >>
> >
>

Re: [RFC PATCH 0/5] Add enum_fmt flag for coded formats with dynamic resolution switching

[Hans Verkuil]

On 7/18/19 10:39 AM, Maxime Jourdan wrote:
> On Mon, Jul 15, 2019 at 2:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 6/11/19 10:13 AM, Hans Verkuil wrote:
>>> On 6/9/19 4:38 PM, Maxime Jourdan wrote:
>>>> Hello,
>>>>
>>>> This RFC proposes a new format flag - V4L2_FMT_FLAG_DYN_RESOLUTION - used
>>>> to tag coded formats for which the device supports dynamic resolution
>>>> switching, via V4L2_EVENT_SOURCE_CHANGE.
>>>> This includes the initial "source change" where the device is able to
>>>> tell userspace about the coded resolution and the DPB size (which
>>>> sometimes translates to V4L2_CID_MIN_BUFFERS_FOR_CAPTURE).
>>>
>>> Shouldn't the initial source change still be there? The amlogic decoder
>>> is capable of determining the resolution of the stream, right? It just
>>> can't handle mid-stream changes.
>>
>> I've been thinking about this a bit more: there are three different HW capabilities:
>>
>> 1) The hardware cannot parse the resolution at all and userspace has to tell it
>> via S_FMT.
>>
>> 2) The hardware can parse the initial resolution, but is not able to handle
>> mid-stream resolution changes.
>>
>> 3) The hardware can parse the initial resolution and all following mid-stream
>> resolution changes.
>>
>> We can consider 2 the default situation.
>>
>> In case of 1 the SOURCE_CHANGE event is absent and userspace cannot subscribe
>> to it. Question: do we want to flag this with the format as well? I.e. with a
>> V4L2_FMT_FLAG_MANUAL_RESOLUTION? I think just not implementing the SOURCE_CHANGE
>> event (and documenting this) is sufficient.
>>
> 
> I think that not implementing SOURCE_CHANGE is sufficient as well. The
> issue (in my case), is that the amlogic decoder _does_ support the
> event (case 3) for anything recent (H264, HEVC, VP9), but not for e.g
> MPEG 1/2 (case 1).
> 
> A possible solution would be to create 2 separate devices, one
> implementing the event, the other not. Do you think this is reasonable
> ? This would discard the need for all the proposed flags, unless there
> are other decoder drivers that fall in case 2.

I don't think it is a good idea to create two device nodes, that's really
confusing. Instead I think we just need a V4L2_FMT_FLAG_MANUAL_RESOLUTION
flag.

BTW, what happens if the application sets the format to e.g. 640x480 but
the MPEG file is a different resolution? Does the decoder fail to produce
anything? Or does it internally parse the resolution from the bitstream
and start decoding it? What if the bitstream resolution is larger than the
resolution set with S_FMT? Does it check for the buffer size?

I just want to make sure it won't write past the end of the buffer.

Regards,

	Hans

> 
>> In case of 3 the format sets the V4L2_FMT_FLAG_DYN_RESOLUTION flag.
>>
>> What do you think?
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Regards,
>>>
>>>       Hans
>>>
>>>> This flag is mainly aimed at stateful decoder drivers.
>>>>
>>>> This RFC is motivated by my development on the amlogic video decoder
>>>> driver, which does not support dynamic resolution switching for older
>>>> coded formats (MPEG 1/2, MPEG 4 part II, H263). It does however support
>>>> it for the newer formats (H264, HEVC, VP9).
>>>>
>>>> The specification regarding stateful video decoders should be amended
>>>> to include that, in the absence of this flag for a certain format,
>>>> userspace is expected to extract the coded resolution and allocate
>>>> a sufficient amount of capture buffers on its own.
>>>> I understand that this point may be tricky, since older kernels with
>>>> close-to-spec drivers would not have this flag available, yet would
>>>> fully support dynamic resolution switching.
>>>> However, with the spec not merged in yet, I wanted to have your opinion
>>>> on this late addition.
>>>>
>>>> The RFC patches also adds support for this flag for the 4 following
>>>> stateful decoder drivers:
>>>>  - venus
>>>>  - s5p-mfc
>>>>  - mtk-vcodec
>>>>  - vicodec
>>>>
>>>> Maxime Jourdan (5):
>>>>   media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION
>>>>   media: venus: vdec: flag OUTPUT formats with
>>>>     V4L2_FMT_FLAG_DYN_RESOLUTION
>>>>   media: s5p_mfc_dec: flag OUTPUT formats with
>>>>     V4L2_FMT_FLAG_DYN_RESOLUTION
>>>>   media: mtk-vcodec: flag OUTPUT formats with
>>>>     V4L2_FMT_FLAG_DYN_RESOLUTION
>>>>   media: vicodec: flag vdec/stateful OUTPUT formats with
>>>>     V4L2_FMT_FLAG_DYN_RESOLUTION
>>>>
>>>>  Documentation/media/uapi/v4l/vidioc-enum-fmt.rst   |  7 +++++++
>>>>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c |  4 ++++
>>>>  drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h |  1 +
>>>>  drivers/media/platform/qcom/venus/core.h           |  1 +
>>>>  drivers/media/platform/qcom/venus/vdec.c           | 11 +++++++++++
>>>>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h    |  1 +
>>>>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c       | 13 +++++++++++++
>>>>  drivers/media/platform/vicodec/vicodec-core.c      |  2 ++
>>>>  include/uapi/linux/videodev2.h                     |  5 +++--
>>>>  9 files changed, 43 insertions(+), 2 deletions(-)
>>>>
>>>
>>

Re: [RFC PATCH 0/5] Add enum_fmt flag for coded formats with dynamic resolution switching

[Maxime Jourdan]

On Thu, Jul 18, 2019 at 11:22 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 7/18/19 10:39 AM, Maxime Jourdan wrote:
> > On Mon, Jul 15, 2019 at 2:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 6/11/19 10:13 AM, Hans Verkuil wrote:
> >>> On 6/9/19 4:38 PM, Maxime Jourdan wrote:
> >>>> Hello,
> >>>>
> >>>> This RFC proposes a new format flag - V4L2_FMT_FLAG_DYN_RESOLUTION - used
> >>>> to tag coded formats for which the device supports dynamic resolution
> >>>> switching, via V4L2_EVENT_SOURCE_CHANGE.
> >>>> This includes the initial "source change" where the device is able to
> >>>> tell userspace about the coded resolution and the DPB size (which
> >>>> sometimes translates to V4L2_CID_MIN_BUFFERS_FOR_CAPTURE).
> >>>
> >>> Shouldn't the initial source change still be there? The amlogic decoder
> >>> is capable of determining the resolution of the stream, right? It just
> >>> can't handle mid-stream changes.
> >>
> >> I've been thinking about this a bit more: there are three different HW capabilities:
> >>
> >> 1) The hardware cannot parse the resolution at all and userspace has to tell it
> >> via S_FMT.
> >>
> >> 2) The hardware can parse the initial resolution, but is not able to handle
> >> mid-stream resolution changes.
> >>
> >> 3) The hardware can parse the initial resolution and all following mid-stream
> >> resolution changes.
> >>
> >> We can consider 2 the default situation.
> >>
> >> In case of 1 the SOURCE_CHANGE event is absent and userspace cannot subscribe
> >> to it. Question: do we want to flag this with the format as well? I.e. with a
> >> V4L2_FMT_FLAG_MANUAL_RESOLUTION? I think just not implementing the SOURCE_CHANGE
> >> event (and documenting this) is sufficient.
> >>
> >
> > I think that not implementing SOURCE_CHANGE is sufficient as well. The
> > issue (in my case), is that the amlogic decoder _does_ support the
> > event (case 3) for anything recent (H264, HEVC, VP9), but not for e.g
> > MPEG 1/2 (case 1).
> >
> > A possible solution would be to create 2 separate devices, one
> > implementing the event, the other not. Do you think this is reasonable
> > ? This would discard the need for all the proposed flags, unless there
> > are other decoder drivers that fall in case 2.
>
> I don't think it is a good idea to create two device nodes, that's really
> confusing. Instead I think we just need a V4L2_FMT_FLAG_MANUAL_RESOLUTION
> flag.
>

I guess I just feel bad about adding a flag (MANUAL_RESOLUTION) for
what is basically a problem with one compression standard for one
driver, with the root cause being bad firmware design. Then again I
don't see a way around it, and case 1 & 2 are indeed two possibilities
that need their own flag.

I'll prepare 2 new patch series if that is okay with you:
 - DYN_RESOLUTION format flag updated series (in this current RFC,
there are issues with the explanation of the flag in the doc)
 - Adding MANUAL_RESOLUTION format flag

> BTW, what happens if the application sets the format to e.g. 640x480 but
> the MPEG file is a different resolution? Does the decoder fail to produce
> anything? Or does it internally parse the resolution from the bitstream
> and start decoding it? What if the bitstream resolution is larger than the
> resolution set with S_FMT? Does it check for the buffer size?
>
> I just want to make sure it won't write past the end of the buffer.
>

I tested this case a long while ago.The DMAs are programmed with the
allocated VB2 buffers, so you get cropped pictures (and no DMA
overflow).


> Regards,
>
>         Hans
>
> >
> >> In case of 3 the format sets the V4L2_FMT_FLAG_DYN_RESOLUTION flag.
> >>
> >> What do you think?
> >>
> >> Regards,
> >>
> >>         Hans

Re: [RFC PATCH 0/5] Add enum_fmt flag for coded formats with dynamic resolution switching

[Hans Verkuil]

On 7/24/19 12:32 PM, Maxime Jourdan wrote:
> On Thu, Jul 18, 2019 at 11:22 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 7/18/19 10:39 AM, Maxime Jourdan wrote:
>>> On Mon, Jul 15, 2019 at 2:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 6/11/19 10:13 AM, Hans Verkuil wrote:
>>>>> On 6/9/19 4:38 PM, Maxime Jourdan wrote:
>>>>>> Hello,
>>>>>>
>>>>>> This RFC proposes a new format flag - V4L2_FMT_FLAG_DYN_RESOLUTION - used
>>>>>> to tag coded formats for which the device supports dynamic resolution
>>>>>> switching, via V4L2_EVENT_SOURCE_CHANGE.
>>>>>> This includes the initial "source change" where the device is able to
>>>>>> tell userspace about the coded resolution and the DPB size (which
>>>>>> sometimes translates to V4L2_CID_MIN_BUFFERS_FOR_CAPTURE).
>>>>>
>>>>> Shouldn't the initial source change still be there? The amlogic decoder
>>>>> is capable of determining the resolution of the stream, right? It just
>>>>> can't handle mid-stream changes.
>>>>
>>>> I've been thinking about this a bit more: there are three different HW capabilities:
>>>>
>>>> 1) The hardware cannot parse the resolution at all and userspace has to tell it
>>>> via S_FMT.
>>>>
>>>> 2) The hardware can parse the initial resolution, but is not able to handle
>>>> mid-stream resolution changes.
>>>>
>>>> 3) The hardware can parse the initial resolution and all following mid-stream
>>>> resolution changes.
>>>>
>>>> We can consider 2 the default situation.
>>>>
>>>> In case of 1 the SOURCE_CHANGE event is absent and userspace cannot subscribe
>>>> to it. Question: do we want to flag this with the format as well? I.e. with a
>>>> V4L2_FMT_FLAG_MANUAL_RESOLUTION? I think just not implementing the SOURCE_CHANGE
>>>> event (and documenting this) is sufficient.
>>>>
>>>
>>> I think that not implementing SOURCE_CHANGE is sufficient as well. The
>>> issue (in my case), is that the amlogic decoder _does_ support the
>>> event (case 3) for anything recent (H264, HEVC, VP9), but not for e.g
>>> MPEG 1/2 (case 1).
>>>
>>> A possible solution would be to create 2 separate devices, one
>>> implementing the event, the other not. Do you think this is reasonable
>>> ? This would discard the need for all the proposed flags, unless there
>>> are other decoder drivers that fall in case 2.
>>
>> I don't think it is a good idea to create two device nodes, that's really
>> confusing. Instead I think we just need a V4L2_FMT_FLAG_MANUAL_RESOLUTION
>> flag.
>>
> 
> I guess I just feel bad about adding a flag (MANUAL_RESOLUTION) for
> what is basically a problem with one compression standard for one
> driver, with the root cause being bad firmware design. Then again I
> don't see a way around it, and case 1 & 2 are indeed two possibilities
> that need their own flag.
> 
> I'll prepare 2 new patch series if that is okay with you:
>  - DYN_RESOLUTION format flag updated series (in this current RFC,
> there are issues with the explanation of the flag in the doc)

Wait with this: I'm about to post a consolidated series with all
outstanding patches for codecs. That includes this series.

>  - Adding MANUAL_RESOLUTION format flag
> 
>> BTW, what happens if the application sets the format to e.g. 640x480 but
>> the MPEG file is a different resolution? Does the decoder fail to produce
>> anything? Or does it internally parse the resolution from the bitstream
>> and start decoding it? What if the bitstream resolution is larger than the
>> resolution set with S_FMT? Does it check for the buffer size?
>>
>> I just want to make sure it won't write past the end of the buffer.
>>
> 
> I tested this case a long while ago.The DMAs are programmed with the
> allocated VB2 buffers, so you get cropped pictures (and no DMA
> overflow).

Good to know.

Regards,

	Hans

> 
> 
>> Regards,
>>
>>         Hans
>>
>>>
>>>> In case of 3 the format sets the V4L2_FMT_FLAG_DYN_RESOLUTION flag.
>>>>
>>>> What do you think?
>>>>
>>>> Regards,
>>>>
>>>>         Hans

Re: [RFC PATCH 0/5] Add enum_fmt flag for coded formats with dynamic resolution switching

[Tomasz Figa]

On Mon, Jul 15, 2019 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 6/11/19 10:13 AM, Hans Verkuil wrote:
> > On 6/9/19 4:38 PM, Maxime Jourdan wrote:
> >> Hello,
> >>
> >> This RFC proposes a new format flag - V4L2_FMT_FLAG_DYN_RESOLUTION - used
> >> to tag coded formats for which the device supports dynamic resolution
> >> switching, via V4L2_EVENT_SOURCE_CHANGE.
> >> This includes the initial "source change" where the device is able to
> >> tell userspace about the coded resolution and the DPB size (which
> >> sometimes translates to V4L2_CID_MIN_BUFFERS_FOR_CAPTURE).
> >
> > Shouldn't the initial source change still be there? The amlogic decoder
> > is capable of determining the resolution of the stream, right? It just
> > can't handle mid-stream changes.
>
> I've been thinking about this a bit more: there are three different HW capabilities:
>
> 1) The hardware cannot parse the resolution at all and userspace has to tell it
> via S_FMT.
>
> 2) The hardware can parse the initial resolution, but is not able to handle
> mid-stream resolution changes.
>
> 3) The hardware can parse the initial resolution and all following mid-stream
> resolution changes.
>
> We can consider 2 the default situation.

Any particular reason for 2 being the default? I'm especially
wondering about that as most of the drivers actually provide 3.

>
> In case of 1 the SOURCE_CHANGE event is absent and userspace cannot subscribe
> to it. Question: do we want to flag this with the format as well? I.e. with a
> V4L2_FMT_FLAG_MANUAL_RESOLUTION? I think just not implementing the SOURCE_CHANGE
> event (and documenting this) is sufficient.
>
> In case of 3 the format sets the V4L2_FMT_FLAG_DYN_RESOLUTION flag.
>
> What do you think?
>
> Regards,
>
>         Hans
>
> >
> > Regards,
> >
> >       Hans
> >
> >> This flag is mainly aimed at stateful decoder drivers.
> >>
> >> This RFC is motivated by my development on the amlogic video decoder
> >> driver, which does not support dynamic resolution switching for older
> >> coded formats (MPEG 1/2, MPEG 4 part II, H263). It does however support
> >> it for the newer formats (H264, HEVC, VP9).
> >>
> >> The specification regarding stateful video decoders should be amended
> >> to include that, in the absence of this flag for a certain format,
> >> userspace is expected to extract the coded resolution and allocate
> >> a sufficient amount of capture buffers on its own.
> >> I understand that this point may be tricky, since older kernels with
> >> close-to-spec drivers would not have this flag available, yet would
> >> fully support dynamic resolution switching.
> >> However, with the spec not merged in yet, I wanted to have your opinion
> >> on this late addition.
> >>
> >> The RFC patches also adds support for this flag for the 4 following
> >> stateful decoder drivers:
> >>  - venus
> >>  - s5p-mfc
> >>  - mtk-vcodec
> >>  - vicodec
> >>
> >> Maxime Jourdan (5):
> >>   media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION
> >>   media: venus: vdec: flag OUTPUT formats with
> >>     V4L2_FMT_FLAG_DYN_RESOLUTION
> >>   media: s5p_mfc_dec: flag OUTPUT formats with
> >>     V4L2_FMT_FLAG_DYN_RESOLUTION
> >>   media: mtk-vcodec: flag OUTPUT formats with
> >>     V4L2_FMT_FLAG_DYN_RESOLUTION
> >>   media: vicodec: flag vdec/stateful OUTPUT formats with
> >>     V4L2_FMT_FLAG_DYN_RESOLUTION
> >>
> >>  Documentation/media/uapi/v4l/vidioc-enum-fmt.rst   |  7 +++++++
> >>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c |  4 ++++
> >>  drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h |  1 +
> >>  drivers/media/platform/qcom/venus/core.h           |  1 +
> >>  drivers/media/platform/qcom/venus/vdec.c           | 11 +++++++++++
> >>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h    |  1 +
> >>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c       | 13 +++++++++++++
> >>  drivers/media/platform/vicodec/vicodec-core.c      |  2 ++
> >>  include/uapi/linux/videodev2.h                     |  5 +++--
> >>  9 files changed, 43 insertions(+), 2 deletions(-)
> >>
> >
>

Re: [RFC PATCH 0/5] Add enum_fmt flag for coded formats with dynamic resolution switching

[Hans Verkuil]

On 7/19/19 4:45 AM, Tomasz Figa wrote:
> On Mon, Jul 15, 2019 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 6/11/19 10:13 AM, Hans Verkuil wrote:
>>> On 6/9/19 4:38 PM, Maxime Jourdan wrote:
>>>> Hello,
>>>>
>>>> This RFC proposes a new format flag - V4L2_FMT_FLAG_DYN_RESOLUTION - used
>>>> to tag coded formats for which the device supports dynamic resolution
>>>> switching, via V4L2_EVENT_SOURCE_CHANGE.
>>>> This includes the initial "source change" where the device is able to
>>>> tell userspace about the coded resolution and the DPB size (which
>>>> sometimes translates to V4L2_CID_MIN_BUFFERS_FOR_CAPTURE).
>>>
>>> Shouldn't the initial source change still be there? The amlogic decoder
>>> is capable of determining the resolution of the stream, right? It just
>>> can't handle mid-stream changes.
>>
>> I've been thinking about this a bit more: there are three different HW capabilities:
>>
>> 1) The hardware cannot parse the resolution at all and userspace has to tell it
>> via S_FMT.
>>
>> 2) The hardware can parse the initial resolution, but is not able to handle
>> mid-stream resolution changes.
>>
>> 3) The hardware can parse the initial resolution and all following mid-stream
>> resolution changes.
>>
>> We can consider 2 the default situation.
> 
> Any particular reason for 2 being the default? I'm especially
> wondering about that as most of the drivers actually provide 3.

Various reasons:

1) I prefer to have a flag indicating what IS supported rather than what
   isn't.
2) An application that checks this flag and doesn't see it (i.e. if a
   flag-aware application is used with an older kernel where these flags
   aren't set) will still work, but with possibly reduced functionality.
   If the flag would indicate that something is NOT supported, then they
   would fail when combined with an older kernel and a driver that doesn't
   support dynamic resolution changes.
3) None of the encoders support it, so there too it makes sense to have
   'no dynamic resolution change' as the default. It's nicely symmetrical
   for encoders and decoders.
4) Some formats do not support it, so again, having no dynamic res changes
   as the default makes sense.

Regards,

	Hans

> 
>>
>> In case of 1 the SOURCE_CHANGE event is absent and userspace cannot subscribe
>> to it. Question: do we want to flag this with the format as well? I.e. with a
>> V4L2_FMT_FLAG_MANUAL_RESOLUTION? I think just not implementing the SOURCE_CHANGE
>> event (and documenting this) is sufficient.
>>
>> In case of 3 the format sets the V4L2_FMT_FLAG_DYN_RESOLUTION flag.
>>
>> What do you think?
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Regards,
>>>
>>>       Hans
>>>
>>>> This flag is mainly aimed at stateful decoder drivers.
>>>>
>>>> This RFC is motivated by my development on the amlogic video decoder
>>>> driver, which does not support dynamic resolution switching for older
>>>> coded formats (MPEG 1/2, MPEG 4 part II, H263). It does however support
>>>> it for the newer formats (H264, HEVC, VP9).
>>>>
>>>> The specification regarding stateful video decoders should be amended
>>>> to include that, in the absence of this flag for a certain format,
>>>> userspace is expected to extract the coded resolution and allocate
>>>> a sufficient amount of capture buffers on its own.
>>>> I understand that this point may be tricky, since older kernels with
>>>> close-to-spec drivers would not have this flag available, yet would
>>>> fully support dynamic resolution switching.
>>>> However, with the spec not merged in yet, I wanted to have your opinion
>>>> on this late addition.
>>>>
>>>> The RFC patches also adds support for this flag for the 4 following
>>>> stateful decoder drivers:
>>>>  - venus
>>>>  - s5p-mfc
>>>>  - mtk-vcodec
>>>>  - vicodec
>>>>
>>>> Maxime Jourdan (5):
>>>>   media: videodev2: add V4L2_FMT_FLAG_DYN_RESOLUTION
>>>>   media: venus: vdec: flag OUTPUT formats with
>>>>     V4L2_FMT_FLAG_DYN_RESOLUTION
>>>>   media: s5p_mfc_dec: flag OUTPUT formats with
>>>>     V4L2_FMT_FLAG_DYN_RESOLUTION
>>>>   media: mtk-vcodec: flag OUTPUT formats with
>>>>     V4L2_FMT_FLAG_DYN_RESOLUTION
>>>>   media: vicodec: flag vdec/stateful OUTPUT formats with
>>>>     V4L2_FMT_FLAG_DYN_RESOLUTION
>>>>
>>>>  Documentation/media/uapi/v4l/vidioc-enum-fmt.rst   |  7 +++++++
>>>>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c |  4 ++++
>>>>  drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h |  1 +
>>>>  drivers/media/platform/qcom/venus/core.h           |  1 +
>>>>  drivers/media/platform/qcom/venus/vdec.c           | 11 +++++++++++
>>>>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h    |  1 +
>>>>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c       | 13 +++++++++++++
>>>>  drivers/media/platform/vicodec/vicodec-core.c      |  2 ++
>>>>  include/uapi/linux/videodev2.h                     |  5 +++--
>>>>  9 files changed, 43 insertions(+), 2 deletions(-)
>>>>
>>>
>>

Re: [RFC PATCH 0/5] Add enum_fmt flag for coded formats with dynamic resolution switching

[Tomasz Figa]

On Fri, Jul 19, 2019 at 5:41 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 7/19/19 4:45 AM, Tomasz Figa wrote:
> > On Mon, Jul 15, 2019 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 6/11/19 10:13 AM, Hans Verkuil wrote:
> >>> On 6/9/19 4:38 PM, Maxime Jourdan wrote:
> >>>> Hello,
> >>>>
> >>>> This RFC proposes a new format flag - V4L2_FMT_FLAG_DYN_RESOLUTION - used
> >>>> to tag coded formats for which the device supports dynamic resolution
> >>>> switching, via V4L2_EVENT_SOURCE_CHANGE.
> >>>> This includes the initial "source change" where the device is able to
> >>>> tell userspace about the coded resolution and the DPB size (which
> >>>> sometimes translates to V4L2_CID_MIN_BUFFERS_FOR_CAPTURE).
> >>>
> >>> Shouldn't the initial source change still be there? The amlogic decoder
> >>> is capable of determining the resolution of the stream, right? It just
> >>> can't handle mid-stream changes.
> >>
> >> I've been thinking about this a bit more: there are three different HW capabilities:
> >>
> >> 1) The hardware cannot parse the resolution at all and userspace has to tell it
> >> via S_FMT.
> >>
> >> 2) The hardware can parse the initial resolution, but is not able to handle
> >> mid-stream resolution changes.
> >>
> >> 3) The hardware can parse the initial resolution and all following mid-stream
> >> resolution changes.
> >>
> >> We can consider 2 the default situation.
> >
> > Any particular reason for 2 being the default? I'm especially
> > wondering about that as most of the drivers actually provide 3.
>
> Various reasons:
>
> 1) I prefer to have a flag indicating what IS supported rather than what
>    isn't.
> 2) An application that checks this flag and doesn't see it (i.e. if a
>    flag-aware application is used with an older kernel where these flags
>    aren't set) will still work, but with possibly reduced functionality.
>    If the flag would indicate that something is NOT supported, then they
>    would fail when combined with an older kernel and a driver that doesn't
>    support dynamic resolution changes.
> 3) None of the encoders support it, so there too it makes sense to have
>    'no dynamic resolution change' as the default. It's nicely symmetrical
>    for encoders and decoders.
> 4) Some formats do not support it, so again, having no dynamic res changes
>    as the default makes sense.

Okay, I think you convinced me, thanks!

Feel free to add my Acked-by.

Best regards,
Tomasz