From: Frieder Schrempf <frieder.schrempf@kontron.de>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-media@vger.kernel.org, Rui Miguel Silva <rmfrfs@gmail.com>,
kernel@pengutronix.de, Fabio Estevam <festevam@gmail.com>,
linux-imx@nxp.com, Steve Longerbeam <slongerbeam@gmail.com>,
Philipp Zabel <p.zabel@pengutronix.de>,
Marek Vasut <marex@denx.de>,
Marco Felsch <m.felsch@pengutronix.de>,
Martin Kepplinger <martin.kepplinger@puri.sm>,
Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
Subject: Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
Date: Tue, 25 May 2021 11:59:40 +0200 [thread overview]
Message-ID: <c4db10e3-9565-a09a-6590-7711c580f856@kontron.de> (raw)
In-Reply-To: <YKRY7y4ykERzdRSe@pendragon.ideasonboard.com>
Hi Laurent,
On 19.05.21 02:16, Laurent Pinchart wrote:
> Hi Frieder,
>
> On Mon, May 17, 2021 at 12:21:17PM +0200, Frieder Schrempf wrote:
>> On 16.05.21 04:42, Laurent Pinchart wrote:
>>> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
>>> capture with an OV5640 sensor connected over CSI-2, showed that the
>>> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
>>> larger than 8 bits. Do so, even if the reference manual doesn't clearly
>>> describe the effect of the field.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> For the ADV7280-M I also have the diffs below applied. Do you think
>> setting BIT_MIPI_DOUBLE_CMPNT is specific to MEDIA_BUS_FMT_UYVY8_2X8?
>
> Do you need MEDIA_BUS_FMT_UYVY8_2X8 ? Neither MEDIA_BUS_FMT_UYVY8_1X16
> nor MEDIA_BUS_FMT_UYVY8_2X8 match exactly how YUV 4:2:2 data is
> transmitted over CSI-2. V4L2 uses MEDIA_BUS_FMT_UYVY8_1X16 as a
> convention.
I just use MEDIA_BUS_FMT_UYVY8_2X8 as the ADV7280 driver sets it. But if the convention is to use MEDIA_BUS_FMT_UYVY8_1X16 for YUV422, then maybe the ADV driver needs to be fixed.
>
>> In the RM it mentions YUV422 in the description of
>> BIT_MIPI_DOUBLE_CMPNT and without setting it, the colors are all
>> wrong.
>
> That's interesting. I've tested YUV 4:2:2 with an OV5640 sensor, and I
> don't recall having to set the MIPI_DOUBLE_CMPNT field. I'll try to
> retest.
>
>> I know this is not really related to this patch. I'm just wondering
>> how to properly support my setup.
>
> It's hard to tell :-( The MIPI_CSI2_ISP_CONFIG PIXEL_MODE and PARALLEL
> fields are not well documented, and neither is the CSI_CR18
> MIPI_DOUBLE_CMPNT field. While the CSIS and the CSI bridge are
> documented, how they're integrated isn't described. So far, I can only
> guess.
>
>> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
>> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
>> @@ -346,6 +346,11 @@ struct csis_pix_format {
>>
>> static const struct csis_pix_format mipi_csis_formats[] = {
>> /* YUV formats. */
>> + {
>> + .code = MEDIA_BUS_FMT_UYVY8_2X8,
>> + .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
>> + .width = 8,
>> + },
>> {
>> .code = MEDIA_BUS_FMT_UYVY8_1X16,
>> .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
>>
>> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
>> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
>> @@ -504,7 +504,7 @@ static void __mipi_csis_set_format(struct csi_state *state)
>> /* Color format */
>> val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
>> val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
>> - val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
>> + val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type) | MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
>> mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
>>
>> /* Pixel resolution */
>>
>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>> @@ -492,7 +492,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>> case MEDIA_BUS_FMT_UYVY8_1X16:
>> case MEDIA_BUS_FMT_YUYV8_2X8:
>> case MEDIA_BUS_FMT_YUYV8_1X16:
>> - cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
>> + cr3 |= BIT_TWO_8BIT_SENSOR;
>> + cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B | BIT_MIPI_DOUBLE_CMPNT;
>
> I notice that you set both PIXEL_MODE_DUAL and MIPI_DOUBLE_CMPNT. Have
> you tried setting neither ?
Yes, but as soon as I don't set PIXEL_MODE_DUAL, I get overflow errors from the MIPI CSI2 controller and it doesn't work at all.
>
> Have you also tried using MEDIA_BUS_FMT_UYVY8_1X16 ? The difference
> between MEDIA_BUS_FMT_UYVY8_2X8 and MEDIA_BUS_FMT_UYVY8_1X16 in this
> driver is the width value passed to v4l2_get_link_freq(). With
> MEDIA_BUS_FMT_UYVY8_2X8 you'll end up with a computed link frequency
> equal to half of the actual value, and thus a wrong Ths_settle value. It
> shuold have no other influence though.
The link frequency calculation doesn't work for me at the moment, as the ADV driver doesn't provide any of the controls V4L2_CID_LINK_FREQ or V4L2_CID_PIXEL_RATE. So for now I just hardcoded the Ths_settle value.
Thanks for your efforts!
Frieder
next prev parent reply other threads:[~2021-05-25 9:59 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-16 2:42 [RFC PATCH 0/3] media: imx: imx7-media-csi: i.MX8MM support Laurent Pinchart
2021-05-16 2:42 ` [RFC PATCH 1/3] dt-bindings: media: nxp,imx7-csi: Add " Laurent Pinchart
2021-05-18 11:26 ` Martin Kepplinger
2021-05-18 11:33 ` Laurent Pinchart
2021-05-18 13:27 ` Rob Herring
2021-05-16 2:42 ` [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats Laurent Pinchart
2021-05-17 10:21 ` Rui Miguel Silva
2021-05-19 0:23 ` [RFC PATCH 2/3 v1.1] " Laurent Pinchart
2021-05-19 14:07 ` Rui Miguel Silva
2021-05-17 10:21 ` [RFC PATCH 2/3] " Frieder Schrempf
2021-05-19 0:16 ` Laurent Pinchart
2021-05-25 9:59 ` Frieder Schrempf [this message]
2021-06-10 14:49 ` Laurent Pinchart
2021-05-18 11:28 ` Martin Kepplinger
2021-07-28 8:54 ` Martin Kepplinger
2021-07-28 13:20 ` Laurent Pinchart
2021-05-16 2:42 ` [RFC PATCH 3/3] media: imx: imx7-media-csi: Don't set PIXEL_BIT in CSICR1 Laurent Pinchart
2021-05-17 10:22 ` Rui Miguel Silva
2021-05-18 11:32 ` Martin Kepplinger
2021-05-17 10:20 ` [RFC PATCH 0/3] media: imx: imx7-media-csi: i.MX8MM support Rui Miguel Silva
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=c4db10e3-9565-a09a-6590-7711c580f856@kontron.de \
--to=frieder.schrempf@kontron.de \
--cc=dorota.czaplejewicz@puri.sm \
--cc=festevam@gmail.com \
--cc=kernel@pengutronix.de \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-imx@nxp.com \
--cc=linux-media@vger.kernel.org \
--cc=m.felsch@pengutronix.de \
--cc=marex@denx.de \
--cc=martin.kepplinger@puri.sm \
--cc=p.zabel@pengutronix.de \
--cc=rmfrfs@gmail.com \
--cc=slongerbeam@gmail.com \
/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).