Re: [PATCH 1/2] drm/mxsfb: Call drm_crtc_vblank_on/off

[Daniel Vetter <daniel@ffwll.ch>]

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
> >>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel