From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Dufresne Subject: Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt Date: Wed, 22 Feb 2017 09:42:25 -0500 Message-ID: <1487774545.5907.17.camel@collabora.com> References: <20170221192059.29745-1-thibault.saunier@osg.samsung.com> <20170221192059.29745-4-thibault.saunier@osg.samsung.com> <78444dcd-169f-0c16-0e09-6b71d1a502b2@samsung.com> Reply-To: Nicolas Dufresne Mime-Version: 1.0 Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-GwKi2XUD95pgM4vA7Cka" Return-path: In-Reply-To: Sender: linux-media-owner@vger.kernel.org To: Thibault Saunier , Andrzej Hajda , linux-kernel@vger.kernel.org Cc: Mauro Carvalho Chehab , Marek Szyprowski , Kukjin Kim , Mauro Carvalho Chehab , Andi Shyti , linux-media@vger.kernel.org, Shuah Khan , Javier Martinez Canillas , linux-samsung-soc@vger.kernel.org, Krzysztof Kozlowski , Inki Dae , Sylwester Nawrocki , linux-arm-kernel@lists.infradead.org, Ulf Hansson , Jeongtae Park , Kyungmin Park , Kamil Debski List-Id: linux-samsung-soc@vger.kernel.org --=-GwKi2XUD95pgM4vA7Cka Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Le mercredi 22 f=C3=A9vrier 2017 =C3=A0 10:10 -0300, Thibault Saunier a =C3= =A9crit=C2=A0: > Hello, >=20 > On 02/22/2017 06:29 AM, Andrzej Hajda wrote: > > On 21.02.2017 20:20, Thibault Saunier wrote: > > > It is required by the standard that the field order is set by the > > > driver. > > >=20 > > > Signed-off-by: Thibault Saunier > > > > > >=20 > > > --- > > >=20 > > > Changes in v5: > > > - Just adapt the field and never error out. > > >=20 > > > Changes in v4: None > > > Changes in v3: > > > - Do not check values in the g_fmt functions as Andrzej explained > > > in previous review > > >=20 > > > Changes in v2: > > > - Fix a silly build error that slipped in while rebasing the > > > patches > > >=20 > > > =C2=A0 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 3 +++ > > > =C2=A0 1 file changed, 3 insertions(+) > > >=20 > > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > > > b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > > > index 0976c3e0a5ce..44ed2afe0780 100644 > > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > > > @@ -386,6 +386,9 @@ static int vidioc_try_fmt(struct file *file, > > > void *priv, struct v4l2_format *f) > > > =C2=A0=C2=A0 struct v4l2_pix_format_mplane *pix_mp =3D &f->fmt.pix_mp= ; > > > =C2=A0=C2=A0 struct s5p_mfc_fmt *fmt; > > > =C2=A0=C2=A0 > > > + if (f->fmt.pix.field =3D=3D V4L2_FIELD_ANY) > > > + f->fmt.pix.field =3D V4L2_FIELD_NONE; > > > + > >=20 > > In previous version the only supported field type was NONE, here > > you > > support everything. > > If the only supported format is none you should set 'field' > > unconditionally to NONE, nothing more. >=20 > Afaict we=C2=A0=C2=A0support 2 things: >=20 > =C2=A0=C2=A0=C2=A01. NONE > =C2=A0=C2=A0=C2=A02. INTERLACE >=20 > Until now we were not checking what was supported or not and > basically=C2=A0 > ignoring that info, this patch > keeps the old behaviour making sure to be compliant. >=20 > I had a doubt and was pondering doing: >=20 > ``` diff >=20 > + if (f->fmt.pix.field !=3D V4L2_FIELD_INTERLACED) > + f->fmt.pix.field =3D V4L2_FIELD_NONE; > + >=20 This looks better to me. > ``` >=20 > instead, it is probably more correct, do you think it is what should > be=C2=A0 > done here? >=20 > Regards, >=20 > Thibault >=20 > >=20 > > Regards > > Andrzej > >=20 > > > =C2=A0=C2=A0 mfc_debug(2, "Type is %d\n", f->type); > > > =C2=A0=C2=A0 if (f->type =3D=3D V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { > > > =C2=A0=C2=A0 fmt =3D find_format(f, MFC_FMT_DEC); >=20 >=20 --=-GwKi2XUD95pgM4vA7Cka Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEABECAAYFAlito1EACgkQcVMCLawGqBy+tgCeN4UKQCQlZflnDAgybPdmgx0y uksAoNNQBZGPedHd6Du/W5ySrwWGBc3B =9TtN -----END PGP SIGNATURE----- --=-GwKi2XUD95pgM4vA7Cka-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: nicolas.dufresne@collabora.com (Nicolas Dufresne) Date: Wed, 22 Feb 2017 09:42:25 -0500 Subject: [PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt In-Reply-To: References: <20170221192059.29745-1-thibault.saunier@osg.samsung.com> <20170221192059.29745-4-thibault.saunier@osg.samsung.com> <78444dcd-169f-0c16-0e09-6b71d1a502b2@samsung.com> Message-ID: <1487774545.5907.17.camel@collabora.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Le mercredi 22 f?vrier 2017 ? 10:10 -0300, Thibault Saunier a ?crit?: > Hello, > > On 02/22/2017 06:29 AM, Andrzej Hajda wrote: > > On 21.02.2017 20:20, Thibault Saunier wrote: > > > It is required by the standard that the field order is set by the > > > driver. > > > > > > Signed-off-by: Thibault Saunier > > > > > > > > > --- > > > > > > Changes in v5: > > > - Just adapt the field and never error out. > > > > > > Changes in v4: None > > > Changes in v3: > > > - Do not check values in the g_fmt functions as Andrzej explained > > > in previous review > > > > > > Changes in v2: > > > - Fix a silly build error that slipped in while rebasing the > > > patches > > > > > > ? drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 3 +++ > > > ? 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > > > b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > > > index 0976c3e0a5ce..44ed2afe0780 100644 > > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > > > @@ -386,6 +386,9 @@ static int vidioc_try_fmt(struct file *file, > > > void *priv, struct v4l2_format *f) > > > ?? struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp; > > > ?? struct s5p_mfc_fmt *fmt; > > > ?? > > > + if (f->fmt.pix.field == V4L2_FIELD_ANY) > > > + f->fmt.pix.field = V4L2_FIELD_NONE; > > > + > > > > In previous version the only supported field type was NONE, here > > you > > support everything. > > If the only supported format is none you should set 'field' > > unconditionally to NONE, nothing more. > > Afaict we??support 2 things: > > ???1. NONE > ???2. INTERLACE > > Until now we were not checking what was supported or not and > basically? > ignoring that info, this patch > keeps the old behaviour making sure to be compliant. > > I had a doubt and was pondering doing: > > ``` diff > > + if (f->fmt.pix.field != V4L2_FIELD_INTERLACED) > + f->fmt.pix.field = V4L2_FIELD_NONE; > + > This looks better to me. > ``` > > instead, it is probably more correct, do you think it is what should > be? > done here? > > Regards, > > Thibault > > > > > Regards > > Andrzej > > > > > ?? mfc_debug(2, "Type is %d\n", f->type); > > > ?? if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { > > > ?? fmt = find_format(f, MFC_FMT_DEC); > > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 181 bytes Desc: This is a digitally signed message part URL: