RE: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.
Hi, > From: Hans Verkuil [mailto:hverk...@xs4all.nl] > Sent: Tuesday, October 21, 2014 1:35 PM > > Hi, > > Let me chime in as well, based on the discussions I had last week in > Düsseldorf: > > On 10/09/2014 12:06 PM, Kamil Debski wrote: > > Hi, > > > >> From: Nicolas Dufresne [mailto:nicolas.dufre...@collabora.com] > >> Sent: Wednesday, October 08, 2014 4:35 PM > >> > >> > >> Le 2014-10-08 06:24, Kamil Debski a écrit : > >>> Hi, > >>> > >>> This patch seems complicated and I do not understand your motives. > >>> > >>> Could you explain what is the problem with the current aligning of > >> the > >>> values? > >>> Is this a hardware problem? Which MFC version does it affect? > >>> Is it a software problem? If so, maybe the user space application > >> should > >>> take extra care on what value it passes/receives to try_fmt? > >> This looks like something I wanted to bring here as an RFC but never > >> manage to get the time. In an Odroid Integration we have started > using > >> the following simple patch to work around this: > >> > >> https://github.com/dsd/linux- > >> odroid/commit/c76b38c1d682b9870ea3b00093ad6500a9c5f5f6 > >> > >> The context is that right now we have decided that alignment in > s_fmt > >> shall be done with a closest rounding. So the format returned may be > >> bigger, or smaller, that's basically random. I've been digging > through > >> a > >> lot, and so far I have found no rational that explains this choice > >> other > >> that this felt right. > >> > >> In real life, whenever the resulting format is smaller then request, > >> there is little we can do other then fail or try again blindly other > >> sizes. But with bigger raw buffers, we can use zero-copy cropping > >> techniques to keep going. Here's a example: > >> > >> image_generator -> hw_converter -> display > >> > >> As hw_converter is a V4L2 M2M, an ideal use case here would be for > >> image_generator to use buffers from the hw_converter. For the > scenario, > >> it is likely that a fixed video size is wanted, but this size is > also > >> likely not to match HW requirement. If hw_converter decide to give > back > >> something smaller, there is nothing image_generator can do. It would > >> have to try again with random size to find out that best match. It's > a > >> bit silly to force that on application, as the hw_converter know the > >> closest best match, which is simply the next valid bigger size if > that > >> exist. > >> > >> hope that helps, > >> Nicolas > > > > Nicolas, thank you for shedding light on this problem. I see that it > is > > not MFC specific. It seems that the problem applies to all > Video4Linux2 > > devices that use v4l_bound_align_image. I agree with you that this > deservers > > an RFC and some discussion as this would change the behaviour of > quite > > a few drivers. > > > > I think the documentation does not specify how TRY_FMT/S_FMT should > adjust > > the parameters. Maybe it would a good idea to add some flagS that > determine > > the behaviour? > > I think we should add flags here as well. NEAREST (the default), > ROUND_DOWN and > ROUND_UP. Existing calls will use NEAREST. I can think of use-cases for > all > three of these, and I think the caller should just have to specify what > is > needed. > > Just replacing the algorithm used seems asking for problems, you want > to be > able to select what you want to do. I agree with Hans here. Nicolas, Pawel, I understand your problem with the nearest value behaviour and I think this should be resolved right. I think the flags could be added to the flags field of v4l2_pix_format (and its multiplane counterpart). Something along the lines: V4L2_PIX_FMT_FLAG_ALIGN_GE, V4L2_PIX_FMT_FLAG_ALIGN_LE (akin to the flags used in the selection API). The function v4l2_bound_align could be then modified to take one more argument and act accordingly. No flags set would mean the current behaviour, and when flags are set it would either round up, down or return an error if the exact value cannot be used. What do you think? Best wishes, -- Kamil Debski Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.
Le 2014-10-21 07:34, Hans Verkuil a écrit : I think we should add flags here as well. NEAREST (the default), ROUND_DOWN and ROUND_UP. Existing calls will use NEAREST. I can think of use-cases for all three of these, and I think the caller should just have to specify what is needed. Just replacing the algorithm used seems asking for problems, you want to be able to select what you want to do. One more thing, we realize that in selection scenario, we do want nearest or lowest, so indeed a flag that let user space choose is the best. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.
Hi, Let me chime in as well, based on the discussions I had last week in Düsseldorf: On 10/09/2014 12:06 PM, Kamil Debski wrote: Hi, From: Nicolas Dufresne [mailto:nicolas.dufre...@collabora.com] Sent: Wednesday, October 08, 2014 4:35 PM Le 2014-10-08 06:24, Kamil Debski a écrit : Hi, This patch seems complicated and I do not understand your motives. Could you explain what is the problem with the current aligning of the values? Is this a hardware problem? Which MFC version does it affect? Is it a software problem? If so, maybe the user space application should take extra care on what value it passes/receives to try_fmt? This looks like something I wanted to bring here as an RFC but never manage to get the time. In an Odroid Integration we have started using the following simple patch to work around this: https://github.com/dsd/linux- odroid/commit/c76b38c1d682b9870ea3b00093ad6500a9c5f5f6 The context is that right now we have decided that alignment in s_fmt shall be done with a closest rounding. So the format returned may be bigger, or smaller, that's basically random. I've been digging through a lot, and so far I have found no rational that explains this choice other that this felt right. In real life, whenever the resulting format is smaller then request, there is little we can do other then fail or try again blindly other sizes. But with bigger raw buffers, we can use zero-copy cropping techniques to keep going. Here's a example: image_generator -> hw_converter -> display As hw_converter is a V4L2 M2M, an ideal use case here would be for image_generator to use buffers from the hw_converter. For the scenario, it is likely that a fixed video size is wanted, but this size is also likely not to match HW requirement. If hw_converter decide to give back something smaller, there is nothing image_generator can do. It would have to try again with random size to find out that best match. It's a bit silly to force that on application, as the hw_converter know the closest best match, which is simply the next valid bigger size if that exist. hope that helps, Nicolas Nicolas, thank you for shedding light on this problem. I see that it is not MFC specific. It seems that the problem applies to all Video4Linux2 devices that use v4l_bound_align_image. I agree with you that this deservers an RFC and some discussion as this would change the behaviour of quite a few drivers. I think the documentation does not specify how TRY_FMT/S_FMT should adjust the parameters. Maybe it would a good idea to add some flagS that determine the behaviour? I think we should add flags here as well. NEAREST (the default), ROUND_DOWN and ROUND_UP. Existing calls will use NEAREST. I can think of use-cases for all three of these, and I think the caller should just have to specify what is needed. Just replacing the algorithm used seems asking for problems, you want to be able to select what you want to do. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.
Le 2014-10-09 06:06, Kamil Debski a écrit : I think the documentation does not specify how TRY_FMT/S_FMT should adjust the parameters. Maybe it would a good idea to add some flagS that determine the behaviour? A flag could be a good option, maybe we should take a minute and discuss this next week. cheers, Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.
Hi, > From: Nicolas Dufresne [mailto:nicolas.dufre...@collabora.com] > Sent: Wednesday, October 08, 2014 4:35 PM > > > Le 2014-10-08 06:24, Kamil Debski a écrit : > > Hi, > > > > This patch seems complicated and I do not understand your motives. > > > > Could you explain what is the problem with the current aligning of > the > > values? > > Is this a hardware problem? Which MFC version does it affect? > > Is it a software problem? If so, maybe the user space application > should > > take extra care on what value it passes/receives to try_fmt? > This looks like something I wanted to bring here as an RFC but never > manage to get the time. In an Odroid Integration we have started using > the following simple patch to work around this: > > https://github.com/dsd/linux- > odroid/commit/c76b38c1d682b9870ea3b00093ad6500a9c5f5f6 > > The context is that right now we have decided that alignment in s_fmt > shall be done with a closest rounding. So the format returned may be > bigger, or smaller, that's basically random. I've been digging through > a > lot, and so far I have found no rational that explains this choice > other > that this felt right. > > In real life, whenever the resulting format is smaller then request, > there is little we can do other then fail or try again blindly other > sizes. But with bigger raw buffers, we can use zero-copy cropping > techniques to keep going. Here's a example: > > image_generator -> hw_converter -> display > > As hw_converter is a V4L2 M2M, an ideal use case here would be for > image_generator to use buffers from the hw_converter. For the scenario, > it is likely that a fixed video size is wanted, but this size is also > likely not to match HW requirement. If hw_converter decide to give back > something smaller, there is nothing image_generator can do. It would > have to try again with random size to find out that best match. It's a > bit silly to force that on application, as the hw_converter know the > closest best match, which is simply the next valid bigger size if that > exist. > > hope that helps, > Nicolas Nicolas, thank you for shedding light on this problem. I see that it is not MFC specific. It seems that the problem applies to all Video4Linux2 devices that use v4l_bound_align_image. I agree with you that this deservers an RFC and some discussion as this would change the behaviour of quite a few drivers. I think the documentation does not specify how TRY_FMT/S_FMT should adjust the parameters. Maybe it would a good idea to add some flagS that determine the behaviour? Best wishes, -- Kamil Debski Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.
Le 2014-10-08 06:24, Kamil Debski a écrit : Hi, This patch seems complicated and I do not understand your motives. Could you explain what is the problem with the current aligning of the values? Is this a hardware problem? Which MFC version does it affect? Is it a software problem? If so, maybe the user space application should take extra care on what value it passes/receives to try_fmt? This looks like something I wanted to bring here as an RFC but never manage to get the time. In an Odroid Integration we have started using the following simple patch to work around this: https://github.com/dsd/linux-odroid/commit/c76b38c1d682b9870ea3b00093ad6500a9c5f5f6 The context is that right now we have decided that alignment in s_fmt shall be done with a closest rounding. So the format returned may be bigger, or smaller, that's basically random. I've been digging through a lot, and so far I have found no rational that explains this choice other that this felt right. In real life, whenever the resulting format is smaller then request, there is little we can do other then fail or try again blindly other sizes. But with bigger raw buffers, we can use zero-copy cropping techniques to keep going. Here's a example: image_generator -> hw_converter -> display As hw_converter is a V4L2 M2M, an ideal use case here would be for image_generator to use buffers from the hw_converter. For the scenario, it is likely that a fixed video size is wanted, but this size is also likely not to match HW requirement. If hw_converter decide to give back something smaller, there is nothing image_generator can do. It would have to try again with random size to find out that best match. It's a bit silly to force that on application, as the hw_converter know the closest best match, which is simply the next valid bigger size if that exist. hope that helps, Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.
Hi,
This patch seems complicated and I do not understand your motives.
Could you explain what is the problem with the current aligning of the
values?
Is this a hardware problem? Which MFC version does it affect?
Is it a software problem? If so, maybe the user space application should
take extra care on what value it passes/receives to try_fmt?
> From: Kiran AVND [mailto:avnd.ki...@samsung.com]
> Sent: Friday, September 26, 2014 6:52 AM
> To: linux-media@vger.kernel.org
> Cc: k.deb...@samsung.com; wuchen...@chromium.org; posc...@chromium.org;
> aru...@samsung.com; i...@chromium.org; prathyus...@samsung.com;
> arun...@samsung.com; ki...@chromium.org
> Subject: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size
> to smaller than the request.
>
> From: Wu-Cheng Li
>
> Use the requested size as the minimum bound, unless it's less than the
> required hardware minimum. The bound align function will align to the
> closest value but we do not want to adjust below the requested size.
This patch does also change the alignment. This is not mentioned in the
commit
message (!). It was 2, now it enforces 16. Could you justify this?
If I remember correctly having even number was enough for MFC v5 encoder
to work properly.
> Signed-off-by: Wu-Cheng Li
> Signed-off-by: Kiran AVND
> ---
> drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> index 407dc63..7b48180 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> @@ -1056,6 +1056,7 @@ static int vidioc_try_fmt(struct file *file, void
> *priv, struct v4l2_format *f)
> struct s5p_mfc_dev *dev = video_drvdata(file);
> struct s5p_mfc_fmt *fmt;
> struct v4l2_pix_format_mplane *pix_fmt_mp = &f->fmt.pix_mp;
> + u32 min_w, min_h;
>
> if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> fmt = find_format(f, MFC_FMT_ENC);
> @@ -1090,8 +1091,16 @@ static int vidioc_try_fmt(struct file *file,
> void *priv, struct v4l2_format *f)
> return -EINVAL;
> }
>
> - v4l_bound_align_image(&pix_fmt_mp->width, 8, 1920, 1,
> - &pix_fmt_mp->height, 4, 1080, 1, 0);
> + /*
> + * Use the requested size as the minimum bound, unless it's
> less
> + * than the required hardware minimum. The bound align
> function
> + * will align to the closest value but we do not want to
> adjust
> + * below the requested size.
Other drivers use v4l2_bound_align and user space apps can cope with
the driver returning a value that is below the requested value.
> + */
> + min_w = min(max(16u, pix_fmt_mp->width), 1920u);
> + min_h = min(max(16u, pix_fmt_mp->height), 1088u);
> + v4l_bound_align_image(&pix_fmt_mp->width, min_w, 1920, 4,
> + &pix_fmt_mp->height, min_h, 1088, 4, 0);
> } else {
> mfc_err("invalid buf type\n");
> return -EINVAL;
> --
> 1.7.9.5
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
