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

Marek Vasut
2022-10-17T07:24:53Z

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.

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