[1/2] drm/mxsfb: Call drm_crtc_vblank_on/off
diff mbox series

Message ID 20200527094757.1414174-1-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • [1/2] drm/mxsfb: Call drm_crtc_vblank_on/off
Related show

Commit Message

Daniel Vetter May 27, 2020, 9:47 a.m. UTC
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(+)

Comments

Daniel Vetter May 28, 2020, 5:46 a.m. UTC | #1
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
>
Stefan Agner May 28, 2020, 7:56 a.m. UTC | #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
>>
Daniel Vetter May 28, 2020, 8:06 a.m. UTC | #3
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
> >>
Stefan Agner May 28, 2020, 8:19 a.m. UTC | #4
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
>> >>
Daniel Vetter May 28, 2020, 8:25 a.m. UTC | #5
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
> >> >>

Patch
diff mbox series

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