Commit 9593126dae3e ("media: venus: Add a handling of QC08C compressed
format") and commit cef92b14e653 ("media: venus: Add a handling of QC10C
compressed format") added support for the QC08C and QC10C compressed
formats respectively.
But these also caused a regression, because the new formats where added
at the beginning of the vdec_formats[] array and the vdec_inst_init()
function sets the default format output and capture using fixed indexes
of that array:
static void vdec_inst_init(struct venus_inst *inst)
{
...
inst->fmt_out = &vdec_formats[8];
inst->fmt_cap = &vdec_formats[0];
...
}
Since now V4L2_PIX_FMT_NV12 is not the first entry in the array anymore,
the default capture format is not set to that as it was done before.
Both commits changed the first index to keep inst->fmt_out default format
set to V4L2_PIX_FMT_H264, but did not update the latter to keep .fmt_out
default format set to V4L2_PIX_FMT_NV12.
Rather than updating the index to the current V4L2_PIX_FMT_NV12 position,
let's reorder the entries so that this format is the first entry again.
This would also make VIDIOC_ENUM_FMT report the V4L2_PIX_FMT_NV12 format
with an index 0 as it did before the QC08C and QC10C formats were added.
Fixes: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")
Fixes: cef92b14e653 ("media: venus: Add a handling of QC10C compressed format")
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
drivers/media/platform/qcom/venus/vdec.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 4ceaba37e2e5..bb14bea9fe09 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -31,15 +31,15 @@
*/
static const struct venus_format vdec_formats[] = {
{
- .pixfmt = V4L2_PIX_FMT_QC08C,
+ .pixfmt = V4L2_PIX_FMT_NV12,
.num_planes = 1,
.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
}, {
- .pixfmt = V4L2_PIX_FMT_QC10C,
+ .pixfmt = V4L2_PIX_FMT_QC08C,
.num_planes = 1,
.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
- },{
- .pixfmt = V4L2_PIX_FMT_NV12,
+ }, {
+ .pixfmt = V4L2_PIX_FMT_QC10C,
.num_planes = 1,
.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
}, {
--
2.39.1
On Fri, Feb 10, 2023 at 09:18:35AM +0100, Javier Martinez Canillas wrote:
> Commit 9593126dae3e ("media: venus: Add a handling of QC08C compressed
> format") and commit cef92b14e653 ("media: venus: Add a handling of QC10C
> compressed format") added support for the QC08C and QC10C compressed
> formats respectively.
>
> But these also caused a regression, because the new formats where added
> at the beginning of the vdec_formats[] array and the vdec_inst_init()
> function sets the default format output and capture using fixed indexes
> of that array:
>
> static void vdec_inst_init(struct venus_inst *inst)
> {
> ...
> inst->fmt_out = &vdec_formats[8];
> inst->fmt_cap = &vdec_formats[0];
> ...
> }
>
> Since now V4L2_PIX_FMT_NV12 is not the first entry in the array anymore,
> the default capture format is not set to that as it was done before.
>
> Both commits changed the first index to keep inst->fmt_out default format
> set to V4L2_PIX_FMT_H264, but did not update the latter to keep .fmt_out
> default format set to V4L2_PIX_FMT_NV12.
>
> Rather than updating the index to the current V4L2_PIX_FMT_NV12 position,
> let's reorder the entries so that this format is the first entry again.
>
> This would also make VIDIOC_ENUM_FMT report the V4L2_PIX_FMT_NV12 format
> with an index 0 as it did before the QC08C and QC10C formats were added.
>
> Fixes: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")
> Fixes: cef92b14e653 ("media: venus: Add a handling of QC10C compressed format")
> Signed-off-by: Javier Martinez Canillas <[email protected]>
I just came across this issue independently and can confirm this patch fixes
the GStreamer V4L2 decoder on QRB5165.
Tested-by: Jordan Crouse <[email protected]>
> ---
>
> drivers/media/platform/qcom/venus/vdec.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 4ceaba37e2e5..bb14bea9fe09 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -31,15 +31,15 @@
> */
> static const struct venus_format vdec_formats[] = {
> {
> - .pixfmt = V4L2_PIX_FMT_QC08C,
> + .pixfmt = V4L2_PIX_FMT_NV12,
> .num_planes = 1,
> .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> }, {
> - .pixfmt = V4L2_PIX_FMT_QC10C,
> + .pixfmt = V4L2_PIX_FMT_QC08C,
> .num_planes = 1,
> .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> - },{
> - .pixfmt = V4L2_PIX_FMT_NV12,
> + }, {
> + .pixfmt = V4L2_PIX_FMT_QC10C,
> .num_planes = 1,
> .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> }, {
Jordan Crouse <[email protected]> writes:
Hello Jordan,
> On Fri, Feb 10, 2023 at 09:18:35AM +0100, Javier Martinez Canillas wrote:
>> Commit 9593126dae3e ("media: venus: Add a handling of QC08C compressed
>> format") and commit cef92b14e653 ("media: venus: Add a handling of QC10C
>> compressed format") added support for the QC08C and QC10C compressed
>> formats respectively.
>>
>> But these also caused a regression, because the new formats where added
>> at the beginning of the vdec_formats[] array and the vdec_inst_init()
>> function sets the default format output and capture using fixed indexes
>> of that array:
>>
>> static void vdec_inst_init(struct venus_inst *inst)
>> {
>> ...
>> inst->fmt_out = &vdec_formats[8];
>> inst->fmt_cap = &vdec_formats[0];
>> ...
>> }
>>
>> Since now V4L2_PIX_FMT_NV12 is not the first entry in the array anymore,
>> the default capture format is not set to that as it was done before.
>>
>> Both commits changed the first index to keep inst->fmt_out default format
>> set to V4L2_PIX_FMT_H264, but did not update the latter to keep .fmt_out
>> default format set to V4L2_PIX_FMT_NV12.
>>
>> Rather than updating the index to the current V4L2_PIX_FMT_NV12 position,
>> let's reorder the entries so that this format is the first entry again.
>>
>> This would also make VIDIOC_ENUM_FMT report the V4L2_PIX_FMT_NV12 format
>> with an index 0 as it did before the QC08C and QC10C formats were added.
>>
>> Fixes: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")
>> Fixes: cef92b14e653 ("media: venus: Add a handling of QC10C compressed format")
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>
> I just came across this issue independently and can confirm this patch fixes
> the GStreamer V4L2 decoder on QRB5165.
>
> Tested-by: Jordan Crouse <[email protected]>
>
Thanks for testing it!
Stanimir, can we please get this for v6.3 as well?
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On 3/6/2023 3:38 PM, Javier Martinez Canillas wrote:
> Jordan Crouse <[email protected]> writes:
>
> Hello Jordan,
>
>> On Fri, Feb 10, 2023 at 09:18:35AM +0100, Javier Martinez Canillas wrote:
>>> Commit 9593126dae3e ("media: venus: Add a handling of QC08C compressed
>>> format") and commit cef92b14e653 ("media: venus: Add a handling of QC10C
>>> compressed format") added support for the QC08C and QC10C compressed
>>> formats respectively.
>>>
>>> But these also caused a regression, because the new formats where added
>>> at the beginning of the vdec_formats[] array and the vdec_inst_init()
>>> function sets the default format output and capture using fixed indexes
>>> of that array:
>>>
>>> static void vdec_inst_init(struct venus_inst *inst)
>>> {
>>> ...
>>> inst->fmt_out = &vdec_formats[8];
>>> inst->fmt_cap = &vdec_formats[0];
>>> ...
>>> }
>>>
>>> Since now V4L2_PIX_FMT_NV12 is not the first entry in the array anymore,
>>> the default capture format is not set to that as it was done before.
>>>
>>> Both commits changed the first index to keep inst->fmt_out default format
>>> set to V4L2_PIX_FMT_H264, but did not update the latter to keep .fmt_out
>>> default format set to V4L2_PIX_FMT_NV12.
>>>
>>> Rather than updating the index to the current V4L2_PIX_FMT_NV12 position,
>>> let's reorder the entries so that this format is the first entry again.
>>>
>>> This would also make VIDIOC_ENUM_FMT report the V4L2_PIX_FMT_NV12 format
>>> with an index 0 as it did before the QC08C and QC10C formats were added.
>>>
>>> Fixes: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")
>>> Fixes: cef92b14e653 ("media: venus: Add a handling of QC10C compressed format")
>>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>> I just came across this issue independently and can confirm this patch fixes
>> the GStreamer V4L2 decoder on QRB5165.
>>
>> Tested-by: Jordan Crouse <[email protected]>
>>
> Thanks for testing it!
>
> Stanimir, can we please get this for v6.3 as well?
Hi Javier, Jordan
Could you please explain what regression/issue you see with patch?
venus hardware supports QC08C which provides better performance hence
driver is publishing it as preferred color format.
if client doesn't support this or want to use any other format, they can
set the desired format with s_fmt.
Thanks,
Dikshita
Dikshita Agarwal <[email protected]> writes:
Hello Dikshita,
> On 3/6/2023 3:38 PM, Javier Martinez Canillas wrote:
>> Jordan Crouse <[email protected]> writes:
>>
>> Hello Jordan,
>>
>>> On Fri, Feb 10, 2023 at 09:18:35AM +0100, Javier Martinez Canillas wrote:
>>>> Commit 9593126dae3e ("media: venus: Add a handling of QC08C compressed
>>>> format") and commit cef92b14e653 ("media: venus: Add a handling of QC10C
>>>> compressed format") added support for the QC08C and QC10C compressed
>>>> formats respectively.
>>>>
>>>> But these also caused a regression, because the new formats where added
>>>> at the beginning of the vdec_formats[] array and the vdec_inst_init()
>>>> function sets the default format output and capture using fixed indexes
>>>> of that array:
>>>>
>>>> static void vdec_inst_init(struct venus_inst *inst)
>>>> {
>>>> ...
>>>> inst->fmt_out = &vdec_formats[8];
>>>> inst->fmt_cap = &vdec_formats[0];
>>>> ...
>>>> }
>>>>
>>>> Since now V4L2_PIX_FMT_NV12 is not the first entry in the array anymore,
>>>> the default capture format is not set to that as it was done before.
>>>>
>>>> Both commits changed the first index to keep inst->fmt_out default format
>>>> set to V4L2_PIX_FMT_H264, but did not update the latter to keep .fmt_out
>>>> default format set to V4L2_PIX_FMT_NV12.
>>>>
>>>> Rather than updating the index to the current V4L2_PIX_FMT_NV12 position,
>>>> let's reorder the entries so that this format is the first entry again.
>>>>
>>>> This would also make VIDIOC_ENUM_FMT report the V4L2_PIX_FMT_NV12 format
>>>> with an index 0 as it did before the QC08C and QC10C formats were added.
>>>>
>>>> Fixes: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")
>>>> Fixes: cef92b14e653 ("media: venus: Add a handling of QC10C compressed format")
>>>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>>> I just came across this issue independently and can confirm this patch fixes
>>> the GStreamer V4L2 decoder on QRB5165.
>>>
>>> Tested-by: Jordan Crouse <[email protected]>
>>>
>> Thanks for testing it!
>>
>> Stanimir, can we please get this for v6.3 as well?
>
> Hi Javier, Jordan
>
> Could you please explain what regression/issue you see with patch?
>
> venus hardware supports QC08C which provides better performance hence
> driver is publishing it as preferred color format.
>
> if client doesn't support this or want to use any other format, they can
> set the desired format with s_fmt.
>
VIDIOC_S_FMT is currently broken for venus, at least on the HP X2
Chromebook and only the default works. I'm still investigating why
vdec_s_fmt() is not working.
But basically, if VIDIOC_S_FMT is called for the capture queue,
then later the VIDIOC_G_FMT ioctl fails with -EINVAL. This is due
the following condition checked in vdec_check_src_change():
static int vdec_check_src_change(struct venus_inst *inst)
{
...
if (inst->subscriptions & V4L2_EVENT_SOURCE_CHANGE &&
inst->codec_state == VENUS_DEC_STATE_INIT &&
!inst->reconfig)
return -EINVAL;
...
}
But regardless, I think that it would be better for a driver to
not change the order of advertised VIDIOC_ENUM_FMT pixel formats.
Because what happens now is that a decoding that was previously
working by default is not working anymore due a combination of
the default being changed and S_FMT not working as expected.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Hi all,
On Tue, Mar 7, 2023 at 9:13 AM Javier Martinez Canillas
<[email protected]> wrote:
>
> Dikshita Agarwal <[email protected]> writes:
>
> Hello Dikshita,
>
> > On 3/6/2023 3:38 PM, Javier Martinez Canillas wrote:
> >> Jordan Crouse <[email protected]> writes:
> >>
> >> Hello Jordan,
> >>
> >>> On Fri, Feb 10, 2023 at 09:18:35AM +0100, Javier Martinez Canillas wrote:
> >>>> Commit 9593126dae3e ("media: venus: Add a handling of QC08C compressed
> >>>> format") and commit cef92b14e653 ("media: venus: Add a handling of QC10C
> >>>> compressed format") added support for the QC08C and QC10C compressed
> >>>> formats respectively.
> >>>>
> >>>> But these also caused a regression, because the new formats where added
> >>>> at the beginning of the vdec_formats[] array and the vdec_inst_init()
> >>>> function sets the default format output and capture using fixed indexes
> >>>> of that array:
> >>>>
> >>>> static void vdec_inst_init(struct venus_inst *inst)
> >>>> {
> >>>> ...
> >>>> inst->fmt_out = &vdec_formats[8];
> >>>> inst->fmt_cap = &vdec_formats[0];
> >>>> ...
> >>>> }
> >>>>
> >>>> Since now V4L2_PIX_FMT_NV12 is not the first entry in the array anymore,
> >>>> the default capture format is not set to that as it was done before.
> >>>>
> >>>> Both commits changed the first index to keep inst->fmt_out default format
> >>>> set to V4L2_PIX_FMT_H264, but did not update the latter to keep .fmt_out
> >>>> default format set to V4L2_PIX_FMT_NV12.
> >>>>
> >>>> Rather than updating the index to the current V4L2_PIX_FMT_NV12 position,
> >>>> let's reorder the entries so that this format is the first entry again.
> >>>>
> >>>> This would also make VIDIOC_ENUM_FMT report the V4L2_PIX_FMT_NV12 format
> >>>> with an index 0 as it did before the QC08C and QC10C formats were added.
> >>>>
> >>>> Fixes: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")
> >>>> Fixes: cef92b14e653 ("media: venus: Add a handling of QC10C compressed format")
> >>>> Signed-off-by: Javier Martinez Canillas <[email protected]>
> >>> I just came across this issue independently and can confirm this patch fixes
> >>> the GStreamer V4L2 decoder on QRB5165.
> >>>
> >>> Tested-by: Jordan Crouse <[email protected]>
> >>>
This patch also fixes an issue running a V4L2 based decoder on Acer
Chromebook Spin 513 which is very similar to the HP X2 Chromebook, not
surprising as both platforms are basically the same, but anyway:
Tested-by: Enric Balletbo i Serra <[email protected]>
>
> >> Thanks for testing it!
> >>
> >> Stanimir, can we please get this for v6.3 as well?
> >
> > Hi Javier, Jordan
> >
> > Could you please explain what regression/issue you see with patch?
> >
> > venus hardware supports QC08C which provides better performance hence
> > driver is publishing it as preferred color format.
> >
> > if client doesn't support this or want to use any other format, they can
> > set the desired format with s_fmt.
> >
I guess general clients are unlikely to support this format as it is
an opaque intermediate format used by Qualcomm platforms, and the
purpose of that format is to be used for other Qualcomm hardware
blocks that know about this format. So I'd say that returning by
default a more common format is more reliable. Using your argument if
someone wants to use QC08C (because he knows it can use it) set with
s_fmt will do the trick too.
In any case, the problem here seems to be that s_fmt is not working,
so it would be nice to have a solution for that first and meanwhile do
not change the old behaviour. Just my two cents.
Best regards,
Enric Balletbo
>
> VIDIOC_S_FMT is currently broken for venus, at least on the HP X2
> Chromebook and only the default works. I'm still investigating why
> vdec_s_fmt() is not working.
>
> But basically, if VIDIOC_S_FMT is called for the capture queue,
> then later the VIDIOC_G_FMT ioctl fails with -EINVAL. This is due
> the following condition checked in vdec_check_src_change():
>
> static int vdec_check_src_change(struct venus_inst *inst)
> {
> ...
> if (inst->subscriptions & V4L2_EVENT_SOURCE_CHANGE &&
> inst->codec_state == VENUS_DEC_STATE_INIT &&
> !inst->reconfig)
> return -EINVAL;
> ...
> }
>
> But regardless, I think that it would be better for a driver to
> not change the order of advertised VIDIOC_ENUM_FMT pixel formats.
>
> Because what happens now is that a decoding that was previously
> working by default is not working anymore due a combination of
> the default being changed and S_FMT not working as expected.
>
> --
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>
On Tue, Mar 07, 2023 at 05:20:18PM +0100, Enric Balletbo i Serra wrote:
> Hi all,
>
> On Tue, Mar 7, 2023 at 9:13 AM Javier Martinez Canillas
> <[email protected]> wrote:
> >
> > Dikshita Agarwal <[email protected]> writes:
> >
> > Hello Dikshita,
> >
> > > On 3/6/2023 3:38 PM, Javier Martinez Canillas wrote:
> > >> Jordan Crouse <[email protected]> writes:
> > >>
> > >> Hello Jordan,
> > >>
> > >>> On Fri, Feb 10, 2023 at 09:18:35AM +0100, Javier Martinez Canillas wrote:
> > >>>> Commit 9593126dae3e ("media: venus: Add a handling of QC08C compressed
> > >>>> format") and commit cef92b14e653 ("media: venus: Add a handling of QC10C
> > >>>> compressed format") added support for the QC08C and QC10C compressed
> > >>>> formats respectively.
> > >>>>
> > >>>> But these also caused a regression, because the new formats where added
> > >>>> at the beginning of the vdec_formats[] array and the vdec_inst_init()
> > >>>> function sets the default format output and capture using fixed indexes
> > >>>> of that array:
> > >>>>
> > >>>> static void vdec_inst_init(struct venus_inst *inst)
> > >>>> {
> > >>>> ...
> > >>>> inst->fmt_out = &vdec_formats[8];
> > >>>> inst->fmt_cap = &vdec_formats[0];
> > >>>> ...
> > >>>> }
> > >>>>
> > >>>> Since now V4L2_PIX_FMT_NV12 is not the first entry in the array anymore,
> > >>>> the default capture format is not set to that as it was done before.
> > >>>>
> > >>>> Both commits changed the first index to keep inst->fmt_out default format
> > >>>> set to V4L2_PIX_FMT_H264, but did not update the latter to keep .fmt_out
> > >>>> default format set to V4L2_PIX_FMT_NV12.
> > >>>>
> > >>>> Rather than updating the index to the current V4L2_PIX_FMT_NV12 position,
> > >>>> let's reorder the entries so that this format is the first entry again.
> > >>>>
> > >>>> This would also make VIDIOC_ENUM_FMT report the V4L2_PIX_FMT_NV12 format
> > >>>> with an index 0 as it did before the QC08C and QC10C formats were added.
> > >>>>
> > >>>> Fixes: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")
> > >>>> Fixes: cef92b14e653 ("media: venus: Add a handling of QC10C compressed format")
> > >>>> Signed-off-by: Javier Martinez Canillas <[email protected]>
> > >>> I just came across this issue independently and can confirm this patch fixes
> > >>> the GStreamer V4L2 decoder on QRB5165.
> > >>>
> > >>> Tested-by: Jordan Crouse <[email protected]>
> > >>>
>
> This patch also fixes an issue running a V4L2 based decoder on Acer
> Chromebook Spin 513 which is very similar to the HP X2 Chromebook, not
> surprising as both platforms are basically the same, but anyway:
>
> Tested-by: Enric Balletbo i Serra <[email protected]>
>
> >
> > >> Thanks for testing it!
> > >>
> > >> Stanimir, can we please get this for v6.3 as well?
> > >
> > > Hi Javier, Jordan
> > >
> > > Could you please explain what regression/issue you see with patch?
> > >
> > > venus hardware supports QC08C which provides better performance hence
> > > driver is publishing it as preferred color format.
> > >
> > > if client doesn't support this or want to use any other format, they can
> > > set the desired format with s_fmt.
> > >
>
> I guess general clients are unlikely to support this format as it is
> an opaque intermediate format used by Qualcomm platforms, and the
> purpose of that format is to be used for other Qualcomm hardware
> blocks that know about this format. So I'd say that returning by
> default a more common format is more reliable. Using your argument if
> someone wants to use QC08C (because he knows it can use it) set with
> s_fmt will do the trick too.
>
> In any case, the problem here seems to be that s_fmt is not working,
> so it would be nice to have a solution for that first and meanwhile do
> not change the old behaviour. Just my two cents.
>
> Best regards,
> Enric Balletbo
>
> >
> > VIDIOC_S_FMT is currently broken for venus, at least on the HP X2
> > Chromebook and only the default works. I'm still investigating why
> > vdec_s_fmt() is not working.
> >
> > But basically, if VIDIOC_S_FMT is called for the capture queue,
> > then later the VIDIOC_G_FMT ioctl fails with -EINVAL. This is due
> > the following condition checked in vdec_check_src_change():
> >
> > static int vdec_check_src_change(struct venus_inst *inst)
> > {
> > ...
> > if (inst->subscriptions & V4L2_EVENT_SOURCE_CHANGE &&
> > inst->codec_state == VENUS_DEC_STATE_INIT &&
> > !inst->reconfig)
> > return -EINVAL;
> > ...
> > }
> >
> > But regardless, I think that it would be better for a driver to
> > not change the order of advertised VIDIOC_ENUM_FMT pixel formats.
> >
> > Because what happens now is that a decoding that was previously
> > working by default is not working anymore due a combination of
> > the default being changed and S_FMT not working as expected.
For my part, I was using the gstreamer v4l2 decoder which for some reason tries
to verify it can support whatever format it gets with G_FMT *before*
trying a S_FMT. I can't confirm or deny if S_FMT currently works or not.
That said, I entirely agree with Javier. While it might be more
bandwidth efficient, QC08C is a obscure format. It is far more likely that the
average open source user would rather use a well known output format and, as
has been mentioned, once S_FMT is fixed those in the know can use the other
formats if they are working with other Qualcomm hardware blocks.
Jordan
Jordan Crouse <[email protected]> writes:
> On Tue, Mar 07, 2023 at 05:20:18PM +0100, Enric Balletbo i Serra wrote:
[...]
>> >
>> > But regardless, I think that it would be better for a driver to
>> > not change the order of advertised VIDIOC_ENUM_FMT pixel formats.
>> >
>> > Because what happens now is that a decoding that was previously
>> > working by default is not working anymore due a combination of
>> > the default being changed and S_FMT not working as expected.
>
> For my part, I was using the gstreamer v4l2 decoder which for some reason tries
> to verify it can support whatever format it gets with G_FMT *before*
> trying a S_FMT. I can't confirm or deny if S_FMT currently works or not.
>
> That said, I entirely agree with Javier. While it might be more
> bandwidth efficient, QC08C is a obscure format. It is far more likely that the
> average open source user would rather use a well known output format and, as
> has been mentioned, once S_FMT is fixed those in the know can use the other
> formats if they are working with other Qualcomm hardware blocks.
>
Agreed. The rule is that the kernel shouldn't regress user-space and the
patches that changed the default format certainly did that. So from that
point of view I think that this patch should land.
There's also Enric's point that NV12 is a more common format and supported
by more user-space programs. That's why think that regardless of the S_FMT
situation, makes sense to revert to the previous default behaviour.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Javier Martinez Canillas <[email protected]> writes:
Hello Stanimir and Dikshita,
> Jordan Crouse <[email protected]> writes:
>
>> On Tue, Mar 07, 2023 at 05:20:18PM +0100, Enric Balletbo i Serra wrote:
>
> [...]
>
>>> >
>>> > But regardless, I think that it would be better for a driver to
>>> > not change the order of advertised VIDIOC_ENUM_FMT pixel formats.
>>> >
>>> > Because what happens now is that a decoding that was previously
>>> > working by default is not working anymore due a combination of
>>> > the default being changed and S_FMT not working as expected.
>>
>> For my part, I was using the gstreamer v4l2 decoder which for some reason tries
>> to verify it can support whatever format it gets with G_FMT *before*
>> trying a S_FMT. I can't confirm or deny if S_FMT currently works or not.
>>
>> That said, I entirely agree with Javier. While it might be more
>> bandwidth efficient, QC08C is a obscure format. It is far more likely that the
>> average open source user would rather use a well known output format and, as
>> has been mentioned, once S_FMT is fixed those in the know can use the other
>> formats if they are working with other Qualcomm hardware blocks.
>>
>
> Agreed. The rule is that the kernel shouldn't regress user-space and the
> patches that changed the default format certainly did that. So from that
> point of view I think that this patch should land.
>
> There's also Enric's point that NV12 is a more common format and supported
> by more user-space programs. That's why think that regardless of the S_FMT
> situation, makes sense to revert to the previous default behaviour.
>
Any news on this patch? It would be great to fix this at least for v6.3.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat