On Tue, Feb 06, 2024 at 12:50:16PM -0600, Adam Ford wrote:

On Tue, Feb 6, 2024 at 11:06 AM Nathan Chancellor [off-list ref] wrote:

quoted

Hi all,

On Sat, Feb 03, 2024 at 10:52:48AM -0600, Adam Ford wrote:

quoted

From: Lucas Stach <l.stach@pengutronix.de>

This IP block is found in the HDMI subsystem of the i.MX8MP SoC. It has a
full timing generator and can switch between different video sources. On
the i.MX8MP however the only supported source is the LCDIF. The block
just needs to be powered up and told about the polarity of the video
sync signals to act in bypass mode.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com> (v7)
Tested-by: Marek Vasut <marex@denx.de> (v1)
Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com> (v7)
Tested-by: Richard Leitner <richard.leitner@skidata.com> (v2)
Tested-by: Frieder Schrempf <redacted> (v2)
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> (v3)
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Tested-by: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Adam Ford <redacted>

<snip>

quoted

diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c
new file mode 100644
index 000000000000..a76b7669fe8a
--- /dev/null
+++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c

...

quoted

+static void imx8mp_hdmi_pvi_bridge_enable(struct drm_bridge *bridge,
+                                       struct drm_bridge_state *bridge_state)
+{
+     struct drm_atomic_state *state = bridge_state->base.state;
+     struct imx8mp_hdmi_pvi *pvi = to_imx8mp_hdmi_pvi(bridge);
+     struct drm_connector_state *conn_state;
+     const struct drm_display_mode *mode;
+     struct drm_crtc_state *crtc_state;
+     struct drm_connector *connector;
+     u32 bus_flags, val;
+
+     connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
+     conn_state = drm_atomic_get_new_connector_state(state, connector);
+     crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+
+     if (WARN_ON(pm_runtime_resume_and_get(pvi->dev)))
+             return;
+
+     mode = &crtc_state->adjusted_mode;
+
+     val = FIELD_PREP(PVI_CTRL_MODE_MASK, PVI_CTRL_MODE_LCDIF) | PVI_CTRL_EN;
+
+     if (mode->flags & DRM_MODE_FLAG_PVSYNC)
+             val |= PVI_CTRL_OP_VSYNC_POL | PVI_CTRL_INP_VSYNC_POL;
+
+     if (mode->flags & DRM_MODE_FLAG_PHSYNC)
+             val |= PVI_CTRL_OP_HSYNC_POL | PVI_CTRL_INP_HSYNC_POL;
+
+     if (pvi->next_bridge->timings)
+             bus_flags = pvi->next_bridge->timings->input_bus_flags;
+     else if (bridge_state)
+             bus_flags = bridge_state->input_bus_cfg.flags;
+
+     if (bus_flags & DRM_BUS_FLAG_DE_HIGH)
+             val |= PVI_CTRL_OP_DE_POL | PVI_CTRL_INP_DE_POL;
+
+     writel(val, pvi->regs + HTX_PVI_CTRL);
+}

Apologies if this has already been reported or fixed, I searched lore
and did not find anything. Clang warns (or errors with CONFIG_WERROR=y)
for this function:

  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:81:11: error: variable 'bus_flags' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
     81 |         else if (bridge_state)
        |                  ^~~~~~~~~~~~
  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:84:6: note: uninitialized use occurs here
     84 |         if (bus_flags & DRM_BUS_FLAG_DE_HIGH)
        |             ^~~~~~~~~
  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:81:7: note: remove the 'if' if its condition is always true
     81 |         else if (bridge_state)
        |              ^~~~~~~~~~~~~~~~~
     82 |                 bus_flags = bridge_state->input_bus_cfg.flags;
  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:60:15: note: initialize the variable 'bus_flags' to silence this warning
     60 |         u32 bus_flags, val;
        |                      ^
        |                       = 0
  1 error generated.

This seems legitimate. If bridge_state can be NULL, should bus_flags be
initialized to zero like it suggests or should that 'else if' be turned
into a plain 'else'? I am happy to send a patch with that guidance.

I don't think we can turn the else-if into a blind else, because in
order to make bus_flags point to bridge_state->input_bus_cfg.flags,
bridge_state must not be NULL, but we could add an additional else to
set bus_flags to 0, but I think the simplest thing to do would be to
set bus_flags = 0 at the initialization on line 60 as it suggests.

If you agree, I can submit a patch later tonight.  I need to fix
another issue found by the build-bot [1]  to make line 113 return NULL
instead of 0 anyway.  I figured I could just fix them both at the same
time.

[1] - https://lore.kernel.org/oe-kbuild-all/202402062134.a6CqAt3s-lkp@intel.com/ (local)

Seems reasonable to me, thanks!

Nathan

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy