| Message ID | 20200527094757.1414174-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/mxsfb: Call drm_crtc_vblank_on/off | expand |
On Wed, May 27, 2020 at 11:47:56AM +0200, Daniel Vetter wrote: > mxsfb has vblank support, is atomic, but doesn't call > drm_crtc_vblank_on/off as it should. Not good. > > With my next patch to add the drm_crtc_vblank_reset to helpers this > means not even the very first crtc enabling will vblanks work anymore, > since they'll just stay off forever. > > Since mxsfb doesn't have any vblank waits of its own in the > enable/disable flow, nor an enable/disable_vblank callback we can do > the on/off as the first respectively last operation, and it should all > work. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Stefan Agner <stefan@agner.ch> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: NXP Linux Team <linux-imx@nxp.com> > Cc: linux-arm-kernel@lists.infradead.org Ping for some ack/review on this one here, it's holding up the subsystem wide fix in patch 2. Thanks, Daniel > --- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index 497cf443a9af..1891cd6deb2f 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -124,6 +124,7 @@ static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, > drm_panel_prepare(mxsfb->panel); > mxsfb_crtc_enable(mxsfb); > drm_panel_enable(mxsfb->panel); > + drm_crtc_vblank_on(&pipe->crtc); > } > > static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > @@ -133,6 +134,7 @@ static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > struct drm_crtc *crtc = &pipe->crtc; > struct drm_pending_vblank_event *event; > > + drm_crtc_vblank_off(&pipe->crtc); > drm_panel_disable(mxsfb->panel); > mxsfb_crtc_disable(mxsfb); > drm_panel_unprepare(mxsfb->panel); > -- > 2.26.2 >
Hi Daniel, On 2020-05-28 07:46, Daniel Vetter wrote: > On Wed, May 27, 2020 at 11:47:56AM +0200, Daniel Vetter wrote: >> mxsfb has vblank support, is atomic, but doesn't call >> drm_crtc_vblank_on/off as it should. Not good. >> >> With my next patch to add the drm_crtc_vblank_reset to helpers this >> means not even the very first crtc enabling will vblanks work anymore, >> since they'll just stay off forever. >> >> Since mxsfb doesn't have any vblank waits of its own in the >> enable/disable flow, nor an enable/disable_vblank callback we can do >> the on/off as the first respectively last operation, and it should all >> work. >> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Marek Vasut <marex@denx.de> >> Cc: Stefan Agner <stefan@agner.ch> >> Cc: Shawn Guo <shawnguo@kernel.org> >> Cc: Sascha Hauer <s.hauer@pengutronix.de> >> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> >> Cc: Fabio Estevam <festevam@gmail.com> >> Cc: NXP Linux Team <linux-imx@nxp.com> >> Cc: linux-arm-kernel@lists.infradead.org > > Ping for some ack/review on this one here, it's holding up the subsystem > wide fix in patch 2. Sorry for the delay. I guess that has the same effect as patch 14 in Laurent's patchset would have: https://lore.kernel.org/dri-devel/20200309195216.31042-15-laurent.pinchart@ideasonboard.com/ But should be rather trivial to rebase. So until Laurent's patchset is ready, we can go with this fix. Acked-by: Stefan Agner <stefan@agner.ch> -- Stefan > > Thanks, Daniel > >> --- >> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c >> index 497cf443a9af..1891cd6deb2f 100644 >> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c >> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c >> @@ -124,6 +124,7 @@ static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, >> drm_panel_prepare(mxsfb->panel); >> mxsfb_crtc_enable(mxsfb); >> drm_panel_enable(mxsfb->panel); >> + drm_crtc_vblank_on(&pipe->crtc); >> } >> >> static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) >> @@ -133,6 +134,7 @@ static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) >> struct drm_crtc *crtc = &pipe->crtc; >> struct drm_pending_vblank_event *event; >> >> + drm_crtc_vblank_off(&pipe->crtc); >> drm_panel_disable(mxsfb->panel); >> mxsfb_crtc_disable(mxsfb); >> drm_panel_unprepare(mxsfb->panel); >> -- >> 2.26.2 >>
On Thu, May 28, 2020 at 9:56 AM Stefan Agner <stefan@agner.ch> wrote: > > Hi Daniel, > > On 2020-05-28 07:46, Daniel Vetter wrote: > > On Wed, May 27, 2020 at 11:47:56AM +0200, Daniel Vetter wrote: > >> mxsfb has vblank support, is atomic, but doesn't call > >> drm_crtc_vblank_on/off as it should. Not good. > >> > >> With my next patch to add the drm_crtc_vblank_reset to helpers this > >> means not even the very first crtc enabling will vblanks work anymore, > >> since they'll just stay off forever. > >> > >> Since mxsfb doesn't have any vblank waits of its own in the > >> enable/disable flow, nor an enable/disable_vblank callback we can do > >> the on/off as the first respectively last operation, and it should all > >> work. > >> > >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >> Cc: Marek Vasut <marex@denx.de> > >> Cc: Stefan Agner <stefan@agner.ch> > >> Cc: Shawn Guo <shawnguo@kernel.org> > >> Cc: Sascha Hauer <s.hauer@pengutronix.de> > >> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > >> Cc: Fabio Estevam <festevam@gmail.com> > >> Cc: NXP Linux Team <linux-imx@nxp.com> > >> Cc: linux-arm-kernel@lists.infradead.org > > > > Ping for some ack/review on this one here, it's holding up the subsystem > > wide fix in patch 2. > > Sorry for the delay. > > I guess that has the same effect as patch 14 in Laurent's patchset would > have: > https://lore.kernel.org/dri-devel/20200309195216.31042-15-laurent.pinchart@ideasonboard.com/ Uh, looking at that patch I realized that mxsfb indeed calls drm_vblank_init before mode_config.num_crtc is set. Which means it never had working vblank support in upstream. That also explains the lack of fireworks, since all other drivers that actually do initialize vblank support have the drm_crtc_vblank_on/off calls - without them the driver doesn't survive for very long. tldr; I don't need this patch here to apply the 2nd one, so no conflict potential at all. And the patch from Laurent does fix up everything correctly, so we should be good. -Daniel > But should be rather trivial to rebase. So until Laurent's patchset is > ready, we can go with this fix. > > Acked-by: Stefan Agner <stefan@agner.ch> > > -- > Stefan > > > > > Thanks, Daniel > > > >> --- > >> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > >> index 497cf443a9af..1891cd6deb2f 100644 > >> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > >> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > >> @@ -124,6 +124,7 @@ static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, > >> drm_panel_prepare(mxsfb->panel); > >> mxsfb_crtc_enable(mxsfb); > >> drm_panel_enable(mxsfb->panel); > >> + drm_crtc_vblank_on(&pipe->crtc); > >> } > >> > >> static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > >> @@ -133,6 +134,7 @@ static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > >> struct drm_crtc *crtc = &pipe->crtc; > >> struct drm_pending_vblank_event *event; > >> > >> + drm_crtc_vblank_off(&pipe->crtc); > >> drm_panel_disable(mxsfb->panel); > >> mxsfb_crtc_disable(mxsfb); > >> drm_panel_unprepare(mxsfb->panel); > >> -- > >> 2.26.2 > >>
On 2020-05-28 10:06, Daniel Vetter wrote: > On Thu, May 28, 2020 at 9:56 AM Stefan Agner <stefan@agner.ch> wrote: >> >> Hi Daniel, >> >> On 2020-05-28 07:46, Daniel Vetter wrote: >> > On Wed, May 27, 2020 at 11:47:56AM +0200, Daniel Vetter wrote: >> >> mxsfb has vblank support, is atomic, but doesn't call >> >> drm_crtc_vblank_on/off as it should. Not good. >> >> >> >> With my next patch to add the drm_crtc_vblank_reset to helpers this >> >> means not even the very first crtc enabling will vblanks work anymore, >> >> since they'll just stay off forever. >> >> >> >> Since mxsfb doesn't have any vblank waits of its own in the >> >> enable/disable flow, nor an enable/disable_vblank callback we can do >> >> the on/off as the first respectively last operation, and it should all >> >> work. >> >> >> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> >> Cc: Marek Vasut <marex@denx.de> >> >> Cc: Stefan Agner <stefan@agner.ch> >> >> Cc: Shawn Guo <shawnguo@kernel.org> >> >> Cc: Sascha Hauer <s.hauer@pengutronix.de> >> >> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> >> >> Cc: Fabio Estevam <festevam@gmail.com> >> >> Cc: NXP Linux Team <linux-imx@nxp.com> >> >> Cc: linux-arm-kernel@lists.infradead.org >> > >> > Ping for some ack/review on this one here, it's holding up the subsystem >> > wide fix in patch 2. >> >> Sorry for the delay. >> >> I guess that has the same effect as patch 14 in Laurent's patchset would >> have: >> https://lore.kernel.org/dri-devel/20200309195216.31042-15-laurent.pinchart@ideasonboard.com/ > > Uh, looking at that patch I realized that mxsfb indeed calls > drm_vblank_init before mode_config.num_crtc is set. Which means it > never had working vblank support in upstream. That also explains the > lack of fireworks, since all other drivers that actually do initialize > vblank support have the drm_crtc_vblank_on/off calls - without them > the driver doesn't survive for very long. > > tldr; I don't need this patch here to apply the 2nd one, so no > conflict potential at all. And the patch from Laurent does fix up > everything correctly, so we should be good. Uh I see, that is somehow unfortunate and fortunate at the same time! Ok, I hope we get this cleaned up soon. -- Stefan > -Daniel > >> But should be rather trivial to rebase. So until Laurent's patchset is >> ready, we can go with this fix. >> >> Acked-by: Stefan Agner <stefan@agner.ch> >> >> -- >> Stefan >> >> > >> > Thanks, Daniel >> > >> >> --- >> >> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 ++ >> >> 1 file changed, 2 insertions(+) >> >> >> >> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c >> >> index 497cf443a9af..1891cd6deb2f 100644 >> >> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c >> >> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c >> >> @@ -124,6 +124,7 @@ static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, >> >> drm_panel_prepare(mxsfb->panel); >> >> mxsfb_crtc_enable(mxsfb); >> >> drm_panel_enable(mxsfb->panel); >> >> + drm_crtc_vblank_on(&pipe->crtc); >> >> } >> >> >> >> static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) >> >> @@ -133,6 +134,7 @@ static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) >> >> struct drm_crtc *crtc = &pipe->crtc; >> >> struct drm_pending_vblank_event *event; >> >> >> >> + drm_crtc_vblank_off(&pipe->crtc); >> >> drm_panel_disable(mxsfb->panel); >> >> mxsfb_crtc_disable(mxsfb); >> >> drm_panel_unprepare(mxsfb->panel); >> >> -- >> >> 2.26.2 >> >>
On Thu, May 28, 2020 at 10:19:46AM +0200, Stefan Agner wrote: > On 2020-05-28 10:06, Daniel Vetter wrote: > > On Thu, May 28, 2020 at 9:56 AM Stefan Agner <stefan@agner.ch> wrote: > >> > >> Hi Daniel, > >> > >> On 2020-05-28 07:46, Daniel Vetter wrote: > >> > On Wed, May 27, 2020 at 11:47:56AM +0200, Daniel Vetter wrote: > >> >> mxsfb has vblank support, is atomic, but doesn't call > >> >> drm_crtc_vblank_on/off as it should. Not good. > >> >> > >> >> With my next patch to add the drm_crtc_vblank_reset to helpers this > >> >> means not even the very first crtc enabling will vblanks work anymore, > >> >> since they'll just stay off forever. > >> >> > >> >> Since mxsfb doesn't have any vblank waits of its own in the > >> >> enable/disable flow, nor an enable/disable_vblank callback we can do > >> >> the on/off as the first respectively last operation, and it should all > >> >> work. > >> >> > >> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >> >> Cc: Marek Vasut <marex@denx.de> > >> >> Cc: Stefan Agner <stefan@agner.ch> > >> >> Cc: Shawn Guo <shawnguo@kernel.org> > >> >> Cc: Sascha Hauer <s.hauer@pengutronix.de> > >> >> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > >> >> Cc: Fabio Estevam <festevam@gmail.com> > >> >> Cc: NXP Linux Team <linux-imx@nxp.com> > >> >> Cc: linux-arm-kernel@lists.infradead.org > >> > > >> > Ping for some ack/review on this one here, it's holding up the subsystem > >> > wide fix in patch 2. > >> > >> Sorry for the delay. > >> > >> I guess that has the same effect as patch 14 in Laurent's patchset would > >> have: > >> https://lore.kernel.org/dri-devel/20200309195216.31042-15-laurent.pinchart@ideasonboard.com/ > > > > Uh, looking at that patch I realized that mxsfb indeed calls > > drm_vblank_init before mode_config.num_crtc is set. Which means it > > never had working vblank support in upstream. That also explains the > > lack of fireworks, since all other drivers that actually do initialize > > vblank support have the drm_crtc_vblank_on/off calls - without them > > the driver doesn't survive for very long. > > > > tldr; I don't need this patch here to apply the 2nd one, so no > > conflict potential at all. And the patch from Laurent does fix up > > everything correctly, so we should be good. > > Uh I see, that is somehow unfortunate and fortunate at the same time! > > Ok, I hope we get this cleaned up soon. I recommend igt tests for actually making sure your driver does something, instead of just thinking you've enabled a feature :-) -Daniel > > -- > Stefan > > > -Daniel > > > >> But should be rather trivial to rebase. So until Laurent's patchset is > >> ready, we can go with this fix. > >> > >> Acked-by: Stefan Agner <stefan@agner.ch> > >> > >> -- > >> Stefan > >> > >> > > >> > Thanks, Daniel > >> > > >> >> --- > >> >> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 ++ > >> >> 1 file changed, 2 insertions(+) > >> >> > >> >> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > >> >> index 497cf443a9af..1891cd6deb2f 100644 > >> >> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > >> >> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > >> >> @@ -124,6 +124,7 @@ static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, > >> >> drm_panel_prepare(mxsfb->panel); > >> >> mxsfb_crtc_enable(mxsfb); > >> >> drm_panel_enable(mxsfb->panel); > >> >> + drm_crtc_vblank_on(&pipe->crtc); > >> >> } > >> >> > >> >> static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > >> >> @@ -133,6 +134,7 @@ static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > >> >> struct drm_crtc *crtc = &pipe->crtc; > >> >> struct drm_pending_vblank_event *event; > >> >> > >> >> + drm_crtc_vblank_off(&pipe->crtc); > >> >> drm_panel_disable(mxsfb->panel); > >> >> mxsfb_crtc_disable(mxsfb); > >> >> drm_panel_unprepare(mxsfb->panel); > >> >> -- > >> >> 2.26.2 > >> >>
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index 497cf443a9af..1891cd6deb2f 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c @@ -124,6 +124,7 @@ static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, drm_panel_prepare(mxsfb->panel); mxsfb_crtc_enable(mxsfb); drm_panel_enable(mxsfb->panel); + drm_crtc_vblank_on(&pipe->crtc); } static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) @@ -133,6 +134,7 @@ static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) struct drm_crtc *crtc = &pipe->crtc; struct drm_pending_vblank_event *event; + drm_crtc_vblank_off(&pipe->crtc); drm_panel_disable(mxsfb->panel); mxsfb_crtc_disable(mxsfb); drm_panel_unprepare(mxsfb->panel);
mxsfb has vblank support, is atomic, but doesn't call drm_crtc_vblank_on/off as it should. Not good. With my next patch to add the drm_crtc_vblank_reset to helpers this means not even the very first crtc enabling will vblanks work anymore, since they'll just stay off forever. Since mxsfb doesn't have any vblank waits of its own in the enable/disable flow, nor an enable/disable_vblank callback we can do the on/off as the first respectively last operation, and it should all work. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Marek Vasut <marex@denx.de> Cc: Stefan Agner <stefan@agner.ch> Cc: Shawn Guo <shawnguo@kernel.org> Cc: Sascha Hauer <s.hauer@pengutronix.de> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> Cc: Fabio Estevam <festevam@gmail.com> Cc: NXP Linux Team <linux-imx@nxp.com> Cc: linux-arm-kernel@lists.infradead.org --- drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 ++ 1 file changed, 2 insertions(+)