| Message ID | 20191117024028.2233-34-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: Convert DSI code to use drm_mipi_dsi and drm_panel | expand |
* 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
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
* 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
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
* 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
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
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
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
* 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
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,
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(-)