Re: [PATCH v7 05/10] drm: bridge: samsung-dsim: Add atomic_check

From: Jagan Teki <jagan@amarulasolutions.com>
To: Marek Vasut <marex@denx.de>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>,
	Inki Dae <inki.dae@samsung.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Joonyoung Shim <jy0922.shim@samsung.com>,
	Seung-Woo Kim <sw0312.kim@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Frieder Schrempf <frieder.schrempf@kontron.de>,
	Fancy Fang <chen.fang@nxp.com>,
	Tim Harvey <tharvey@gateworks.com>,
	Michael Nazzareno Trimarchi <michael@amarulasolutions.com>,
	Adam Ford <aford173@gmail.com>,
	Neil Armstrong <narmstrong@linaro.org>,
	Robert Foss <robert.foss@linaro.org>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	Tommaso Merciai <tommaso.merciai@amarulasolutions.com>,
	Matteo Lisi <matteo.lisi@engicam.com>,
	dri-devel@lists.freedesktop.org,
	linux-samsung-soc@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	NXP Linux Team <linux-imx@nxp.com>,
	linux-amarula <linux-amarula@amarulasolutions.com>
Subject: Re: [PATCH v7 05/10] drm: bridge: samsung-dsim: Add atomic_check
Date: Mon, 17 Oct 2022 13:21:22 +0530	[thread overview]
Message-ID: <CAMty3ZDpez9Ls2gzGcAuM92UO2d2tkNe5FuLGuw0=GTSS-0oeQ@mail.gmail.com> (raw)
In-Reply-To: <80ace77a-49c6-9b22-5c59-d0afa9b32153@denx.de>

On Mon, Oct 17, 2022 at 12:53 PM Marek Vasut <marex@denx.de> wrote:
>
> On 10/17/22 05:54, Jagan Teki wrote:
> > On Sun, Oct 16, 2022 at 2:53 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 10/5/22 17:13, Jagan Teki wrote:
> >>> Look like an explicit fixing up of mode_flags is required for DSIM IP
> >>> present in i.MX8M Mini/Nano SoCs.
> >>>
> >>> At least the LCDIF + DSIM needs active low sync polarities in order
> >>> to correlate the correct sync flags of the surrounding components in
> >>> the chain to make sure the whole pipeline can work properly.
> >>>
> >>> On the other hand the i.MX 8M Mini Applications Processor Reference Manual,
> >>> Rev. 3, 11/2020 says.
> >>> "13.6.3.5.2 RGB interface
> >>>    Vsync, Hsync, and VDEN are active high signals."
> >>>
> >>> No clear evidence about whether it can be documentation issues or
> >>> something, so added a comment FIXME for this and updated the active low
> >>> sync polarities using SAMSUNG_DSIM_TYPE_IMX8MM hw_type.
> >>
> >> [...]
> >>
> >>> +static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
> >>> +                                  struct drm_bridge_state *bridge_state,
> >>> +                                  struct drm_crtc_state *crtc_state,
> >>> +                                  struct drm_connector_state *conn_state)
> >>> +{
> >>> +     struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> >>> +     struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> >>> +
> >>> +     if (dsi->plat_data->hw_type == SAMSUNG_DSIM_TYPE_IMX8MM) {
> >>> +             /**
> >>> +              * FIXME:
> >>> +              * At least LCDIF + DSIM needs active low sync,
> >>> +              * but i.MX 8M Mini Applications Processor Reference Manual,
> >>> +              * Rev. 3, 11/2020 says
> >>> +              *
> >>> +              * 13.6.3.5.2 RGB interface
> >>> +              * Vsync, Hsync, and VDEN are active high signals.
> >>> +              */
> >>> +             adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> >>> +             adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> >>> +     }
> >>
> >> It would be good to explain what exactly is going on here in the
> >> comment, the comment says "Vsync, Hsync, and VDEN are active high
> >> signals." and the code below does exact opposite and sets NxSYNC flags.
> >>
> >> Yes, the MX8MM/MN does need HS/VS/DE active LOW, it is a quirk of that
> >> MXSFB-DSIM glue logic. The MX8MP needs the exact opposite on all three,
> >> active HIGH.
> >
> > This is what exactly is mentioned in the comments.
> >
> > 2nd line mentioned the active low of signals.
> >>> +              * At least LCDIF + DSIM needs active low sync,
> >
> > from 3rd line onwards it mentioned what reference manual is referring
> >
> > Not quite understand what is misleading here.
>
> This part:
> "
> +  * Vsync, Hsync, and VDEN are active high signals.
> +  */
> +  adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> "
>
> Comment claims the signals are active high by quoting datasheet, and
> then sets the exact opposite on next line of code.

The comment stated what is done first and then gave the datasheet
reference. look this sequence seems confusing, I will recheck and
update the best possible comment.

>
> Have a look at this patch, I updated the comment there for MX8MP too:
> [PATCH] drm: bridge: samsung-dsim: Add i.MX8M Plus support

I will check.

Thanks,
Jagan.