| Message ID | 20250227-fd-mdp4-lvds-v3-6-c983788987ae@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/mdp4: rework LVDS/LCDC panel support | expand |
On 2/26/2025 6:25 PM, Dmitry Baryshkov wrote: > LVDS support in MDP4 driver makes use of drm_connector directly. However > LCDC encoder and LVDS connector are wrappers around drm_panel. Switch > them to use drm_panel_bridge/drm_bridge_connector. This allows using > standard interface for the drm_panel and also inserting additional > bridges between encoder and panel. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/Makefile | 1 - > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 34 +++++-- > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h | 6 +- > drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 20 +---- > .../gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 100 --------------------- > 5 files changed, 28 insertions(+), 133 deletions(-) > > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile > index 5df20cbeafb8bf07c825a1fd72719d5a56c38613..7a2ada6e2d74a902879e4f12a78ed475e5209ec2 100644 > --- a/drivers/gpu/drm/msm/Makefile > +++ b/drivers/gpu/drm/msm/Makefile > @@ -48,7 +48,6 @@ msm-display-$(CONFIG_DRM_MSM_MDP4) += \ > disp/mdp4/mdp4_dsi_encoder.o \ > disp/mdp4/mdp4_dtv_encoder.o \ > disp/mdp4/mdp4_lcdc_encoder.o \ > - disp/mdp4/mdp4_lvds_connector.o \ > disp/mdp4/mdp4_lvds_pll.o \ > disp/mdp4/mdp4_irq.o \ > disp/mdp4/mdp4_kms.o \ > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > index 689e210660a5218ed1e2d116073723215af5a187..93c9411eb422bc67b7fedb5ffce4c330310b520f 100644 > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > @@ -6,6 +6,8 @@ > > #include <linux/delay.h> > > +#include <drm/drm_bridge.h> > +#include <drm/drm_bridge_connector.h> > #include <drm/drm_vblank.h> > > #include "msm_drv.h" > @@ -189,7 +191,7 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms, > struct msm_drm_private *priv = dev->dev_private; > struct drm_encoder *encoder; > struct drm_connector *connector; > - struct device_node *panel_node; > + struct drm_bridge *next_bridge; > int dsi_id; > int ret; > > @@ -199,27 +201,43 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms, > * bail out early if there is no panel node (no need to > * initialize LCDC encoder and LVDS connector) > */ > - panel_node = of_graph_get_remote_node(dev->dev->of_node, 0, 0); > - if (!panel_node) > - return 0; > + next_bridge = devm_drm_of_get_bridge(dev->dev, dev->dev->of_node, 0, 0); > + if (IS_ERR(next_bridge)) { > + ret = PTR_ERR(next_bridge); > + if (ret == -ENODEV) > + return 0; > + return ret; > + } > > - encoder = mdp4_lcdc_encoder_init(dev, panel_node); > + encoder = mdp4_lcdc_encoder_init(dev); > if (IS_ERR(encoder)) { > DRM_DEV_ERROR(dev->dev, "failed to construct LCDC encoder\n"); > - of_node_put(panel_node); > return PTR_ERR(encoder); > } > > /* LCDC can be hooked to DMA_P (TODO: Add DMA_S later?) */ > encoder->possible_crtcs = 1 << DMA_P; > > - connector = mdp4_lvds_connector_init(dev, panel_node, encoder); > + ret = drm_bridge_attach(encoder, next_bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); > + if (ret) { > + DRM_DEV_ERROR(dev->dev, "failed to attach LVDS panel/bridge: %d\n", ret); > + > + return ret; > + } Can you pls point me to the lvds bridge used with this apq8064 board? I was unable to find it. Just wanted to compare that against this while reviewing. > + > + connector = drm_bridge_connector_init(dev, encoder); > if (IS_ERR(connector)) { > DRM_DEV_ERROR(dev->dev, "failed to initialize LVDS connector\n"); > - of_node_put(panel_node); > return PTR_ERR(connector); > } > > + ret = drm_connector_attach_encoder(connector, encoder); > + if (ret) { > + DRM_DEV_ERROR(dev->dev, "failed to attach LVDS connector: %d\n", ret); > + > + return ret; > + } > + > break; > case DRM_MODE_ENCODER_TMDS: > encoder = mdp4_dtv_encoder_init(dev); > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h > index e0380d3b7e0cee99c4c376bf6369887106f44ede..306f5ca8f810aaeecea56e74065933bbffcb67ec 100644 > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h > @@ -191,11 +191,7 @@ struct drm_crtc *mdp4_crtc_init(struct drm_device *dev, > long mdp4_dtv_round_pixclk(struct drm_encoder *encoder, unsigned long rate); > struct drm_encoder *mdp4_dtv_encoder_init(struct drm_device *dev); > > -struct drm_encoder *mdp4_lcdc_encoder_init(struct drm_device *dev, > - struct device_node *panel_node); > - > -struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev, > - struct device_node *panel_node, struct drm_encoder *encoder); > +struct drm_encoder *mdp4_lcdc_encoder_init(struct drm_device *dev); > > #ifdef CONFIG_DRM_MSM_DSI > struct drm_encoder *mdp4_dsi_encoder_init(struct drm_device *dev); > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c > index cfcedd8a635cf0297365e845ef415a8f0d553183..a4f3edabefbd06286bfb8fbcd7f8c0a4281e5ef1 100644 > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c > @@ -14,7 +14,6 @@ > > struct mdp4_lcdc_encoder { > struct drm_encoder base; > - struct device_node *panel_node; > struct drm_panel *panel; > struct clk *lcdc_clk; > unsigned long int pixclock; > @@ -262,19 +261,12 @@ static void mdp4_lcdc_encoder_disable(struct drm_encoder *encoder) > struct mdp4_lcdc_encoder *mdp4_lcdc_encoder = > to_mdp4_lcdc_encoder(encoder); > struct mdp4_kms *mdp4_kms = get_kms(encoder); > - struct drm_panel *panel; > > if (WARN_ON(!mdp4_lcdc_encoder->enabled)) > return; > > mdp4_write(mdp4_kms, REG_MDP4_LCDC_ENABLE, 0); > > - panel = of_drm_find_panel(mdp4_lcdc_encoder->panel_node); > - if (!IS_ERR(panel)) { > - drm_panel_disable(panel); > - drm_panel_unprepare(panel); > - } > - > /* > * Wait for a vsync so we know the ENABLE=0 latched before > * the (connector) source of the vsync's gets disabled, > @@ -300,7 +292,6 @@ static void mdp4_lcdc_encoder_enable(struct drm_encoder *encoder) > to_mdp4_lcdc_encoder(encoder); > unsigned long pc = mdp4_lcdc_encoder->pixclock; > struct mdp4_kms *mdp4_kms = get_kms(encoder); > - struct drm_panel *panel; > uint32_t config; > int ret; > > @@ -335,12 +326,6 @@ static void mdp4_lcdc_encoder_enable(struct drm_encoder *encoder) > if (ret) > DRM_DEV_ERROR(dev->dev, "failed to enable lcdc_clk: %d\n", ret); > > - panel = of_drm_find_panel(mdp4_lcdc_encoder->panel_node); > - if (!IS_ERR(panel)) { > - drm_panel_prepare(panel); > - drm_panel_enable(panel); > - } > - > setup_phy(encoder); > > mdp4_write(mdp4_kms, REG_MDP4_LCDC_ENABLE, 1); > @@ -375,8 +360,7 @@ static const struct drm_encoder_helper_funcs mdp4_lcdc_encoder_helper_funcs = { > }; > > /* initialize encoder */ > -struct drm_encoder *mdp4_lcdc_encoder_init(struct drm_device *dev, > - struct device_node *panel_node) > +struct drm_encoder *mdp4_lcdc_encoder_init(struct drm_device *dev) > { > struct drm_encoder *encoder; > struct mdp4_lcdc_encoder *mdp4_lcdc_encoder; > @@ -387,8 +371,6 @@ struct drm_encoder *mdp4_lcdc_encoder_init(struct drm_device *dev, > if (IS_ERR(mdp4_lcdc_encoder)) > return ERR_CAST(mdp4_lcdc_encoder); > > - mdp4_lcdc_encoder->panel_node = panel_node; > - > encoder = &mdp4_lcdc_encoder->base; > > drm_encoder_helper_add(encoder, &mdp4_lcdc_encoder_helper_funcs); > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c > deleted file mode 100644 > index 4755eb13ef79f313d2be088145c8cd2e615226fe..0000000000000000000000000000000000000000 > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c > +++ /dev/null > @@ -1,100 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0-only > -/* > - * Copyright (C) 2014 Red Hat > - * Author: Rob Clark <robdclark@gmail.com> > - * Author: Vinay Simha <vinaysimha@inforcecomputing.com> > - */ > - > -#include "mdp4_kms.h" > - > -struct mdp4_lvds_connector { > - struct drm_connector base; > - struct drm_encoder *encoder; > - struct device_node *panel_node; > - struct drm_panel *panel; > -}; > -#define to_mdp4_lvds_connector(x) container_of(x, struct mdp4_lvds_connector, base) > - > -static enum drm_connector_status mdp4_lvds_connector_detect( > - struct drm_connector *connector, bool force) > -{ > - struct mdp4_lvds_connector *mdp4_lvds_connector = > - to_mdp4_lvds_connector(connector); > - > - if (!mdp4_lvds_connector->panel) { > - mdp4_lvds_connector->panel = > - of_drm_find_panel(mdp4_lvds_connector->panel_node); > - if (IS_ERR(mdp4_lvds_connector->panel)) > - mdp4_lvds_connector->panel = NULL; > - } > - > - return mdp4_lvds_connector->panel ? > - connector_status_connected : > - connector_status_disconnected; > -} > - > -static void mdp4_lvds_connector_destroy(struct drm_connector *connector) > -{ > - struct mdp4_lvds_connector *mdp4_lvds_connector = > - to_mdp4_lvds_connector(connector); > - > - drm_connector_cleanup(connector); > - > - kfree(mdp4_lvds_connector); > -} > - > -static int mdp4_lvds_connector_get_modes(struct drm_connector *connector) > -{ > - struct mdp4_lvds_connector *mdp4_lvds_connector = > - to_mdp4_lvds_connector(connector); > - struct drm_panel *panel = mdp4_lvds_connector->panel; > - int ret = 0; > - > - if (panel) > - ret = drm_panel_get_modes(panel, connector); > - > - return ret; > -} > - > -static const struct drm_connector_funcs mdp4_lvds_connector_funcs = { > - .detect = mdp4_lvds_connector_detect, > - .fill_modes = drm_helper_probe_single_connector_modes, > - .destroy = mdp4_lvds_connector_destroy, > - .reset = drm_atomic_helper_connector_reset, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > -}; > - > -static const struct drm_connector_helper_funcs mdp4_lvds_connector_helper_funcs = { > - .get_modes = mdp4_lvds_connector_get_modes, > -}; > - > -/* initialize connector */ > -struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev, > - struct device_node *panel_node, struct drm_encoder *encoder) > -{ > - struct drm_connector *connector = NULL; > - struct mdp4_lvds_connector *mdp4_lvds_connector; > - > - mdp4_lvds_connector = kzalloc(sizeof(*mdp4_lvds_connector), GFP_KERNEL); > - if (!mdp4_lvds_connector) > - return ERR_PTR(-ENOMEM); > - > - mdp4_lvds_connector->encoder = encoder; > - mdp4_lvds_connector->panel_node = panel_node; > - > - connector = &mdp4_lvds_connector->base; > - > - drm_connector_init(dev, connector, &mdp4_lvds_connector_funcs, > - DRM_MODE_CONNECTOR_LVDS); > - drm_connector_helper_add(connector, &mdp4_lvds_connector_helper_funcs); > - > - connector->polled = 0; > - > - connector->interlace_allowed = 0; > - connector->doublescan_allowed = 0; > - > - drm_connector_attach_encoder(connector, encoder); > - > - return connector; > -} >
On 4/24/2025 3:23 AM, Dmitry Baryshkov wrote: > On Wed, Apr 23, 2025 at 07:04:16PM -0700, Abhinav Kumar wrote: >> >> >> On 2/26/2025 6:25 PM, Dmitry Baryshkov wrote: >>> LVDS support in MDP4 driver makes use of drm_connector directly. However >>> LCDC encoder and LVDS connector are wrappers around drm_panel. Switch >>> them to use drm_panel_bridge/drm_bridge_connector. This allows using >>> standard interface for the drm_panel and also inserting additional >>> bridges between encoder and panel. >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/Makefile | 1 - >>> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 34 +++++-- >>> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h | 6 +- >>> drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 20 +---- >>> .../gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 100 --------------------- >>> 5 files changed, 28 insertions(+), 133 deletions(-) >>> >>> @@ -199,27 +201,43 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms, >>> * bail out early if there is no panel node (no need to >>> * initialize LCDC encoder and LVDS connector) >>> */ >>> - panel_node = of_graph_get_remote_node(dev->dev->of_node, 0, 0); >>> - if (!panel_node) >>> - return 0; >>> + next_bridge = devm_drm_of_get_bridge(dev->dev, dev->dev->of_node, 0, 0); >>> + if (IS_ERR(next_bridge)) { >>> + ret = PTR_ERR(next_bridge); >>> + if (ret == -ENODEV) >>> + return 0; >>> + return ret; >>> + } >>> - encoder = mdp4_lcdc_encoder_init(dev, panel_node); >>> + encoder = mdp4_lcdc_encoder_init(dev); >>> if (IS_ERR(encoder)) { >>> DRM_DEV_ERROR(dev->dev, "failed to construct LCDC encoder\n"); >>> - of_node_put(panel_node); >>> return PTR_ERR(encoder); >>> } >>> /* LCDC can be hooked to DMA_P (TODO: Add DMA_S later?) */ >>> encoder->possible_crtcs = 1 << DMA_P; >>> - connector = mdp4_lvds_connector_init(dev, panel_node, encoder); >>> + ret = drm_bridge_attach(encoder, next_bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); >>> + if (ret) { >>> + DRM_DEV_ERROR(dev->dev, "failed to attach LVDS panel/bridge: %d\n", ret); >>> + >>> + return ret; >>> + } >> >> Can you pls point me to the lvds bridge used with this apq8064 board? I was >> unable to find it. Just wanted to compare that against this while reviewing. > > It's the panel bridge, wrapping one of the LVDS panels. > Yes but what I wanted to check was which LVDS panel was being used so far. Looks like for arm32 the dts is missing? As I couldnt find the lvds out endpoint. So can you pls point me to the lvds panel you verified this with? >>> + >>> + connector = drm_bridge_connector_init(dev, encoder); >>> if (IS_ERR(connector)) { >>> DRM_DEV_ERROR(dev->dev, "failed to initialize LVDS connector\n"); >>> - of_node_put(panel_node); >>> return PTR_ERR(connector); >>> } >>> + ret = drm_connector_attach_encoder(connector, encoder); >>> + if (ret) { >>> + DRM_DEV_ERROR(dev->dev, "failed to attach LVDS connector: %d\n", ret); >>> + >>> + return ret; >>> + } >>> + >>> break; >>> case DRM_MODE_ENCODER_TMDS: >>> encoder = mdp4_dtv_encoder_init(dev); >
On Fri, 25 Apr 2025 at 00:00, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 4/24/2025 3:23 AM, Dmitry Baryshkov wrote: > > On Wed, Apr 23, 2025 at 07:04:16PM -0700, Abhinav Kumar wrote: > >> > >> > >> On 2/26/2025 6:25 PM, Dmitry Baryshkov wrote: > >>> LVDS support in MDP4 driver makes use of drm_connector directly. However > >>> LCDC encoder and LVDS connector are wrappers around drm_panel. Switch > >>> them to use drm_panel_bridge/drm_bridge_connector. This allows using > >>> standard interface for the drm_panel and also inserting additional > >>> bridges between encoder and panel. > >>> > >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>> --- > >>> drivers/gpu/drm/msm/Makefile | 1 - > >>> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 34 +++++-- > >>> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h | 6 +- > >>> drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 20 +---- > >>> .../gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 100 --------------------- > >>> 5 files changed, 28 insertions(+), 133 deletions(-) > >>> > >>> @@ -199,27 +201,43 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms, > >>> * bail out early if there is no panel node (no need to > >>> * initialize LCDC encoder and LVDS connector) > >>> */ > >>> - panel_node = of_graph_get_remote_node(dev->dev->of_node, 0, 0); > >>> - if (!panel_node) > >>> - return 0; > >>> + next_bridge = devm_drm_of_get_bridge(dev->dev, dev->dev->of_node, 0, 0); > >>> + if (IS_ERR(next_bridge)) { > >>> + ret = PTR_ERR(next_bridge); > >>> + if (ret == -ENODEV) > >>> + return 0; > >>> + return ret; > >>> + } > >>> - encoder = mdp4_lcdc_encoder_init(dev, panel_node); > >>> + encoder = mdp4_lcdc_encoder_init(dev); > >>> if (IS_ERR(encoder)) { > >>> DRM_DEV_ERROR(dev->dev, "failed to construct LCDC encoder\n"); > >>> - of_node_put(panel_node); > >>> return PTR_ERR(encoder); > >>> } > >>> /* LCDC can be hooked to DMA_P (TODO: Add DMA_S later?) */ > >>> encoder->possible_crtcs = 1 << DMA_P; > >>> - connector = mdp4_lvds_connector_init(dev, panel_node, encoder); > >>> + ret = drm_bridge_attach(encoder, next_bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); > >>> + if (ret) { > >>> + DRM_DEV_ERROR(dev->dev, "failed to attach LVDS panel/bridge: %d\n", ret); > >>> + > >>> + return ret; > >>> + } > >> > >> Can you pls point me to the lvds bridge used with this apq8064 board? I was > >> unable to find it. Just wanted to compare that against this while reviewing. > > > > It's the panel bridge, wrapping one of the LVDS panels. > > > > Yes but what I wanted to check was which LVDS panel was being used so > far. Looks like for arm32 the dts is missing? As I couldnt find the lvds > out endpoint. So can you pls point me to the lvds panel you verified > this with? I used the AUO b101xtn01 panel connected to the LVDS connector on the IFC6410. I'm not posting DT bits since the panel is not a part of the kit. > > > >>> + > >>> + connector = drm_bridge_connector_init(dev, encoder); > >>> if (IS_ERR(connector)) { > >>> DRM_DEV_ERROR(dev->dev, "failed to initialize LVDS connector\n"); > >>> - of_node_put(panel_node); > >>> return PTR_ERR(connector); > >>> } > >>> + ret = drm_connector_attach_encoder(connector, encoder); > >>> + if (ret) { > >>> + DRM_DEV_ERROR(dev->dev, "failed to attach LVDS connector: %d\n", ret); > >>> + > >>> + return ret; > >>> + } > >>> + > >>> break; > >>> case DRM_MODE_ENCODER_TMDS: > >>> encoder = mdp4_dtv_encoder_init(dev); > > >
On 4/25/2025 2:27 AM, Dmitry Baryshkov wrote: > On Fri, 25 Apr 2025 at 00:00, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 4/24/2025 3:23 AM, Dmitry Baryshkov wrote: >>> On Wed, Apr 23, 2025 at 07:04:16PM -0700, Abhinav Kumar wrote: >>>> >>>> >>>> On 2/26/2025 6:25 PM, Dmitry Baryshkov wrote: >>>>> LVDS support in MDP4 driver makes use of drm_connector directly. However >>>>> LCDC encoder and LVDS connector are wrappers around drm_panel. Switch >>>>> them to use drm_panel_bridge/drm_bridge_connector. This allows using >>>>> standard interface for the drm_panel and also inserting additional >>>>> bridges between encoder and panel. >>>>> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> --- >>>>> drivers/gpu/drm/msm/Makefile | 1 - >>>>> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 34 +++++-- >>>>> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h | 6 +- >>>>> drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 20 +---- >>>>> .../gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 100 --------------------- >>>>> 5 files changed, 28 insertions(+), 133 deletions(-) >>>>> >>>>> @@ -199,27 +201,43 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms, >>>>> * bail out early if there is no panel node (no need to >>>>> * initialize LCDC encoder and LVDS connector) >>>>> */ >>>>> - panel_node = of_graph_get_remote_node(dev->dev->of_node, 0, 0); >>>>> - if (!panel_node) >>>>> - return 0; >>>>> + next_bridge = devm_drm_of_get_bridge(dev->dev, dev->dev->of_node, 0, 0); >>>>> + if (IS_ERR(next_bridge)) { >>>>> + ret = PTR_ERR(next_bridge); >>>>> + if (ret == -ENODEV) >>>>> + return 0; >>>>> + return ret; >>>>> + } >>>>> - encoder = mdp4_lcdc_encoder_init(dev, panel_node); >>>>> + encoder = mdp4_lcdc_encoder_init(dev); >>>>> if (IS_ERR(encoder)) { >>>>> DRM_DEV_ERROR(dev->dev, "failed to construct LCDC encoder\n"); >>>>> - of_node_put(panel_node); >>>>> return PTR_ERR(encoder); >>>>> } >>>>> /* LCDC can be hooked to DMA_P (TODO: Add DMA_S later?) */ >>>>> encoder->possible_crtcs = 1 << DMA_P; >>>>> - connector = mdp4_lvds_connector_init(dev, panel_node, encoder); >>>>> + ret = drm_bridge_attach(encoder, next_bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); >>>>> + if (ret) { >>>>> + DRM_DEV_ERROR(dev->dev, "failed to attach LVDS panel/bridge: %d\n", ret); >>>>> + >>>>> + return ret; >>>>> + } >>>> >>>> Can you pls point me to the lvds bridge used with this apq8064 board? I was >>>> unable to find it. Just wanted to compare that against this while reviewing. >>> >>> It's the panel bridge, wrapping one of the LVDS panels. >>> >> >> Yes but what I wanted to check was which LVDS panel was being used so >> far. Looks like for arm32 the dts is missing? As I couldnt find the lvds >> out endpoint. So can you pls point me to the lvds panel you verified >> this with? > > I used the AUO b101xtn01 panel connected to the LVDS connector on the > IFC6410. I'm not posting DT bits since the panel is not a part of the > kit. > Ok, so at this point of time, this is just the driver piece which does not have a real HW in the tree to be verified with. >> >> >>>>> + >>>>> + connector = drm_bridge_connector_init(dev, encoder); >>>>> if (IS_ERR(connector)) { >>>>> DRM_DEV_ERROR(dev->dev, "failed to initialize LVDS connector\n"); >>>>> - of_node_put(panel_node); >>>>> return PTR_ERR(connector); >>>>> } >>>>> + ret = drm_connector_attach_encoder(connector, encoder); >>>>> + if (ret) { >>>>> + DRM_DEV_ERROR(dev->dev, "failed to attach LVDS connector: %d\n", ret); >>>>> + >>>>> + return ret; >>>>> + } >>>>> + >>>>> break; >>>>> case DRM_MODE_ENCODER_TMDS: >>>>> encoder = mdp4_dtv_encoder_init(dev); >>> >> > >
On Fri, Apr 25, 2025 at 01:01:10PM -0700, Abhinav Kumar wrote: > > > On 4/25/2025 2:27 AM, Dmitry Baryshkov wrote: > > On Fri, 25 Apr 2025 at 00:00, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > > > > > > > > > On 4/24/2025 3:23 AM, Dmitry Baryshkov wrote: > > > > On Wed, Apr 23, 2025 at 07:04:16PM -0700, Abhinav Kumar wrote: > > > > > > > > > > > > > > > On 2/26/2025 6:25 PM, Dmitry Baryshkov wrote: > > > > > > LVDS support in MDP4 driver makes use of drm_connector directly. However > > > > > > LCDC encoder and LVDS connector are wrappers around drm_panel. Switch > > > > > > them to use drm_panel_bridge/drm_bridge_connector. This allows using > > > > > > standard interface for the drm_panel and also inserting additional > > > > > > bridges between encoder and panel. > > > > > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > --- > > > > > > drivers/gpu/drm/msm/Makefile | 1 - > > > > > > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 34 +++++-- > > > > > > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h | 6 +- > > > > > > drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 20 +---- > > > > > > .../gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 100 --------------------- > > > > > > 5 files changed, 28 insertions(+), 133 deletions(-) > > > > > > > > > > > > @@ -199,27 +201,43 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms, > > > > > > * bail out early if there is no panel node (no need to > > > > > > * initialize LCDC encoder and LVDS connector) > > > > > > */ > > > > > > - panel_node = of_graph_get_remote_node(dev->dev->of_node, 0, 0); > > > > > > - if (!panel_node) > > > > > > - return 0; > > > > > > + next_bridge = devm_drm_of_get_bridge(dev->dev, dev->dev->of_node, 0, 0); > > > > > > + if (IS_ERR(next_bridge)) { > > > > > > + ret = PTR_ERR(next_bridge); > > > > > > + if (ret == -ENODEV) > > > > > > + return 0; > > > > > > + return ret; > > > > > > + } > > > > > > - encoder = mdp4_lcdc_encoder_init(dev, panel_node); > > > > > > + encoder = mdp4_lcdc_encoder_init(dev); > > > > > > if (IS_ERR(encoder)) { > > > > > > DRM_DEV_ERROR(dev->dev, "failed to construct LCDC encoder\n"); > > > > > > - of_node_put(panel_node); > > > > > > return PTR_ERR(encoder); > > > > > > } > > > > > > /* LCDC can be hooked to DMA_P (TODO: Add DMA_S later?) */ > > > > > > encoder->possible_crtcs = 1 << DMA_P; > > > > > > - connector = mdp4_lvds_connector_init(dev, panel_node, encoder); > > > > > > + ret = drm_bridge_attach(encoder, next_bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); > > > > > > + if (ret) { > > > > > > + DRM_DEV_ERROR(dev->dev, "failed to attach LVDS panel/bridge: %d\n", ret); > > > > > > + > > > > > > + return ret; > > > > > > + } > > > > > > > > > > Can you pls point me to the lvds bridge used with this apq8064 board? I was > > > > > unable to find it. Just wanted to compare that against this while reviewing. > > > > > > > > It's the panel bridge, wrapping one of the LVDS panels. > > > > > > > > > > Yes but what I wanted to check was which LVDS panel was being used so > > > far. Looks like for arm32 the dts is missing? As I couldnt find the lvds > > > out endpoint. So can you pls point me to the lvds panel you verified > > > this with? > > > > I used the AUO b101xtn01 panel connected to the LVDS connector on the > > IFC6410. I'm not posting DT bits since the panel is not a part of the > > kit. > > > > Ok, so at this point of time, this is just the driver piece which does not > have a real HW in the tree to be verified with. More or less so. Srini tried to push the panel in 2015 ([1]). PR was rejected and then nobody ever implemented EDID-based selection for LVDS panels. I simply soldered the LVDS cable and used more or less the same patch (I can post it separately if necessary). To verify the pipeline you can use any LVDS panel defined in the DT, it should be enough to verify it. [1] https://lore.kernel.org/linux-arm-msm/1438088076-17606-1-git-send-email-srinivas.kandagatla@linaro.org/ > > > > > > > > > > > > > > + > > > > > > + connector = drm_bridge_connector_init(dev, encoder); > > > > > > if (IS_ERR(connector)) { > > > > > > DRM_DEV_ERROR(dev->dev, "failed to initialize LVDS connector\n"); > > > > > > - of_node_put(panel_node); > > > > > > return PTR_ERR(connector); > > > > > > } > > > > > > + ret = drm_connector_attach_encoder(connector, encoder); > > > > > > + if (ret) { > > > > > > + DRM_DEV_ERROR(dev->dev, "failed to attach LVDS connector: %d\n", ret); > > > > > > + > > > > > > + return ret; > > > > > > + } > > > > > > + > > > > > > break; > > > > > > case DRM_MODE_ENCODER_TMDS: > > > > > > encoder = mdp4_dtv_encoder_init(dev); > > > > > > > > > > > >
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 5df20cbeafb8bf07c825a1fd72719d5a56c38613..7a2ada6e2d74a902879e4f12a78ed475e5209ec2 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -48,7 +48,6 @@ msm-display-$(CONFIG_DRM_MSM_MDP4) += \ disp/mdp4/mdp4_dsi_encoder.o \ disp/mdp4/mdp4_dtv_encoder.o \ disp/mdp4/mdp4_lcdc_encoder.o \ - disp/mdp4/mdp4_lvds_connector.o \ disp/mdp4/mdp4_lvds_pll.o \ disp/mdp4/mdp4_irq.o \ disp/mdp4/mdp4_kms.o \ diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c index 689e210660a5218ed1e2d116073723215af5a187..93c9411eb422bc67b7fedb5ffce4c330310b520f 100644 --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c @@ -6,6 +6,8 @@ #include <linux/delay.h> +#include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_vblank.h> #include "msm_drv.h" @@ -189,7 +191,7 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms, struct msm_drm_private *priv = dev->dev_private; struct drm_encoder *encoder; struct drm_connector *connector; - struct device_node *panel_node; + struct drm_bridge *next_bridge; int dsi_id; int ret; @@ -199,27 +201,43 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms, * bail out early if there is no panel node (no need to * initialize LCDC encoder and LVDS connector) */ - panel_node = of_graph_get_remote_node(dev->dev->of_node, 0, 0); - if (!panel_node) - return 0; + next_bridge = devm_drm_of_get_bridge(dev->dev, dev->dev->of_node, 0, 0); + if (IS_ERR(next_bridge)) { + ret = PTR_ERR(next_bridge); + if (ret == -ENODEV) + return 0; + return ret; + } - encoder = mdp4_lcdc_encoder_init(dev, panel_node); + encoder = mdp4_lcdc_encoder_init(dev); if (IS_ERR(encoder)) { DRM_DEV_ERROR(dev->dev, "failed to construct LCDC encoder\n"); - of_node_put(panel_node); return PTR_ERR(encoder); } /* LCDC can be hooked to DMA_P (TODO: Add DMA_S later?) */ encoder->possible_crtcs = 1 << DMA_P; - connector = mdp4_lvds_connector_init(dev, panel_node, encoder); + ret = drm_bridge_attach(encoder, next_bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (ret) { + DRM_DEV_ERROR(dev->dev, "failed to attach LVDS panel/bridge: %d\n", ret); + + return ret; + } + + connector = drm_bridge_connector_init(dev, encoder); if (IS_ERR(connector)) { DRM_DEV_ERROR(dev->dev, "failed to initialize LVDS connector\n"); - of_node_put(panel_node); return PTR_ERR(connector); } + ret = drm_connector_attach_encoder(connector, encoder); + if (ret) { + DRM_DEV_ERROR(dev->dev, "failed to attach LVDS connector: %d\n", ret); + + return ret; + } + break; case DRM_MODE_ENCODER_TMDS: encoder = mdp4_dtv_encoder_init(dev); diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h index e0380d3b7e0cee99c4c376bf6369887106f44ede..306f5ca8f810aaeecea56e74065933bbffcb67ec 100644 --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h @@ -191,11 +191,7 @@ struct drm_crtc *mdp4_crtc_init(struct drm_device *dev, long mdp4_dtv_round_pixclk(struct drm_encoder *encoder, unsigned long rate); struct drm_encoder *mdp4_dtv_encoder_init(struct drm_device *dev); -struct drm_encoder *mdp4_lcdc_encoder_init(struct drm_device *dev, - struct device_node *panel_node); - -struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev, - struct device_node *panel_node, struct drm_encoder *encoder); +struct drm_encoder *mdp4_lcdc_encoder_init(struct drm_device *dev); #ifdef CONFIG_DRM_MSM_DSI struct drm_encoder *mdp4_dsi_encoder_init(struct drm_device *dev); diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c index cfcedd8a635cf0297365e845ef415a8f0d553183..a4f3edabefbd06286bfb8fbcd7f8c0a4281e5ef1 100644 --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c @@ -14,7 +14,6 @@ struct mdp4_lcdc_encoder { struct drm_encoder base; - struct device_node *panel_node; struct drm_panel *panel; struct clk *lcdc_clk; unsigned long int pixclock; @@ -262,19 +261,12 @@ static void mdp4_lcdc_encoder_disable(struct drm_encoder *encoder) struct mdp4_lcdc_encoder *mdp4_lcdc_encoder = to_mdp4_lcdc_encoder(encoder); struct mdp4_kms *mdp4_kms = get_kms(encoder); - struct drm_panel *panel; if (WARN_ON(!mdp4_lcdc_encoder->enabled)) return; mdp4_write(mdp4_kms, REG_MDP4_LCDC_ENABLE, 0); - panel = of_drm_find_panel(mdp4_lcdc_encoder->panel_node); - if (!IS_ERR(panel)) { - drm_panel_disable(panel); - drm_panel_unprepare(panel); - } - /* * Wait for a vsync so we know the ENABLE=0 latched before * the (connector) source of the vsync's gets disabled, @@ -300,7 +292,6 @@ static void mdp4_lcdc_encoder_enable(struct drm_encoder *encoder) to_mdp4_lcdc_encoder(encoder); unsigned long pc = mdp4_lcdc_encoder->pixclock; struct mdp4_kms *mdp4_kms = get_kms(encoder); - struct drm_panel *panel; uint32_t config; int ret; @@ -335,12 +326,6 @@ static void mdp4_lcdc_encoder_enable(struct drm_encoder *encoder) if (ret) DRM_DEV_ERROR(dev->dev, "failed to enable lcdc_clk: %d\n", ret); - panel = of_drm_find_panel(mdp4_lcdc_encoder->panel_node); - if (!IS_ERR(panel)) { - drm_panel_prepare(panel); - drm_panel_enable(panel); - } - setup_phy(encoder); mdp4_write(mdp4_kms, REG_MDP4_LCDC_ENABLE, 1); @@ -375,8 +360,7 @@ static const struct drm_encoder_helper_funcs mdp4_lcdc_encoder_helper_funcs = { }; /* initialize encoder */ -struct drm_encoder *mdp4_lcdc_encoder_init(struct drm_device *dev, - struct device_node *panel_node) +struct drm_encoder *mdp4_lcdc_encoder_init(struct drm_device *dev) { struct drm_encoder *encoder; struct mdp4_lcdc_encoder *mdp4_lcdc_encoder; @@ -387,8 +371,6 @@ struct drm_encoder *mdp4_lcdc_encoder_init(struct drm_device *dev, if (IS_ERR(mdp4_lcdc_encoder)) return ERR_CAST(mdp4_lcdc_encoder); - mdp4_lcdc_encoder->panel_node = panel_node; - encoder = &mdp4_lcdc_encoder->base; drm_encoder_helper_add(encoder, &mdp4_lcdc_encoder_helper_funcs); diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c deleted file mode 100644 index 4755eb13ef79f313d2be088145c8cd2e615226fe..0000000000000000000000000000000000000000 --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c +++ /dev/null @@ -1,100 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/* - * Copyright (C) 2014 Red Hat - * Author: Rob Clark <robdclark@gmail.com> - * Author: Vinay Simha <vinaysimha@inforcecomputing.com> - */ - -#include "mdp4_kms.h" - -struct mdp4_lvds_connector { - struct drm_connector base; - struct drm_encoder *encoder; - struct device_node *panel_node; - struct drm_panel *panel; -}; -#define to_mdp4_lvds_connector(x) container_of(x, struct mdp4_lvds_connector, base) - -static enum drm_connector_status mdp4_lvds_connector_detect( - struct drm_connector *connector, bool force) -{ - struct mdp4_lvds_connector *mdp4_lvds_connector = - to_mdp4_lvds_connector(connector); - - if (!mdp4_lvds_connector->panel) { - mdp4_lvds_connector->panel = - of_drm_find_panel(mdp4_lvds_connector->panel_node); - if (IS_ERR(mdp4_lvds_connector->panel)) - mdp4_lvds_connector->panel = NULL; - } - - return mdp4_lvds_connector->panel ? - connector_status_connected : - connector_status_disconnected; -} - -static void mdp4_lvds_connector_destroy(struct drm_connector *connector) -{ - struct mdp4_lvds_connector *mdp4_lvds_connector = - to_mdp4_lvds_connector(connector); - - drm_connector_cleanup(connector); - - kfree(mdp4_lvds_connector); -} - -static int mdp4_lvds_connector_get_modes(struct drm_connector *connector) -{ - struct mdp4_lvds_connector *mdp4_lvds_connector = - to_mdp4_lvds_connector(connector); - struct drm_panel *panel = mdp4_lvds_connector->panel; - int ret = 0; - - if (panel) - ret = drm_panel_get_modes(panel, connector); - - return ret; -} - -static const struct drm_connector_funcs mdp4_lvds_connector_funcs = { - .detect = mdp4_lvds_connector_detect, - .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = mdp4_lvds_connector_destroy, - .reset = drm_atomic_helper_connector_reset, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, -}; - -static const struct drm_connector_helper_funcs mdp4_lvds_connector_helper_funcs = { - .get_modes = mdp4_lvds_connector_get_modes, -}; - -/* initialize connector */ -struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev, - struct device_node *panel_node, struct drm_encoder *encoder) -{ - struct drm_connector *connector = NULL; - struct mdp4_lvds_connector *mdp4_lvds_connector; - - mdp4_lvds_connector = kzalloc(sizeof(*mdp4_lvds_connector), GFP_KERNEL); - if (!mdp4_lvds_connector) - return ERR_PTR(-ENOMEM); - - mdp4_lvds_connector->encoder = encoder; - mdp4_lvds_connector->panel_node = panel_node; - - connector = &mdp4_lvds_connector->base; - - drm_connector_init(dev, connector, &mdp4_lvds_connector_funcs, - DRM_MODE_CONNECTOR_LVDS); - drm_connector_helper_add(connector, &mdp4_lvds_connector_helper_funcs); - - connector->polled = 0; - - connector->interlace_allowed = 0; - connector->doublescan_allowed = 0; - - drm_connector_attach_encoder(connector, encoder); - - return connector; -}
LVDS support in MDP4 driver makes use of drm_connector directly. However LCDC encoder and LVDS connector are wrappers around drm_panel. Switch them to use drm_panel_bridge/drm_bridge_connector. This allows using standard interface for the drm_panel and also inserting additional bridges between encoder and panel. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/Makefile | 1 - drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 34 +++++-- drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h | 6 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 20 +---- .../gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 100 --------------------- 5 files changed, 28 insertions(+), 133 deletions(-)