From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
To: Helen Koike <helen.koike@collabora.com>,
linux-media@vger.kernel.org, ezequiel@collabora.com,
hverkuil@xs4all.nl, kernel@collabora.com, dafna3@gmail.com,
sakari.ailus@linux.intel.com, linux-rockchip@lists.infradead.org,
mchehab@kernel.org, laurent.pinchart@ideasonboard.com,
Tomasz Figa <tfiga@chromium.org>
Subject: Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros
Date: Fri, 22 May 2020 14:11:22 +0200 [thread overview]
Message-ID: <d0c93454-8a51-a28c-639d-948041fc602a@collabora.com> (raw)
In-Reply-To: <4bd94509-79af-16db-3721-2553508a6c42@collabora.com>
On 21.05.20 00:08, Helen Koike wrote:
>
>
> On 5/20/20 6:54 PM, Helen Koike wrote:
>> Hi Dafna,
>>
>> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
>>> The resize block of rkisp1 always get the input as yuv422.
>>> Instead of defining the default hdiv, vdiv as macros, the
>>> code is more clear if it takes the (hv)div from the YUV422P
>>> info. This makes it clear where those values come from.
>>> The patch also adds documentation to explain that.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>
>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>
>> Thanks!
>> Helen
>>
>>> ---
>>> drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
>>> 1 file changed, 12 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>> index d049374413dc..04a29af8cc92 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>> @@ -16,9 +16,6 @@
>>> #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8
>>> #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV
>>>
>>> -#define RKISP1_MBUS_FMT_HDIV 2
>>> -#define RKISP1_MBUS_FMT_VDIV 1
>>> -
>>> enum rkisp1_shadow_regs_when {
>>> RKISP1_SHADOW_REGS_SYNC,
>>> RKISP1_SHADOW_REGS_ASYNC,
>>> @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz,
>>> static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
>>> enum rkisp1_shadow_regs_when when)
>>> {
>>> - u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV;
>>> struct v4l2_rect sink_y, sink_c, src_y, src_c;
>>> struct v4l2_mbus_framefmt *src_fmt;
>>> struct v4l2_rect *sink_crop;
>>> struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
>>> + const struct v4l2_format_info *yuv422_info =
>>> + v4l2_format_info(V4L2_PIX_FMT_YUV422P);
>>>
>>> sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK,
>>> V4L2_SUBDEV_FORMAT_ACTIVE);
>>> @@ -386,8 +384,9 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
>>> src_y.width = src_fmt->width;
>>> src_y.height = src_fmt->height;
>>>
>>> - sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV;
>>> - sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV;
>>> + /* The input format of the resizer is always yuv422 */
>>> + sink_c.width = sink_y.width / yuv422_info->hdiv;
>>> + sink_c.height = sink_y.height / yuv422_info->vdiv;
>>>
>>> /*
>>> * The resizer is used not only to change the dimensions of the frame
>>> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
>>> * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr
>>> * streams should be set according to the pixel format in the capture.
>>> * The resizer always gets the input as YUV422. If the capture format
>>> - * is RGB then the memory input should be YUV422 so we don't change the
>>> - * default hdiv, vdiv in that case.
>>> + * is RGB then the memory input (the resizer output) should be YUV422
>>> + * so we use the hdiv, vdiv of the YUV422 info in this case.
>>> */
>>> if (v4l2_is_format_yuv(cap->pix.info)) {
>>> - hdiv = cap->pix.info->hdiv;
>>> - vdiv = cap->pix.info->vdiv;
>>> + src_c.width = cap->pix.info->hdiv;
>>> + src_c.height = cap->pix.info->vdiv;
>
> Sorry, I just noticed you are assigning hdiv and vdiv directly to width and height, which looks wrong.
>
> It should be:
>
> src_c.width = src_y.width / cap->pix.info->hdiv;
> src_c.height = src_y.height / cap->pix.info->vdiv;
autch, thanks for finding it, I probably concentrated only on testing the new RGB formats
Thanks,
Dafna
>
> Regards,
> Helen
>
>>> + } else {
>>> + src_c.width = src_y.width / yuv422_info->hdiv;
>>> + src_c.height = src_y.height / yuv422_info->vdiv;
>>> }
>>>
>>> - src_c.width = src_y.width / hdiv;
>>> - src_c.height = src_y.height / vdiv;
>>> -
>>> if (sink_c.width == src_c.width && sink_c.height == src_c.height) {
>>> rkisp1_rsz_disable(rsz, when);
>>> return;
>>>
next prev parent reply other threads:[~2020-05-22 12:11 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-15 14:29 [PATCH v2 0/4] media: staging: rkisp1: various fixes to capture formats Dafna Hirschfeld
2020-05-15 14:29 ` [PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32 Dafna Hirschfeld
2020-05-20 21:50 ` Helen Koike
2020-05-26 23:04 ` Tomasz Figa
2020-06-10 16:11 ` Dafna Hirschfeld
2020-06-10 16:15 ` Tomasz Figa
2020-05-15 14:29 ` [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros Dafna Hirschfeld
2020-05-20 21:54 ` Helen Koike
2020-05-20 22:08 ` Helen Koike
2020-05-22 12:11 ` Dafna Hirschfeld [this message]
2020-05-22 13:31 ` Ezequiel Garcia
2020-05-22 14:15 ` Dafna Hirschfeld
2020-05-22 15:04 ` Ezequiel Garcia
2020-05-25 9:51 ` Dafna Hirschfeld
2020-05-26 22:26 ` Tomasz Figa
2020-05-22 13:57 ` Laurent Pinchart
2020-05-22 14:54 ` Dafna Hirschfeld
2020-05-22 14:59 ` Laurent Pinchart
2020-06-10 16:36 ` Dafna Hirschfeld
2020-05-26 22:44 ` Tomasz Figa
2020-06-10 17:01 ` Dafna Hirschfeld
2020-06-10 17:08 ` Tomasz Figa
2020-05-15 14:29 ` [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444 Dafna Hirschfeld
2020-05-20 22:23 ` Helen Koike
2020-05-26 23:09 ` Tomasz Figa
2020-06-12 12:45 ` Dafna Hirschfeld
2020-06-18 17:47 ` Tomasz Figa
2020-06-18 18:00 ` Dafna Hirschfeld
2020-06-18 18:12 ` Tomasz Figa
2020-06-18 18:50 ` Dafna Hirschfeld
2020-06-18 19:18 ` Tomasz Figa
2020-06-19 7:30 ` Dafna Hirschfeld
2020-06-19 12:04 ` Tomasz Figa
2020-07-18 16:05 ` Dafna Hirschfeld
2020-05-15 14:29 ` [PATCH v2 4/4] media: staging: rkisp1: cap: remove support of BGR666 format Dafna Hirschfeld
2020-05-20 22:34 ` Helen Koike
2020-05-26 22:57 ` Tomasz Figa
2020-05-27 10:14 ` Helen Koike
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d0c93454-8a51-a28c-639d-948041fc602a@collabora.com \
--to=dafna.hirschfeld@collabora.com \
--cc=dafna3@gmail.com \
--cc=ezequiel@collabora.com \
--cc=helen.koike@collabora.com \
--cc=hverkuil@xs4all.nl \
--cc=kernel@collabora.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=mchehab@kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=tfiga@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).