Re: [RFC v3] drm/exynos: g2d: fix runtime PM

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Tobias,

On 2016-09-26 16:15, Tobias Jakobi wrote:
Marek Szyprowski wrote:
On 2016-09-24 20:58, Tobias Jakobi wrote:
The commit b05984e21a7e000bf5074ace00d7a574944b2c16 broke
operation of the G2D. After this commit the following
happens.
- exynos_g2d_exec_ioctl() prepares a runqueue node and
    calls g2d_exec_runqueue()
- g2d_exec_runqueue() calls g2d_dma_start() which gets
    runtime PM sync
- runtime PM calls g2d_runtime_resume()
- g2d_runtime_resume() calls g2d_exec_runqueue()

Luckily for us this doesn't really loop, but creates a
mutex lockup, which the kernel even predicts.

Anyway, we fix this by reintroducing dedicated sleep
operations again, and only letting runtime PM control
the gate clock.

This is not enough to fix the issue though.
- We switch runtime PM to autosuspend. Currently clocks get
    disabled, and then re-enabled again in the runqueue worker
    when a node is completed and the next is started.
    With the upcoming introduction of IOMMU runtime PM this
    situations gets worse, since now also the IOMMU goes
    through a disable/enable cycle before the next node is
    started.
- We consolidate all runtime PM management to the runqueue
    worker.
- We introduce g2d_wait_finish() which waits until the currently
    processed runqueue node is finished.
    If this takes too long, the engine is forcibly reset. This
    is necessary to properly close the driver in case the engine
    should hang with read/write access to some area of memory.
    In this situation we can't properly unmap GEM/userptr
    objects, since this might create a pagefault.
- Sleep suspend issues a suspend of the runqueue and then calls
    g2d_wait_finish(), which returns the engine in the idle state.
    The current runqueue node gets completed, all other queued
    nodes stay in the queue. There is no hardware state that
    needs to be retained.
- Sleep resume just pokes the runqueue worker, which, should
    something be in queue, continues its work, or just exits.
IMHO there is too much done in one patch. It would be much easier to
understand it if the changes are split into 2 parts: restoring dedicated
speep callbacks and their integration with runqueue worker and addition
of autosuspend-based runtime pm.
so you mean first revert your patch, and then put more changes on top of
it in a separate patch? I'm not sure into which 'functional units' I
should break this one down.

I can of course create a separate patch for autosuspend, that that would
only replace a put_sync() with mark_last_busy() + put_autosuspend(),
wouldn't it?


My current idea of functional units:
1) revert Marek's patch
2) move runpm management to g2d_runqueue_worker()
3) introduce g2d_wait_finish() and use it sleep call
4) also use it in g2d_close() and g2d_remove()
5) put autosuspend on top

Would this make sense?

Yes, definitely this approach makes much more sense in my opinion. I'm sorry
for my broken initial patch and the additional work you had to do.

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux