Date:	Tue, 12 Jul 2016 18:10:26 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Tomasz Figa <tfiga@...omium.org>
Cc:	Sean Paul <seanpaul@...omium.org>, Mark Yao <yzq@...k-chips.com>,
	Inki Dae <inki.dae@...sung.com>,
	Jingoo Han <jingoohan1@...il.com>,
	Heiko Stuebner <heiko@...ech.de>,
	Stéphane Marchesin <marcheu@...omium.org>,
	Doug Anderson <dianders@...omium.org>,
	Thierry Reding <treding@...dia.com>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Javier Martinez Canillas <javier@....samsung.com>,
	David Airlie <airlied@...ux.ie>,
	Emil Velikov <emil.l.velikov@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	dri-devel <dri-devel@...ts.freedesktop.org>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
	"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>
Subject: Re: [PATCH v3 2/4] drm/rockchip: add an common abstracted PSR driver

On Wed, Jul 13, 2016 at 12:10:13AM +0900, Tomasz Figa wrote:
> On Tue, Jul 12, 2016 at 9:38 PM, Daniel Vetter <daniel@...ll.ch> wrote:
> > On Fri, Jul 01, 2016 at 02:00:00PM -0400, Sean Paul wrote:
> >> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@...k-chips.com> wrote:
> >> > The PSR driver have exported four symbols for specific device driver:
> >> > - rockchip_drm_psr_register()
> >> > - rockchip_drm_psr_unregister()
> >> > - rockchip_drm_psr_enable()
> >> > - rockchip_drm_psr_disable()
> >> > - rockchip_drm_psr_flush()
> >> >
> >> > Encoder driver should call the register/unregister interfaces to hook
> >> > itself into common PSR driver, encoder have implement the 'psr_set'
> >> > callback which use the set PSR state in hardware side.
> >> >
> >> > Crtc driver would call the enable/disable interfaces when vblank is
> >> > enable/disable, after that the common PSR driver would call the encoder
> >> > registered callback to set the PSR state.
> >> >
> >>
> >> This feels overly complicated. It seems like you could cut out a bunch
> >> of code by just coding the psr functions into vop and
> >> analogix_dp-rockchip. I suppose the only reason to keep it abstracted
> >> would be if you plan on supporting psr in a different encoder or crtc
> >> in rockchip, or if you're planning on moving this into drm core.
> >
> > Agreed on the layers of indirection. Also, you end up with 3 delayed
> > timers in total:
> > - defio timer from fbdev emulation
> > - timer in this abstraction
> > - delayed work in the psr backend driver
> 
> Maybe I'm missing something obvious (don't know how the PSR is
> implemented in hardware or in other drivers), but why do we need all
> these timers? Couldn't we just trigger a fake page flip on fb dirty
> call?

Page flips are ratelimited to vrefresh, and not all drivers support it.
The other issue is that dirty ioctl supports dirty rectangles, but page
flip is still a full-screen upload. We discussed adding a dirty rectangle
to the kms properties to fix this gap, but until that's done I don't think
it makes much sense to remap dirty to an (atomic) page flip. One small
technicality is that dirty works on framebuffers, but page flips
(especially atomic ones) work on crtcs. That makes the remapping a notch
more tricky.

But in principle, fully agreed. And once we have atomic dirty rectangles
we could type up a small helper that drivers can plug into the dirty
ioctl. Since new properties requires new userspace we could just track the
dirty rectangle internally to get this helper landed, if there's
interested for that. I'll certainly be happy to review patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists