Re: [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Aug 27, 2015 at 04:00:05PM +0200, Maarten Lankhorst wrote:
> Op 27-08-15 om 15:50 schreef Ville Syrjälä:
> > On Thu, Aug 27, 2015 at 03:05:38PM +0200, Maarten Lankhorst wrote:
> >> Op 27-08-15 om 14:52 schreef Ville Syrjälä:
> >>> On Thu, Aug 27, 2015 at 02:50:34PM +0200, Maarten Lankhorst wrote:
> >>>> Op 27-08-15 om 14:48 schreef Ville Syrjälä:
> >>>>> On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
> >>>>>> Op 27-08-15 om 14:19 schreef Daniel Stone:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> On 4 August 2015 at 12:34, Maarten Lankhorst
> >>>>>>> <maarten.lankhorst@linux.intel.com> wrote:
> >>>>>>>> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
> >>>>>>>> "drm/atomic: Cleanup on error properly in the atomic ioctl."
> >>>>>>>> cleaned up some error paths, but didn't fix the TEST_ONLY path.
> >>>>>>>> In the check only case plane->fb shouldn't be updated, and
> >>>>>>>> the vblank events should be cleared as on failure.
> >>>>>>> Bikeshedding a bit ...
> >>>>>>>
> >>>>>>> An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
> >>>>>>> to mention this in the commit message; in this case, the main change
> >>>>>>> is about plane->{,old_}fb.
> >>>>>> Even testing with PAGE_FLIP_EVENT would be useful because
> >>>>>> event && !crtc_state->active should not be allowed. In that case test
> >>>>>> could succeed but commit could fail.
> >>>>> Why would commit fail when the we're in DPMS off? I would suggest it
> >>>>> should be allowed. The operation would just a be a nop from a HW point
> >>>>> of view, all the calculation/checks would still be performed.
> >>>>>
> >>>> You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.
> >>> What's so special about the event here? Just send it out as soon as the
> >>> state has been swapped.
> >> Previously this has been disallowed for legacy page flips.
> > I don't think so. Speaking for i915, I think we've just rejected legacy page
> > flips entirely with the pipe is off on account of drm_vblank_get() failing.
> No atomic driver handles this case correctly. You can't get vblank events with the crtc off.

I don't understand what you're saying. Should there be a comma after
"No" ? If not, then I'm not sure what they don't handle.

> >> I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.
> > We could stick the last vbl count/timestamp in there.
> >
> > Not allowing means userspace is forced to consider the dpms state
> > whenever it wants to call the atomic ioctl.
> Userspace was the one turning off the crtc in the first place; it shouldn't continue flipping but preserve power. :-)

Meh. Much simpler to write code when you don't have to worry about such
details.

In the kernel it should amount to
if (!pipe_active)
	send_event

We anyway need something like that for the crtc getting disabled case,
don't we?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx