Re: [PATCH 09/10] media: hantro: Don't require unneeded H264_SLICE_PARAMS

From: Ezequiel Garcia <ezequiel@collabora.com>
To: Alexandre Courbot <acourbot@chromium.org>
Cc: Linux Media Mailing List <linux-media@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Tomasz Figa <tfiga@chromium.org>,
	kernel@collabora.com, Jonas Karlman <jonas@kwiboo.se>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	Jeffrey Kardatzke <jkardatzke@chromium.org>,
	Nicolas Dufresne <nicolas.dufresne@collabora.com>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Maxime Ripard <mripard@kernel.org>,
	Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Subject: Re: [PATCH 09/10] media: hantro: Don't require unneeded H264_SLICE_PARAMS
Date: Mon, 27 Jul 2020 11:44:10 -0300	[thread overview]
Message-ID: <44dcbbc97a06352169a2cc99536e41f2ad111238.camel@collabora.com> (raw)
In-Reply-To: <CAPBb6MWG5aXuc7ExiaKvgtLL0o=HDYKFa4D4-v8hfvyGiNFBdA@mail.gmail.com>

Hi Alexandre,

Despite you've asked to ignore this review,
let me comment on it.

On Sat, 2020-07-25 at 23:45 +0900, Alexandre Courbot wrote:
> On Thu, Jul 16, 2020 at 5:23 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > Now that slice invariant parameters have been moved,
> > the driver no longer needs this control, so drop it.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/staging/media/hantro/hantro_drv.c  | 5 -----
> >  drivers/staging/media/hantro/hantro_h264.c | 5 -----
> >  drivers/staging/media/hantro/hantro_hw.h   | 2 --
> >  3 files changed, 12 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > index 34797507f214..3cd00cc0a364 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -306,11 +306,6 @@ static const struct hantro_ctrl controls[] = {
> >                 .cfg = {
> >                         .id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
> >                 },
> > -       }, {
> > -               .codec = HANTRO_H264_DECODER,
> > -               .cfg = {
> > -                       .id = V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
> > -               },
> 
> Isn't this going to make the driver reject (as opposed to just ignore)
> this control altogether? Also, even though the control is not required
> anymore, don't we want to check that it is provided in order to ensure
> user-space follows the spec (granted, this would be better done in a
> common framework shared by all drivers).
> 

As you mentioned on your next reply, indeed frame-based drivers
can't really parse the slice headers.

I believe the above comment would make sense, if we want to avoid
breaking compatibility.

In our case, we are already breaking this (it's broken from the minute
you change any control in the API, as V4L2 reject mismatch sizes
for the controls).

So, I'd say it makes sense to drop it now while we can.

Also, Nicolas has suggested that this makes applications simpler. 

> I'd also suggest this patch (and the following one) to be merged into
> the previous one as they are just removing fields that have become
> unneeded because of it.

I'd like to keep the patches touching the uAPI separate from the
ones touching the driver, when possible (i.e. when the build
is not broken by API changes).

Thanks,
Ezequiel