| Message ID | 20210124085552.29146-5-paul@crapouillou.net |
|---|---|
| State | Accepted |
| Commit | 7b4957684e5d813fcbdc98144e3cc5c4467b3e2e |
| Headers | show |
| Series | [v3,1/4] drm: bridge/panel: Cleanup connector on bridge detach | expand |
Hi Paul, we observed the same issue on the jz4730 (which is almost identical to the jz4740 wrt. LCDC) and our solution [1] was simpler. It leaves the hwdesc f0 and f1 as they are and just takes f1 for really programming the first DMA descriptor if there is no OSD. We have tested on jz4730 and jz4780. Maybe you want to consider that. Then I can officially post it. [1] https://github.com/goldelico/letux-kernel/commit/3be1de5fdabf2cc1c17f198ded3328cc6e4b9844 > Am 24.01.2021 um 09:55 schrieb Paul Cercueil <paul@crapouillou.net>: > > Even though the JZ4740 did not have the OSD mode, it had (according to > the documentation) two DMA channels, but there is absolutely no > information about how to select the second DMA channel. > > Make the ingenic-drm driver work in non-OSD mode by using the > foreground0 plane (which is bound to the DMA0 channel) as the primary > plane, instead of the foreground1 plane, which is the primary plane > when in OSD mode. > > Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode") > Cc: <stable@vger.kernel.org> # v5.8+ > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index b23011c1c5d9..59ce43862e16 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, > height = state->src_h >> 16; > cpp = state->fb->format->cpp[0]; > > - if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY) > + if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY) > hwdesc = &priv->dma_hwdescs->hwdesc_f0; > else > hwdesc = &priv->dma_hwdescs->hwdesc_f1; we just replace this with if (priv->soc_info->has_osd && plane->type != DRM_PLANE_TYPE_OVERLAY) hwdesc = &priv->dma_hwdescs->hwdesc_f1; else hwdesc = &priv->dma_hwdescs->hwdesc_f0; and the remainder can stay as is. > @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) > const struct jz_soc_info *soc_info; > struct ingenic_drm *priv; > struct clk *parent_clk; > + struct drm_plane *primary; > struct drm_bridge *bridge; > struct drm_panel *panel; > struct drm_encoder *encoder; > @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) > if (soc_info->has_osd) > priv->ipu_plane = drm_plane_from_index(drm, 0); > > - drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs); > + primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0; > > - ret = drm_universal_plane_init(drm, &priv->f1, 1, > + drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs); > + > + ret = drm_universal_plane_init(drm, primary, 1, > &ingenic_drm_primary_plane_funcs, > priv->soc_info->formats_f1, > priv->soc_info->num_formats_f1, > @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) > > drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs); > > - ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1, > + ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary, > NULL, &ingenic_drm_crtc_funcs, NULL); > if (ret) { > dev_err(dev, "Failed to init CRTC: %i\n", ret); > -- > 2.29.2 > BR and thanks, Nikolaus
Hi Nikolaus, Le dim. 24 janv. 2021 à 10:30, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Paul, > we observed the same issue on the jz4730 (which is almost identical > to the jz4740 wrt. LCDC) and our solution [1] was simpler. > > It leaves the hwdesc f0 and f1 as they are and just takes f1 for > really > programming the first DMA descriptor if there is no OSD. Disagreed. With your solution, it ends up using priv->f1 plane with hwdesc_f0. That's very confusing. > We have tested on jz4730 and jz4780. Could I get a tested-by then? :) Cheers, -Paul > Maybe you want to consider that. Then I can officially post it. > > [1] > https://github.com/goldelico/letux-kernel/commit/3be1de5fdabf2cc1c17f198ded3328cc6e4b9844 > >> Am 24.01.2021 um 09:55 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> Even though the JZ4740 did not have the OSD mode, it had (according >> to >> the documentation) two DMA channels, but there is absolutely no >> information about how to select the second DMA channel. >> >> Make the ingenic-drm driver work in non-OSD mode by using the >> foreground0 plane (which is bound to the DMA0 channel) as the >> primary >> plane, instead of the foreground1 plane, which is the primary plane >> when in OSD mode. >> >> Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode") >> Cc: <stable@vger.kernel.org> # v5.8+ >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- >> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> index b23011c1c5d9..59ce43862e16 100644 >> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> @@ -554,7 +554,7 @@ static void >> ingenic_drm_plane_atomic_update(struct drm_plane *plane, >> height = state->src_h >> 16; >> cpp = state->fb->format->cpp[0]; >> >> - if (priv->soc_info->has_osd && plane->type == >> DRM_PLANE_TYPE_OVERLAY) >> + if (!priv->soc_info->has_osd || plane->type == >> DRM_PLANE_TYPE_OVERLAY) >> hwdesc = &priv->dma_hwdescs->hwdesc_f0; >> else >> hwdesc = &priv->dma_hwdescs->hwdesc_f1; > > we just replace this with > > if (priv->soc_info->has_osd && plane->type != > DRM_PLANE_TYPE_OVERLAY) > hwdesc = &priv->dma_hwdescs->hwdesc_f1; > else > hwdesc = &priv->dma_hwdescs->hwdesc_f0; > > and the remainder can stay as is. > >> @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, >> bool has_components) >> const struct jz_soc_info *soc_info; >> struct ingenic_drm *priv; >> struct clk *parent_clk; >> + struct drm_plane *primary; >> struct drm_bridge *bridge; >> struct drm_panel *panel; >> struct drm_encoder *encoder; >> @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device >> *dev, bool has_components) >> if (soc_info->has_osd) >> priv->ipu_plane = drm_plane_from_index(drm, 0); >> >> - drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs); >> + primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0; >> >> - ret = drm_universal_plane_init(drm, &priv->f1, 1, >> + drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs); >> + >> + ret = drm_universal_plane_init(drm, primary, 1, >> &ingenic_drm_primary_plane_funcs, >> priv->soc_info->formats_f1, >> priv->soc_info->num_formats_f1, >> @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, >> bool has_components) >> >> drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs); >> >> - ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1, >> + ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary, >> NULL, &ingenic_drm_crtc_funcs, NULL); >> if (ret) { >> dev_err(dev, "Failed to init CRTC: %i\n", ret); >> -- >> 2.29.2 >> > > BR and thanks, > Nikolaus >
Hi Paul, > Am 24.01.2021 um 10:47 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > > Hi Paul, > >> Am 24.01.2021 um 10:43 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> Hi Nikolaus, >> >> Le dim. 24 janv. 2021 à 10:30, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >>> Hi Paul, >>> we observed the same issue on the jz4730 (which is almost identical >>> to the jz4740 wrt. LCDC) and our solution [1] was simpler. >>> It leaves the hwdesc f0 and f1 as they are and just takes f1 for really >>> programming the first DMA descriptor if there is no OSD. >> >> Disagreed. With your solution, it ends up using priv->f1 plane with hwdesc_f0. That's very confusing. > > It is a tradeoff between code simplicity and confusion. Indeed difficult to decide. Fortunately I don't have to :) > >> >>> We have tested on jz4730 and jz4780. >> >> Could I get a tested-by then? :) > > I'll test with our tree and test both SoC in the next days and give feedback. Ok, works on both devices: a) Skytone Alpha 400 compatible Letux 400 with not-yet-upstream jz4730 and internal panel b) CI20 with external HDMI (tablet) and our not-yet-upstream HDMI code So the hwdesc handling is as ok as it was with our patch. I'll keep a copy of yours in our tree until it arrives upstream. Tested-by: H. Nikolaus Schaller <hns@goldelico.com> BR and thanks, Nikolaus > > BR and thanks, > Nikolaus > >> >> Cheers, >> -Paul >> >>> Maybe you want to consider that. Then I can officially post it. >>> [1] https://github.com/goldelico/letux-kernel/commit/3be1de5fdabf2cc1c17f198ded3328cc6e4b9844 >>>> Am 24.01.2021 um 09:55 schrieb Paul Cercueil <paul@crapouillou.net>: >>>> Even though the JZ4740 did not have the OSD mode, it had (according to >>>> the documentation) two DMA channels, but there is absolutely no >>>> information about how to select the second DMA channel. >>>> Make the ingenic-drm driver work in non-OSD mode by using the >>>> foreground0 plane (which is bound to the DMA0 channel) as the primary >>>> plane, instead of the foreground1 plane, which is the primary plane >>>> when in OSD mode. >>>> Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode") >>>> Cc: <stable@vger.kernel.org> # v5.8+ >>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> --- >>>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++---- >>>> 1 file changed, 7 insertions(+), 4 deletions(-) >>>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >>>> index b23011c1c5d9..59ce43862e16 100644 >>>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >>>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >>>> @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, >>>> height = state->src_h >> 16; >>>> cpp = state->fb->format->cpp[0]; >>>> - if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY) >>>> + if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY) >>>> hwdesc = &priv->dma_hwdescs->hwdesc_f0; >>>> else >>>> hwdesc = &priv->dma_hwdescs->hwdesc_f1; >>> we just replace this with >>> if (priv->soc_info->has_osd && plane->type != DRM_PLANE_TYPE_OVERLAY) >>> hwdesc = &priv->dma_hwdescs->hwdesc_f1; >>> else >>> hwdesc = &priv->dma_hwdescs->hwdesc_f0; >>> and the remainder can stay as is. >>>> @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) >>>> const struct jz_soc_info *soc_info; >>>> struct ingenic_drm *priv; >>>> struct clk *parent_clk; >>>> + struct drm_plane *primary; >>>> struct drm_bridge *bridge; >>>> struct drm_panel *panel; >>>> struct drm_encoder *encoder; >>>> @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) >>>> if (soc_info->has_osd) >>>> priv->ipu_plane = drm_plane_from_index(drm, 0); >>>> - drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs); >>>> + primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0; >>>> - ret = drm_universal_plane_init(drm, &priv->f1, 1, >>>> + drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs); >>>> + >>>> + ret = drm_universal_plane_init(drm, primary, 1, >>>> &ingenic_drm_primary_plane_funcs, >>>> priv->soc_info->formats_f1, >>>> priv->soc_info->num_formats_f1, >>>> @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) >>>> drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs); >>>> - ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1, >>>> + ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary, >>>> NULL, &ingenic_drm_crtc_funcs, NULL); >>>> if (ret) { >>>> dev_err(dev, "Failed to init CRTC: %i\n", ret); >>>> -- >>>> 2.29.2 >>> BR and thanks, >>> Nikolaus >> >> >
Hi Paul, Thank you for the patch. On Sun, Jan 24, 2021 at 08:55:52AM +0000, Paul Cercueil wrote: > Even though the JZ4740 did not have the OSD mode, it had (according to > the documentation) two DMA channels, but there is absolutely no > information about how to select the second DMA channel. > > Make the ingenic-drm driver work in non-OSD mode by using the > foreground0 plane (which is bound to the DMA0 channel) as the primary > plane, instead of the foreground1 plane, which is the primary plane > when in OSD mode. > > Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode") > Cc: <stable@vger.kernel.org> # v5.8+ > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> I don't have much knowledge about this platform, but the change looks reasonable to me. Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index b23011c1c5d9..59ce43862e16 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, > height = state->src_h >> 16; > cpp = state->fb->format->cpp[0]; > > - if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY) > + if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY) > hwdesc = &priv->dma_hwdescs->hwdesc_f0; > else > hwdesc = &priv->dma_hwdescs->hwdesc_f1; > @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) > const struct jz_soc_info *soc_info; > struct ingenic_drm *priv; > struct clk *parent_clk; > + struct drm_plane *primary; > struct drm_bridge *bridge; > struct drm_panel *panel; > struct drm_encoder *encoder; > @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) > if (soc_info->has_osd) > priv->ipu_plane = drm_plane_from_index(drm, 0); > > - drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs); > + primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0; > > - ret = drm_universal_plane_init(drm, &priv->f1, 1, > + drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs); > + > + ret = drm_universal_plane_init(drm, primary, 1, > &ingenic_drm_primary_plane_funcs, > priv->soc_info->formats_f1, > priv->soc_info->num_formats_f1, > @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) > > drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs); > > - ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1, > + ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary, > NULL, &ingenic_drm_crtc_funcs, NULL); > if (ret) { > dev_err(dev, "Failed to init CRTC: %i\n", ret);
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index b23011c1c5d9..59ce43862e16 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, height = state->src_h >> 16; cpp = state->fb->format->cpp[0]; - if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY) + if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY) hwdesc = &priv->dma_hwdescs->hwdesc_f0; else hwdesc = &priv->dma_hwdescs->hwdesc_f1; @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) const struct jz_soc_info *soc_info; struct ingenic_drm *priv; struct clk *parent_clk; + struct drm_plane *primary; struct drm_bridge *bridge; struct drm_panel *panel; struct drm_encoder *encoder; @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) if (soc_info->has_osd) priv->ipu_plane = drm_plane_from_index(drm, 0); - drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs); + primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0; - ret = drm_universal_plane_init(drm, &priv->f1, 1, + drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs); + + ret = drm_universal_plane_init(drm, primary, 1, &ingenic_drm_primary_plane_funcs, priv->soc_info->formats_f1, priv->soc_info->num_formats_f1, @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs); - ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1, + ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary, NULL, &ingenic_drm_crtc_funcs, NULL); if (ret) { dev_err(dev, "Failed to init CRTC: %i\n", ret);
Even though the JZ4740 did not have the OSD mode, it had (according to the documentation) two DMA channels, but there is absolutely no information about how to select the second DMA channel. Make the ingenic-drm driver work in non-OSD mode by using the foreground0 plane (which is bound to the DMA0 channel) as the primary plane, instead of the foreground1 plane, which is the primary plane when in OSD mode. Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode") Cc: <stable@vger.kernel.org> # v5.8+ Signed-off-by: Paul Cercueil <paul@crapouillou.net> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)