Hi Daniel, On Thu, 2016-06-09 at 14:26 +0200, Daniel Vetter wrote: > On Thu, Jun 09, 2016 at 10:54:45AM +0000, Alexey Brodkin wrote: > > > > Hi Daniel, > > > > On Wed, 2016-06-08 at 16:30 +0200, Daniel Vetter wrote: > > > > > > On Wed, Jun 08, 2016 at 04:14:38PM +0200, Maarten Lankhorst wrote: > > > > > > > > > > > > Op 08-06-16 om 14:18 schreef Daniel Vetter: > > > > > > > > > > > > > > > The drm core has a nice ready-made helper for exactly the simple case > > > > > where it should fire on the next vblank. > > > > > > > > > > Note that arming the vblank event in _begin is probably too early, and > > > > > might easily result in the vblank firing too early, before the new set > > > > > of planes are actually disabled. But that's kinda a minor issue > > > > > compared to just outright hanging userspace. > > > > > > > > > > v2: Be more robust and either arm, when the CRTC is on, or just send > > > > > the event out right away. > > > > > > > > > > Cc: Carlos Palminha <palminha@xxxxxxxxxxxx> > > > > > Cc: Alexey Brodkin <abrodkin@xxxxxxxxxxxx> > > > > > Cc: linux-snps-arc@xxxxxxxxxxxxxxxxxxx > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > > > Wouldn't it be better to do this in atomic_flush then? > > > I'm not going to fix up other people's drivers completely, just enough to > > > hopefully not break them. If arc also blocks vblank interrupts with the go > > > bit, then doing this in _begin is correct. Either way it needs hw-specific > > > knowledge to asses whether it's correct, since doing the vblank event > > > stuff in _flush is also racy without some prevention. > > Actually in ARC PGU driver that was one of many other copy-pastes from > > other drivers. I.e. for me this is another boilerplate and if that's the > > same for other drivers as well probably that's a good candidate for > > generalization into something like drm_helper_crtc_atomic_check(). > > I checked them all, you are special with your code here. And this can't be > generalized since you must send out vblank events in a race-free manner > against the actual hw update. This requires deep knowledge of the actual > hw, and it's not something the helpers can take care of you. It is very > much not boilerplate, but crucial for a correct implementation. And most > likely arcpgu is wrong, but since I don't have that hw knowledge I'm not > going to change it more than absolutely required. Well I meant as of today we don't support vblank interrupts and so arc_pgu_crtc_atomic_begin() barely makes any sense. Still in the future we do plan to add support of interrupts. -Alexey _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
