| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250618-fragrant-seagull-of-tranquility-c91dfd@houat>
Date: Wed, 18 Jun 2025 10:41:50 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Dharma Balasubiramani <dharma.b@...rochip.com>
Cc: Manikandan Muralidharan <manikandan.m@...rochip.com>,
Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>,
Robert Foss <rfoss@...nel.org>, Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, Sandeep Sheriker M <sandeep.sheriker@...rochip.com>
Subject: Re: [PATCH] drm/bridge: fix LVDS controller bus format
Hi,
On Wed, Jun 18, 2025 at 10:02:42AM +0530, Dharma Balasubiramani wrote:
> From: Sandeep Sheriker M <sandeep.sheriker@...rochip.com>
>
> The current LVDS controller driver is hardcoded to map LVDS lanes to the
> JEIDA format. Consequently, connecting an LVDS display that supports the
> VESA format results in a distorted display due to the format mismatch.
>
> Query the panel driver and set the appropriate format to resolve the issue.
>
> Signed-off-by: Sandeep Sheriker M <sandeep.sheriker@...rochip.com>
> Signed-off-by: Dharma Balasubiramani <dharma.b@...rochip.com>
It looks like there's a bit of context missing to explain why you needed
to do it that way. See below.
> ---
> drivers/gpu/drm/bridge/microchip-lvds.c | 108 ++++++++++++++++++++++++++++++--
> 1 file changed, 102 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/microchip-lvds.c b/drivers/gpu/drm/bridge/microchip-lvds.c
> index 9f4ff82bc6b4..5e99c01033bb 100644
> --- a/drivers/gpu/drm/bridge/microchip-lvds.c
> +++ b/drivers/gpu/drm/bridge/microchip-lvds.c
> @@ -11,6 +11,7 @@
> #include <linux/component.h>
> #include <linux/delay.h>
> #include <linux/jiffies.h>
> +#include <linux/media-bus-format.h>
> #include <linux/mfd/syscon.h>
> #include <linux/of_graph.h>
> #include <linux/pinctrl/devinfo.h>
> @@ -41,9 +42,11 @@
>
> /* Bitfields in LVDSC_CFGR (Configuration Register) */
> #define LVDSC_CFGR_PIXSIZE_24BITS 0
> +#define LVDSC_CFGR_PIXSIZE_18BITS 1
> #define LVDSC_CFGR_DEN_POL_HIGH 0
> #define LVDSC_CFGR_DC_UNBALANCED 0
> #define LVDSC_CFGR_MAPPING_JEIDA BIT(6)
> +#define LVDSC_CFGR_MAPPING_VESA 0
>
> /*Bitfields in LVDSC_SR */
> #define LVDSC_SR_CS BIT(0)
> @@ -58,6 +61,7 @@ struct mchp_lvds {
> struct clk *pclk;
> struct drm_panel *panel;
> struct drm_bridge bridge;
> + struct drm_connector connector;
> struct drm_bridge *panel_bridge;
> };
>
> @@ -66,6 +70,11 @@ static inline struct mchp_lvds *bridge_to_lvds(struct drm_bridge *bridge)
> return container_of(bridge, struct mchp_lvds, bridge);
> }
>
> +static inline struct mchp_lvds *drm_connector_to_mchp_lvds(struct drm_connector *connector)
> +{
> + return container_of(connector, struct mchp_lvds, connector);
> +}
> +
> static inline u32 lvds_readl(struct mchp_lvds *lvds, u32 offset)
> {
> return readl_relaxed(lvds->regs + offset);
> @@ -79,6 +88,11 @@ static inline void lvds_writel(struct mchp_lvds *lvds, u32 offset, u32 val)
> static void lvds_serialiser_on(struct mchp_lvds *lvds)
> {
> unsigned long timeout = jiffies + msecs_to_jiffies(LVDS_POLL_TIMEOUT_MS);
> + struct drm_connector *connector = &lvds->connector;
How does that work if the bridge was attached with NO_CONNECTOR? Would
the structure be uninitialized?
> +
> + /* default to jeida-24 */
> + u32 bus_formats = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> + u8 map, pix_size;
>
> /* The LVDSC registers can only be written if WPEN is cleared */
> lvds_writel(lvds, LVDSC_WPMR, (LVDSC_WPMR_WPKEY_PSSWD &
> @@ -93,24 +107,106 @@ static void lvds_serialiser_on(struct mchp_lvds *lvds)
> usleep_range(1000, 2000);
> }
>
> + if (connector && connector->display_info.num_bus_formats)
> + bus_formats = connector->display_info.bus_formats[0];
> +
> /* Configure the LVDSC */
> - lvds_writel(lvds, LVDSC_CFGR, (LVDSC_CFGR_MAPPING_JEIDA |
> - LVDSC_CFGR_DC_UNBALANCED |
> - LVDSC_CFGR_DEN_POL_HIGH |
> - LVDSC_CFGR_PIXSIZE_24BITS));
> + switch (bus_formats) {
> + case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> + map = LVDSC_CFGR_MAPPING_JEIDA;
> + pix_size = LVDSC_CFGR_PIXSIZE_18BITS;
> + break;
> + case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> + map = LVDSC_CFGR_MAPPING_VESA;
> + pix_size = LVDSC_CFGR_PIXSIZE_24BITS;
> + break;
> + default:
> + map = LVDSC_CFGR_MAPPING_JEIDA;
> + pix_size = LVDSC_CFGR_PIXSIZE_24BITS;
> + break;
> + }
> +
> + lvds_writel(lvds, LVDSC_CFGR, (map | LVDSC_CFGR_DC_UNBALANCED |
> + LVDSC_CFGR_DEN_POL_HIGH | pix_size));
>
> /* Enable the LVDS serializer */
> lvds_writel(lvds, LVDSC_CR, LVDSC_CR_SER_EN);
> }
Aside from the bit above, that part looks fine to me.
>
> +static int mchp_lvds_connector_get_modes(struct drm_connector *connector)
> +{
> + struct mchp_lvds *lvds = drm_connector_to_mchp_lvds(connector);
> +
> + return drm_panel_get_modes(lvds->panel, connector);
> +}
> +
> +static const struct drm_connector_helper_funcs mchp_lvds_connector_helper_funcs = {
> + .get_modes = mchp_lvds_connector_get_modes,
> +};
> +
> +static const struct drm_connector_funcs panel_bridge_connector_funcs = {
> + .reset = drm_atomic_helper_connector_reset,
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .destroy = drm_connector_cleanup,
> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> static int mchp_lvds_attach(struct drm_bridge *bridge,
> struct drm_encoder *encoder,
> enum drm_bridge_attach_flags flags)
> {
> struct mchp_lvds *lvds = bridge_to_lvds(bridge);
> + struct drm_connector *connector = &lvds->connector;
> + int ret;
> +
> + ret = drm_bridge_attach(encoder, lvds->panel_bridge,
> + bridge, flags);
> +
> + if (ret < 0)
> + return ret;
> +
> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> + return 0;
> +
> + if (!encoder) {
> + dev_err(lvds->dev, "Missing encoder\n");
> + return -ENODEV;
> + }
> +
> + drm_connector_helper_add(connector,
> + &mchp_lvds_connector_helper_funcs);
> +
> + ret = drm_connector_init(bridge->dev, connector,
> + &panel_bridge_connector_funcs,
> + DRM_MODE_CONNECTOR_LVDS);
> + if (ret) {
> + dev_err(lvds->dev, "Failed to initialize connector %d\n", ret);
> + return ret;
> + }
>
> - return drm_bridge_attach(encoder, lvds->panel_bridge,
> - bridge, flags);
> + drm_panel_bridge_set_orientation(connector, bridge);
> +
> + ret = drm_connector_attach_encoder(&lvds->connector, encoder);
> + if (ret) {
> + dev_err(lvds->dev, "Failed to attach connector to encoder %d\n", ret);
> + drm_connector_cleanup(connector);
> + return ret;
> + }
> +
> + if (bridge->dev->registered) {
> + if (connector->funcs->reset)
> + connector->funcs->reset(connector);
> +
> + ret = drm_connector_register(connector);
> + if (ret) {
> + dev_err(lvds->dev, "Failed to attach connector to register %d\n", ret);
> + drm_connector_cleanup(connector);
> + return ret;
> + }
> + }
> +
> + return 0;
However, this is the part I don't really get. AFAIU, you create a
connector, for the sole purpose of calling the panel get_modes. But the
panel bridge you already using is calling that function already. So
there must be something more.
Did you create the connector only to be able to access it in
lvds_serialiser_on and thus retrieve the bus_formats? If so, you should
convert enable / disable to atomic_enable / atomic_disable, pass
drm_atomic_state to lvds_serialiser_on, and then call
drm_atomic_get_new_connector_for_encoder(bridge->encoder).
Maximee
Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)
Powered by blists - more mailing lists