The drm_display_mode_to_videomode() does not populate DISPLAY_FLAGS_DE_LOW or DISPLAY_FLAGS_PIXDATA_NEGEDGE flags in struct videomode. Therefore, no matter what polarity the next bridge or display might require, these flags are never set, and thus the LTDC GCR_DEPOL and GCR_PCPOL bits are never set, and the LTDC behaves as if both DISPLAY_FLAGS_PIXDATA_POSEDGE and DISPLAY_FLAGS_DE_HIGH were always set.
The fix for this problem is taken almost verbatim from MXSFB driver. In case there is a bridge attached to the LTDC, the bridge might have extra polarity requirements, so extract bus_flags from the bridge and use them for LTDC configuration. Otherwise, extract bus_flags from the connector, which is the display.
Fixes: b759012c5fa7 ("drm/stm: Add STM32 LTDC driver") Signed-off-by: Marek Vasut marex@denx.de Cc: Alexandre Torgue alexandre.torgue@st.com Cc: Antonio Borneo antonio.borneo@st.com Cc: Benjamin Gaignard benjamin.gaignard@st.com Cc: Maxime Coquelin mcoquelin.stm32@gmail.com Cc: Philippe Cornu philippe.cornu@st.com Cc: Sam Ravnborg sam@ravnborg.org Cc: Vincent Abriou vincent.abriou@st.com Cc: Yannick Fertre yannick.fertre@st.com Cc: linux-arm-kernel@lists.infradead.org Cc: linux-stm32@st-md-mailman.stormreply.com To: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/stm/ltdc.c | 22 ++++++++++++++++++++-- drivers/gpu/drm/stm/ltdc.h | 2 ++ 2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c index 07c73079293c..a282a1553497 100644 --- a/drivers/gpu/drm/stm/ltdc.c +++ b/drivers/gpu/drm/stm/ltdc.c @@ -546,11 +546,17 @@ static void ltdc_crtc_mode_set_nofb(struct drm_crtc *crtc) struct drm_device *ddev = crtc->dev; struct drm_display_mode *mode = &crtc->state->adjusted_mode; struct videomode vm; + u32 bus_flags = 0; u32 hsync, vsync, accum_hbp, accum_vbp, accum_act_w, accum_act_h; u32 total_width, total_height; u32 val; int ret;
+ if (ldev->bridge) + bus_flags = ldev->bridge->timings->input_bus_flags; + else if (ldev->connector) + bus_flags = ldev->connector->display_info.bus_flags; + if (!pm_runtime_active(ddev->dev)) { ret = pm_runtime_get_sync(ddev->dev); if (ret) { @@ -586,10 +592,10 @@ static void ltdc_crtc_mode_set_nofb(struct drm_crtc *crtc) if (vm.flags & DISPLAY_FLAGS_VSYNC_HIGH) val |= GCR_VSPOL;
- if (vm.flags & DISPLAY_FLAGS_DE_LOW) + if (bus_flags & DRM_BUS_FLAG_DE_LOW) val |= GCR_DEPOL;
- if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE) + if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) val |= GCR_PCPOL;
reg_update_bits(ldev->regs, LTDC_GCR, @@ -1098,6 +1104,8 @@ static const struct drm_encoder_helper_funcs ltdc_encoder_helper_funcs = {
static int ltdc_encoder_init(struct drm_device *ddev, struct drm_bridge *bridge) { + struct ltdc_device *ldev = ddev->dev_private; + struct drm_connector_list_iter iter; struct drm_encoder *encoder; int ret;
@@ -1119,6 +1127,16 @@ static int ltdc_encoder_init(struct drm_device *ddev, struct drm_bridge *bridge) return -EINVAL; }
+ ldev->bridge = bridge; + + /* + * Get hold of the connector. This is a bit of a hack, until the bridge + * API gives us bus flags and formats. + */ + drm_connector_list_iter_begin(ddev, &iter); + ldev->connector = drm_connector_list_iter_next(&iter); + drm_connector_list_iter_end(&iter); + DRM_DEBUG_DRIVER("Bridge encoder:%d created\n", encoder->base.id);
return 0; diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h index f153b908c70e..d0d2c81de29a 100644 --- a/drivers/gpu/drm/stm/ltdc.h +++ b/drivers/gpu/drm/stm/ltdc.h @@ -38,6 +38,8 @@ struct ltdc_device { u32 irq_status; struct fps_info plane_fpsi[LTDC_MAX_LAYER]; struct drm_atomic_state *suspend_state; + struct drm_bridge *bridge; + struct drm_connector *connector; };
int ltdc_load(struct drm_device *ddev);
Hi Marek.
On Sat, Oct 03, 2020 at 01:07:26AM +0200, Marek Vasut wrote:
The drm_display_mode_to_videomode() does not populate DISPLAY_FLAGS_DE_LOW or DISPLAY_FLAGS_PIXDATA_NEGEDGE flags in struct videomode.
So after reading this paragrahp I assumed this patch would fix this, alas there is no changes to drm_modes.c. Rather than introducing hacks could we try to fix the root cause here?
Root cause - as you point out is that there is missing some flags in the conversion. Another point is that the stm driver could drop the use of drm_display_mode_to_videomode() - all the information is available in drm_display_mode and using drm_display_mode_to_videomode() does not help much.
If the point is to have easier access to hsync and friendns maybe we should have a few helpers operating direct on display_mode and not this conversion to video_mode which is something that belongs to fbdev and should IMO not be used inside a drm driver.
Sam
Therefore, no matter what polarity the next bridge or display might require, these flags are never set, and thus the LTDC GCR_DEPOL and GCR_PCPOL bits are never set, and the LTDC behaves as if both DISPLAY_FLAGS_PIXDATA_POSEDGE and DISPLAY_FLAGS_DE_HIGH were always set.
The fix for this problem is taken almost verbatim from MXSFB driver. In case there is a bridge attached to the LTDC, the bridge might have extra polarity requirements, so extract bus_flags from the bridge and use them for LTDC configuration. Otherwise, extract bus_flags from the connector, which is the display.
Fixes: b759012c5fa7 ("drm/stm: Add STM32 LTDC driver") Signed-off-by: Marek Vasut marex@denx.de Cc: Alexandre Torgue alexandre.torgue@st.com Cc: Antonio Borneo antonio.borneo@st.com Cc: Benjamin Gaignard benjamin.gaignard@st.com Cc: Maxime Coquelin mcoquelin.stm32@gmail.com Cc: Philippe Cornu philippe.cornu@st.com Cc: Sam Ravnborg sam@ravnborg.org Cc: Vincent Abriou vincent.abriou@st.com Cc: Yannick Fertre yannick.fertre@st.com Cc: linux-arm-kernel@lists.infradead.org Cc: linux-stm32@st-md-mailman.stormreply.com To: dri-devel@lists.freedesktop.org
drivers/gpu/drm/stm/ltdc.c | 22 ++++++++++++++++++++-- drivers/gpu/drm/stm/ltdc.h | 2 ++ 2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c index 07c73079293c..a282a1553497 100644 --- a/drivers/gpu/drm/stm/ltdc.c +++ b/drivers/gpu/drm/stm/ltdc.c @@ -546,11 +546,17 @@ static void ltdc_crtc_mode_set_nofb(struct drm_crtc *crtc) struct drm_device *ddev = crtc->dev; struct drm_display_mode *mode = &crtc->state->adjusted_mode; struct videomode vm;
u32 bus_flags = 0; u32 hsync, vsync, accum_hbp, accum_vbp, accum_act_w, accum_act_h; u32 total_width, total_height; u32 val; int ret;
if (ldev->bridge)
bus_flags = ldev->bridge->timings->input_bus_flags;else if (ldev->connector)
bus_flags = ldev->connector->display_info.bus_flags;if (!pm_runtime_active(ddev->dev)) { ret = pm_runtime_get_sync(ddev->dev); if (ret) {
@@ -586,10 +592,10 @@ static void ltdc_crtc_mode_set_nofb(struct drm_crtc *crtc) if (vm.flags & DISPLAY_FLAGS_VSYNC_HIGH) val |= GCR_VSPOL;
- if (vm.flags & DISPLAY_FLAGS_DE_LOW)
- if (bus_flags & DRM_BUS_FLAG_DE_LOW) val |= GCR_DEPOL;
- if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) val |= GCR_PCPOL;
reg_update_bits(ldev->regs, LTDC_GCR,
@@ -1098,6 +1104,8 @@ static const struct drm_encoder_helper_funcs ltdc_encoder_helper_funcs = {
static int ltdc_encoder_init(struct drm_device *ddev, struct drm_bridge *bridge) {
- struct ltdc_device *ldev = ddev->dev_private;
- struct drm_connector_list_iter iter; struct drm_encoder *encoder; int ret;
@@ -1119,6 +1127,16 @@ static int ltdc_encoder_init(struct drm_device *ddev, struct drm_bridge *bridge) return -EINVAL; }
ldev->bridge = bridge;
/*
* Get hold of the connector. This is a bit of a hack, until the bridge* API gives us bus flags and formats.*/drm_connector_list_iter_begin(ddev, &iter);
ldev->connector = drm_connector_list_iter_next(&iter);
drm_connector_list_iter_end(&iter);
DRM_DEBUG_DRIVER("Bridge encoder:%d created\n", encoder->base.id);
return 0;
diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h index f153b908c70e..d0d2c81de29a 100644 --- a/drivers/gpu/drm/stm/ltdc.h +++ b/drivers/gpu/drm/stm/ltdc.h @@ -38,6 +38,8 @@ struct ltdc_device { u32 irq_status; struct fps_info plane_fpsi[LTDC_MAX_LAYER]; struct drm_atomic_state *suspend_state;
- struct drm_bridge *bridge;
- struct drm_connector *connector;
};
int ltdc_load(struct drm_device *ddev);
2.28.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 10/16/20 7:44 PM, Sam Ravnborg wrote:
Hi Marek.
Hi,
On Sat, Oct 03, 2020 at 01:07:26AM +0200, Marek Vasut wrote:
The drm_display_mode_to_videomode() does not populate DISPLAY_FLAGS_DE_LOW or DISPLAY_FLAGS_PIXDATA_NEGEDGE flags in struct videomode.
So after reading this paragrahp I assumed this patch would fix this, alas there is no changes to drm_modes.c. Rather than introducing hacks could we try to fix the root cause here?
Nope, this is a driver fix, hence the drm/stm tags.
Root cause - as you point out is that there is missing some flags in the conversion. Another point is that the stm driver could drop the use of drm_display_mode_to_videomode() - all the information is available in drm_display_mode and using drm_display_mode_to_videomode() does not help much.
I kinda already have such a patch in the tree to remove this, but I want to have a patch which fixes the problem described here and can be easily backported to stable too.
If the point is to have easier access to hsync and friendns maybe we should have a few helpers operating direct on display_mode and not this conversion to video_mode which is something that belongs to fbdev and should IMO not be used inside a drm driver.
Yep, that is for another patch though, because if that one breaks something, I want to be able to bisect it.
dri-devel@lists.freedesktop.org
-
Marek Vasut -
Sam Ravnborg