Date:	Fri, 3 Oct 2014 13:39:21 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Andrzej Hajda <a.hajda@...sung.com>
Cc:	"open list:DRM DRIVERS" <dri-devel@...ts.freedesktop.org>,
	Kukjin Kim <kgene.kim@...sung.com>,
	"open list:INTEL DRM DRIVERS..." <intel-gfx@...ts.freedesktop.org>,
	Seung-Woo Kim <sw0312.kim@...sung.com>,
	open list <linux-kernel@...r.kernel.org>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	"moderated list:ARM/S5P EXYNOS AR..." 
	<linux-samsung-soc@...r.kernel.org>,
	Daniel Vetter <daniel.vetter@...el.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Russell King <rmk+kernel@....linux.org.uk>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS
 drm drivers

On Fri, Oct 3, 2014 at 11:42 AM, Andrzej Hajda <a.hajda@...sung.com> wrote:
> On 10/03/2014 10:31 AM, Daniel Vetter wrote:
>> On Fri, Oct 03, 2014 at 10:24:09AM +0200, Andrzej Hajda wrote:
>>> The main intent of this patchset is to allow use of suspend/resume drm driver
>>> callbacks in KMS drivers, as these callbacks seems to me the best place
>>> to implement suspend/resume functionality in drm driver.
>>> Implementing this functionality in master component driver PM ops is problematic
>>> as those callbacks can be called asynchronously regardless of state/existence of
>>> drm device, thus it would require additional synchronization mechanism.
>>>
>>> Callbacks re-enabling requires small changes in i915 and exynos driver.
>>> The patchset contains also fix of exynos resume callback.
>> Nack.
>>
>> Like completely and totally. The drm core has really no business doing
>> hardware stuff, which includes runtime pm, system suspend and all that
>> nonsense. It' an interface between userspace and drivers, with a big
>> library to back it all up. Everything else just repeats the old midlayer
>> mistake.
>
> Hmm, I have just tried to reuse the existing infrastructure, I did not see
> any sign "do not touch, this is a mistake". Now I see it, thanks :)

As a rule of thumb, if you see a !DRIVER_MODESET check anywhere, then
that's a clear sign that you're wandering off the light and into the
dangerous parts of what drm was like age dark ages ;-)

>> If you driver needs this, do it there. Also, the component framework is
>> probably the solution you're looking for. And if there are synchronization
>> issues with that then we need to fix those instead of reinventing yet
>> another half-assed broken wheel.
>
> But this is an issue closely connected with component framework.
> Component framework separates master component probe and drm device
> initialization. As a result PM ops which are synchronized with probe
> (via device_lock)
> are no more synchronized with drm initialization which is usually called
> from
> .bind callback.

Now I'm confused. component->bind and drm initialization is about
driver load, but all this code here is about system suspend/resume
really. So I'm rather confused what problem you actually want to fix
..

>> Aside: With David Herrmann's latest patches to de-midlayer the drm
>> init/teardown sequence the driver is in full control of when the drm data
>> structures get allocate, initialized and registered. If you convert to
>> that plus the component framework I'm pretty sure your problem is solved.
>
> I will look closer at it but as I described above it is rather matter of
> separation
> of master component and drm device initialization.
>
> My idea was to avoid creation of new synchronization mechanism and to
> reuse the
> existing ones which seems to fit perfectly to the scenario, but if there
> is big NO for it
> another solution should be found.
>
> Anyway I guess the problem exists for all drivers having component
> framework and suspend:
> exynos, msm and incoming rockchip.

It sounds like the component framework needs to be teached to work
with system suspend/resume. I guess either we need to have clever
abuse of deferred probing to make sure that the master device is
probed first and so in the correct place in the system/resume
sequence. Or the master framework needs to grow pm_ops itself so that
the state associated with the logical componentized framework can be
saved/restored.

So master pm_ops would save/restore kms state, and all the components
would just save/restore any additional (register, clock, whatever)
state that each componnent driver would need.

But I'm not really an expert here, so adding more people.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists