[RFCv1,33/42] drm/omap: dsi: use atomic helper for dirtyfb
diff mbox series

Message ID 20191117024028.2233-34-sebastian.reichel@collabora.com
State New
Headers show
Series
  • drm/omap: Convert DSI code to use drm_mipi_dsi and drm_panel
Related show

Commit Message

Sebastian Reichel Nov. 17, 2019, 2:40 a.m. UTC
We can simply use the atomic helper for
handling the dirtyfb callback.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c |  6 +-----
 drivers/gpu/drm/omapdrm/omap_crtc.h |  1 -
 drivers/gpu/drm/omapdrm/omap_fb.c   | 21 ++-------------------
 3 files changed, 3 insertions(+), 25 deletions(-)

Comments

Tony Lindgren Nov. 18, 2019, 11:05 p.m. UTC | #1
* Sebastian Reichel <sebastian.reichel@collabora.com> [191117 07:11]:
> We can simply use the atomic helper for
> handling the dirtyfb callback.
...
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> -void omap_crtc_flush(struct drm_crtc *crtc)
> +static void omap_crtc_flush(struct drm_crtc *crtc)
>  {
>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> -	struct omap_crtc_state *omap_state = to_omap_crtc_state(crtc->state);
> -
> -	if (!omap_state->manually_updated)
> -		return;
>  
>  	if (!delayed_work_pending(&omap_crtc->update_work))
>  		schedule_delayed_work(&omap_crtc->update_work, 0);

It would be nice if omap_crtc_flush() would become just some generic
void function with no need to pass it a crtc. I guess for that it
should know what panels are in manual command mode to refresh them.

The reason I'm bringing this up is because it looks like we need
to also flush DSI command mode panels from omap_gem_op_finish()
for gles and the gem code probably should not need to know anything
about crtc, right?

Or maybe there is some better place to call it?

Regards,

Tony
Tomi Valkeinen Nov. 19, 2019, 5:42 a.m. UTC | #2
On 19/11/2019 01:05, Tony Lindgren wrote:
> * Sebastian Reichel <sebastian.reichel@collabora.com> [191117 07:11]:
>> We can simply use the atomic helper for
>> handling the dirtyfb callback.
> ...
>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> -void omap_crtc_flush(struct drm_crtc *crtc)
>> +static void omap_crtc_flush(struct drm_crtc *crtc)
>>   {
>>   	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> -	struct omap_crtc_state *omap_state = to_omap_crtc_state(crtc->state);
>> -
>> -	if (!omap_state->manually_updated)
>> -		return;
>>   
>>   	if (!delayed_work_pending(&omap_crtc->update_work))
>>   		schedule_delayed_work(&omap_crtc->update_work, 0);
> 
> It would be nice if omap_crtc_flush() would become just some generic
> void function with no need to pass it a crtc. I guess for that it
> should know what panels are in manual command mode to refresh them.
> 
> The reason I'm bringing this up is because it looks like we need
> to also flush DSI command mode panels from omap_gem_op_finish()
> for gles and the gem code probably should not need to know anything
> about crtc, right?

We haven't had omap_gem_op_finish() in the kernel for some years now...

Shouldn't a normal page flip, or if doing single-buffering, using the 
dirtyfb ioctl, do the job?

  Tomi
Tony Lindgren Nov. 19, 2019, 2:32 p.m. UTC | #3
* Tomi Valkeinen <tomi.valkeinen@ti.com> [191119 05:42]:
> On 19/11/2019 01:05, Tony Lindgren wrote:
> > * Sebastian Reichel <sebastian.reichel@collabora.com> [191117 07:11]:
> > > We can simply use the atomic helper for
> > > handling the dirtyfb callback.
> > ...
> > > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > > -void omap_crtc_flush(struct drm_crtc *crtc)
> > > +static void omap_crtc_flush(struct drm_crtc *crtc)
> > >   {
> > >   	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> > > -	struct omap_crtc_state *omap_state = to_omap_crtc_state(crtc->state);
> > > -
> > > -	if (!omap_state->manually_updated)
> > > -		return;
> > >   	if (!delayed_work_pending(&omap_crtc->update_work))
> > >   		schedule_delayed_work(&omap_crtc->update_work, 0);
> > 
> > It would be nice if omap_crtc_flush() would become just some generic
> > void function with no need to pass it a crtc. I guess for that it
> > should know what panels are in manual command mode to refresh them.

Proably still cannot be a void function, but seems like we need to
call something outside omap_crtc.c.

> > The reason I'm bringing this up is because it looks like we need
> > to also flush DSI command mode panels from omap_gem_op_finish()
> > for gles and the gem code probably should not need to know anything
> > about crtc, right?
> 
> We haven't had omap_gem_op_finish() in the kernel for some years now...
> 
> Shouldn't a normal page flip, or if doing single-buffering, using the
> dirtyfb ioctl, do the job?

It does not seem to happen with the old pvr-omap4 related patches
and whatever gles related tests at least. If you have some idea
where it should get called I can take a look at some point.

Regards,

Tony
Tomi Valkeinen Nov. 19, 2019, 2:53 p.m. UTC | #4
On 19/11/2019 16:32, Tony Lindgren wrote:

>> We haven't had omap_gem_op_finish() in the kernel for some years now...
>>
>> Shouldn't a normal page flip, or if doing single-buffering, using the
>> dirtyfb ioctl, do the job?
> 
> It does not seem to happen with the old pvr-omap4 related patches
> and whatever gles related tests at least. If you have some idea
> where it should get called I can take a look at some point.

The userspace apps need to do this. If they're using single-buffering, 
then using the dirtyfb ioctl should do the trick, after the SGX has 
finished drawing.

It's probably somewhat difficult if EGL is controlling the display, as, 
afaik, SGX EGL is closed source, and that's probably where it should be 
done.

But adding back the hacky omap gem sync stuff, and then somehow guessing 
from the sync finish that some display needs to be updated... It just 
does not sound right to me.

  Tomi
Tony Lindgren Nov. 19, 2019, 3:06 p.m. UTC | #5
* Tomi Valkeinen <tomi.valkeinen@ti.com> [191119 14:54]:
> On 19/11/2019 16:32, Tony Lindgren wrote:
> 
> > > We haven't had omap_gem_op_finish() in the kernel for some years now...
> > > 
> > > Shouldn't a normal page flip, or if doing single-buffering, using the
> > > dirtyfb ioctl, do the job?
> > 
> > It does not seem to happen with the old pvr-omap4 related patches
> > and whatever gles related tests at least. If you have some idea
> > where it should get called I can take a look at some point.
> 
> The userspace apps need to do this. If they're using single-buffering, then
> using the dirtyfb ioctl should do the trick, after the SGX has finished
> drawing.

Sounds like that's not happening.

But why would the userspace app need to know this might be needed for
a DSI command mode display and that it's not needed for HDMI?

> It's probably somewhat difficult if EGL is controlling the display, as,
> afaik, SGX EGL is closed source, and that's probably where it should be
> done.
> 
> But adding back the hacky omap gem sync stuff, and then somehow guessing
> from the sync finish that some display needs to be updated... It just does
> not sound right to me.

Right it's ugly. Still sounds like we need something in the kernel
that knows "this is a DSI command mode LCD and needs to be updated".

Regards,

Tony
Tomi Valkeinen Nov. 19, 2019, 3:55 p.m. UTC | #6
On 19/11/2019 17:06, Tony Lindgren wrote:

>> The userspace apps need to do this. If they're using single-buffering, then
>> using the dirtyfb ioctl should do the trick, after the SGX has finished
>> drawing.
> 
> Sounds like that's not happening.
> 
> But why would the userspace app need to know this might be needed for
> a DSI command mode display and that it's not needed for HDMI?

When double-buffering, the userspace doesn't need to care, as the 
page-flip ioctl explicitly tells that the buffer is ready.

When single buffering, either the userspace has to tell that the buffer 
is now ready, or the kernel has to guess based on something. But afaics, 
the latter is always a guess, and may not be a good guess.

The kernel could trigger a delayed update based on, say, page fault if 
drawing with CPU. But how much delayed... And with this scenario, we 
would somehow need to find a way to catch the writes from any IP to the 
buffer, and afaik there's no such thing.

>> It's probably somewhat difficult if EGL is controlling the display, as,
>> afaik, SGX EGL is closed source, and that's probably where it should be
>> done.
>>
>> But adding back the hacky omap gem sync stuff, and then somehow guessing
>> from the sync finish that some display needs to be updated... It just does
>> not sound right to me.
> 
> Right it's ugly. Still sounds like we need something in the kernel
> that knows "this is a DSI command mode LCD and needs to be updated".

I think one option is to refresh the command mode display all the time. 
Either using a timer, or trigger updates based on the previous update 
being finished.

Of course, that's kind of against the whole point of manual update 
display, but then it should effectively behave like a conventional 
always-updating display (except your HW is more expensive and consumes 
more power than a conventional display).

There's this Panel Self Refresh feature in DisplayPort (which I think is 
implemented in drm_self_refresh_helper.c), which has some similarities 
to this case. But if I read it right, that also expects some kind of 
trigger from userspace (any DRM commit) to start the refresh.

Afaik, Weston and X both handle page flips and/or dirtying the fb, so 
they should work. Are there applications that do not work, and cannot be 
made to work, except the few SGX test apps?

  Tomi
Andreas Kemnade Nov. 19, 2019, 6:46 p.m. UTC | #7
On Tue, 19 Nov 2019 17:55:57 +0200
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> On 19/11/2019 17:06, Tony Lindgren wrote:
> 
> >> The userspace apps need to do this. If they're using single-buffering, then
> >> using the dirtyfb ioctl should do the trick, after the SGX has finished
> >> drawing.  
> > 
> > Sounds like that's not happening.
> > 
> > But why would the userspace app need to know this might be needed for
> > a DSI command mode display and that it's not needed for HDMI?  
> 
> When double-buffering, the userspace doesn't need to care, as the 
> page-flip ioctl explicitly tells that the buffer is ready.
> 
> When single buffering, either the userspace has to tell that the buffer 
> is now ready, or the kernel has to guess based on something. But afaics, 
> the latter is always a guess, and may not be a good guess.
> 
> The kernel could trigger a delayed update based on, say, page fault if 
> drawing with CPU. But how much delayed... And with this scenario, we 
> would somehow need to find a way to catch the writes from any IP to the 
> buffer, and afaik there's no such thing.
> 
> >> It's probably somewhat difficult if EGL is controlling the display, as,
> >> afaik, SGX EGL is closed source, and that's probably where it should be
> >> done.
> >>
> >> But adding back the hacky omap gem sync stuff, and then somehow guessing
> >> from the sync finish that some display needs to be updated... It just does
> >> not sound right to me.  
> > 
> > Right it's ugly. Still sounds like we need something in the kernel
> > that knows "this is a DSI command mode LCD and needs to be updated".  
> 
well, if we look broader around and not only at the remaining displays
to be converted from omapdrm to generic and look at more displays, there
are also EPDs.

> I think one option is to refresh the command mode display all the time. 
> Either using a timer, or trigger updates based on the previous update 
> being finished.
> 
> Of course, that's kind of against the whole point of manual update 
> display, but then it should effectively behave like a conventional 
> always-updating display (except your HW is more expensive and consumes 
> more power than a conventional display).
> 
And again as an extreme example about power consumption: EPDs.
So I guess we need a generic interface for userspace-triggered
refreshes anyways. And in that case that are only partly refreshes.

Regards,
Andreas
Sebastian Reichel Nov. 19, 2019, 9:15 p.m. UTC | #8
Hi,

On Tue, Nov 19, 2019 at 07:46:28PM +0100, Andreas Kemnade wrote:
> On Tue, 19 Nov 2019 17:55:57 +0200
> Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> 
> > On 19/11/2019 17:06, Tony Lindgren wrote:
> > 
> > >> The userspace apps need to do this. If they're using single-buffering, then
> > >> using the dirtyfb ioctl should do the trick, after the SGX has finished
> > >> drawing.  
> > > 
> > > Sounds like that's not happening.
> > > 
> > > But why would the userspace app need to know this might be needed for
> > > a DSI command mode display and that it's not needed for HDMI?  
> > 
> > When double-buffering, the userspace doesn't need to care, as the 
> > page-flip ioctl explicitly tells that the buffer is ready.
> > 
> > When single buffering, either the userspace has to tell that the buffer 
> > is now ready, or the kernel has to guess based on something. But afaics, 
> > the latter is always a guess, and may not be a good guess.
> > 
> > The kernel could trigger a delayed update based on, say, page fault if 
> > drawing with CPU. But how much delayed... And with this scenario, we 
> > would somehow need to find a way to catch the writes from any IP to the 
> > buffer, and afaik there's no such thing.
> > 
> > >> It's probably somewhat difficult if EGL is controlling the display, as,
> > >> afaik, SGX EGL is closed source, and that's probably where it should be
> > >> done.
> > >>
> > >> But adding back the hacky omap gem sync stuff, and then somehow guessing
> > >> from the sync finish that some display needs to be updated... It just does
> > >> not sound right to me.  
> > > 
> > > Right it's ugly. Still sounds like we need something in the kernel
> > > that knows "this is a DSI command mode LCD and needs to be updated".  
> > 
> well, if we look broader around and not only at the remaining displays
> to be converted from omapdrm to generic and look at more displays, there
> are also EPDs.
> 
> > I think one option is to refresh the command mode display all the time. 
> > Either using a timer, or trigger updates based on the previous update 
> > being finished.
> > 
> > Of course, that's kind of against the whole point of manual update 
> > display, but then it should effectively behave like a conventional 
> > always-updating display (except your HW is more expensive and consumes 
> > more power than a conventional display).
> > 
> And again as an extreme example about power consumption: EPDs.
> So I guess we need a generic interface for userspace-triggered
> refreshes anyways. And in that case that are only partly refreshes.

You can do exactly this using the dirtyfb ioctl, which tells the
kernel, that some parts of the framebuffer are "dirty" and should
be send to the hardware. This is being used by the kernel console
and X.org. For omapdrm this triggers a full refresh of DSI command
mode panels.

The second method (which only supports full framebuffer for obvious
reasons) is double buffering. If you toggle from first to second
framebuffer, the driver will send the framebuffer to hardware. This
is being used by Weston when the DRM backend is selected.

-- Sebastian
Tony Lindgren Nov. 20, 2019, 3:48 a.m. UTC | #9
* Tomi Valkeinen <tomi.valkeinen@ti.com> [191119 15:56]:
> Afaik, Weston and X both handle page flips and/or dirtying the fb, so they
> should work. Are there applications that do not work, and cannot be made to
> work, except the few SGX test apps?

I'm not sure sure yet what all it affects, I'll do some more tests on it.

Regards,

Tony

Patch
diff mbox series

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 80ed1b15ba49..2129cba7f4e2 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -346,13 +346,9 @@  void omap_crtc_framedone_irq(struct drm_crtc *crtc, uint32_t irqstatus)
 	wake_up(&omap_crtc->pending_wait);
 }
 
-void omap_crtc_flush(struct drm_crtc *crtc)
+static void omap_crtc_flush(struct drm_crtc *crtc)
 {
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
-	struct omap_crtc_state *omap_state = to_omap_crtc_state(crtc->state);
-
-	if (!omap_state->manually_updated)
-		return;
 
 	if (!delayed_work_pending(&omap_crtc->update_work))
 		schedule_delayed_work(&omap_crtc->update_work, 0);
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.h b/drivers/gpu/drm/omapdrm/omap_crtc.h
index 2fd57751ae2b..fe88f78f9711 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.h
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.h
@@ -31,6 +31,5 @@  int omap_crtc_wait_pending(struct drm_crtc *crtc);
 void omap_crtc_error_irq(struct drm_crtc *crtc, u32 irqstatus);
 void omap_crtc_vblank_irq(struct drm_crtc *crtc);
 void omap_crtc_framedone_irq(struct drm_crtc *crtc, uint32_t irqstatus);
-void omap_crtc_flush(struct drm_crtc *crtc);
 
 #endif /* __OMAPDRM_CRTC_H__ */
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 9aeab81dfb90..b0e972945252 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -9,6 +9,7 @@ 
 #include <drm/drm_modeset_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_damage_helper.h>
 
 #include "omap_dmm_tiler.h"
 #include "omap_drv.h"
@@ -55,28 +56,10 @@  struct omap_framebuffer {
 	struct mutex lock;
 };
 
-static int omap_framebuffer_dirty(struct drm_framebuffer *fb,
-				  struct drm_file *file_priv,
-				  unsigned flags, unsigned color,
-				  struct drm_clip_rect *clips,
-				  unsigned num_clips)
-{
-	struct drm_crtc *crtc;
-
-	drm_modeset_lock_all(fb->dev);
-
-	drm_for_each_crtc(crtc, fb->dev)
-		omap_crtc_flush(crtc);
-
-	drm_modeset_unlock_all(fb->dev);
-
-	return 0;
-}
-
 static const struct drm_framebuffer_funcs omap_framebuffer_funcs = {
 	.create_handle = drm_gem_fb_create_handle,
-	.dirty = omap_framebuffer_dirty,
 	.destroy = drm_gem_fb_destroy,
+	.dirty = drm_atomic_helper_dirtyfb,
 };
 
 static u32 get_linear_addr(struct drm_framebuffer *fb,