All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Brian Starkey <brian.starkey@arm.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>
Subject: Re: [RFC PATCH 00/11] Introduce writeback connectors
Date: Wed, 12 Oct 2016 08:56:18 +0200	[thread overview]
Message-ID: <20161012065618.GI20761@phenom.ffwll.local> (raw)
In-Reply-To: <20161011212423.GA10077@e106950-lin.cambridge.arm.com>

On Tue, Oct 11, 2016 at 10:24:23PM +0100, Brian Starkey wrote:
> On Tue, Oct 11, 2016 at 10:02:43PM +0200, Daniel Vetter wrote:
> > The problem with just that is that there's lots of different things
> > that can feed into the overall needs_modeset variable. That's why we
> > split it up into multiple booleans.
> > 
> > So yes you're supposed to clear connectors_changed if there is some
> > change that you can handle without a full modeset. If you want, think
> > of connectors_changed as
> > needs_modeset_due_to_change_in_connnector_state, but that's cumbersome
> > to type and too long ;-)
> > 
> 
> All right, got it :-). This intention wasn't clear to me from the
> comments in the code.

A patch to update the kernel-doc to make it clearer (there's mode_changed,
connectors_changed and active_changed, plus drm_crtc_needs_modeset) would
be awesome. I'm trying to write useful docs, but since I designed this all
I sometimes forget to make the non-obvious assumptions clear enough.

Volunteered?

> > > > tbh I don't like that, I think it'd be better to make this truly
> > > > one-shot. Otherwise we'll have real fun problems with hw where the
> > > > writeback can take longer than a vblank (it happens ...). So one-shot,
> > > > with auto-clearing to NULL/0 is imo the right approach.
> > > 
> > > That's an interesting point about hardware which won't finish within
> > > one frame; but I don't see how "true one-shot" helps.
> > > 
> > > What's the expected behaviour if userspace makes a new atomic commit
> > > with a writeback framebuffer whilst a previous writeback is ongoing?
> > > 
> > > In both cases, you either need to block or fail the commit - whether
> > > the framebuffer gets removed when it's done is immaterial.
> > 
> > See Eric's question. We need to define that, and I think the simplest
> > approach is a completion fence/sync_file. It's destaged now in 4.9, we
> > can use them. I think the simplest uabi would be a pointer property
> > (u64) where we write the fd of the fence we'll signal when write-out
> > completes.
> > 
> 
> That tells userspace that the previous writeback is finished, I agree that's
> needed. It doesn't define any behaviour in case userspace asks for another
> writeback before that fence fires though.

Hm, good point. I guess we could just state that if userspace does a
writeback, and issues a new writeback before both a) the atomic flip and
b) the write back complete fence signalled will lead to undefined
behaviour. Undefined as in: data corruption, rejected atomic commit or
anything else than a kernel crash is allowed. This is similar to doing a
page flip and starting to render to the old buffers before the flip event
signalled completion: Userspace gets the mess it asked for ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch>
To: Brian Starkey <brian.starkey@arm.com>
Cc: Daniel Vetter <daniel@ffwll.ch>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	Liviu Dudau <liviu.dudau@arm.com>,
	"Clark, Rob" <robdclark@gmail.com>,
	Hans Verkuil <hverkuil@xs4all.nl>, Eric Anholt <eric@anholt.net>,
	"Syrjala, Ville" <ville.syrjala@linux.intel.com>
Subject: Re: [RFC PATCH 00/11] Introduce writeback connectors
Date: Wed, 12 Oct 2016 08:56:18 +0200	[thread overview]
Message-ID: <20161012065618.GI20761@phenom.ffwll.local> (raw)
In-Reply-To: <20161011212423.GA10077@e106950-lin.cambridge.arm.com>

On Tue, Oct 11, 2016 at 10:24:23PM +0100, Brian Starkey wrote:
> On Tue, Oct 11, 2016 at 10:02:43PM +0200, Daniel Vetter wrote:
> > The problem with just that is that there's lots of different things
> > that can feed into the overall needs_modeset variable. That's why we
> > split it up into multiple booleans.
> > 
> > So yes you're supposed to clear connectors_changed if there is some
> > change that you can handle without a full modeset. If you want, think
> > of connectors_changed as
> > needs_modeset_due_to_change_in_connnector_state, but that's cumbersome
> > to type and too long ;-)
> > 
> 
> All right, got it :-). This intention wasn't clear to me from the
> comments in the code.

A patch to update the kernel-doc to make it clearer (there's mode_changed,
connectors_changed and active_changed, plus drm_crtc_needs_modeset) would
be awesome. I'm trying to write useful docs, but since I designed this all
I sometimes forget to make the non-obvious assumptions clear enough.

Volunteered?

> > > > tbh I don't like that, I think it'd be better to make this truly
> > > > one-shot. Otherwise we'll have real fun problems with hw where the
> > > > writeback can take longer than a vblank (it happens ...). So one-shot,
> > > > with auto-clearing to NULL/0 is imo the right approach.
> > > 
> > > That's an interesting point about hardware which won't finish within
> > > one frame; but I don't see how "true one-shot" helps.
> > > 
> > > What's the expected behaviour if userspace makes a new atomic commit
> > > with a writeback framebuffer whilst a previous writeback is ongoing?
> > > 
> > > In both cases, you either need to block or fail the commit - whether
> > > the framebuffer gets removed when it's done is immaterial.
> > 
> > See Eric's question. We need to define that, and I think the simplest
> > approach is a completion fence/sync_file. It's destaged now in 4.9, we
> > can use them. I think the simplest uabi would be a pointer property
> > (u64) where we write the fd of the fence we'll signal when write-out
> > completes.
> > 
> 
> That tells userspace that the previous writeback is finished, I agree that's
> needed. It doesn't define any behaviour in case userspace asks for another
> writeback before that fence fires though.

Hm, good point. I guess we could just state that if userspace does a
writeback, and issues a new writeback before both a) the atomic flip and
b) the write back complete fence signalled will lead to undefined
behaviour. Undefined as in: data corruption, rejected atomic commit or
anything else than a kernel crash is allowed. This is similar to doing a
page flip and starting to render to the old buffers before the flip event
signalled completion: Userspace gets the mess it asked for ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

  reply	other threads:[~2016-10-12  6:56 UTC|newest]

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-11 14:53 [RFC PATCH 00/11] Introduce writeback connectors Brian Starkey
2016-10-11 14:53 ` Brian Starkey
2016-10-11 14:53 ` [RFC PATCH 01/11] drm: Add writeback connector type Brian Starkey
2016-10-11 14:53   ` Brian Starkey
2016-10-11 14:53 ` [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors Brian Starkey
2016-10-11 14:53   ` Brian Starkey
2016-10-11 15:44   ` Daniel Vetter
2016-10-11 15:44     ` Daniel Vetter
2016-10-11 16:47     ` Brian Starkey
2016-10-11 16:47       ` Brian Starkey
2016-10-11 16:56       ` Daniel Vetter
2016-10-11 16:56         ` Daniel Vetter
2016-10-11 14:54 ` [RFC PATCH 03/11] drm: Extract CRTC/plane disable from drm_framebuffer_remove Brian Starkey
2016-10-11 14:54   ` Brian Starkey
2016-10-11 14:54 ` [RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic Brian Starkey
2016-10-11 14:54   ` Brian Starkey
2016-10-11 15:51   ` Daniel Vetter
2016-10-11 15:51     ` Daniel Vetter
2016-10-11 14:54 ` [RFC PATCH 05/11] drm: Add fb to connector state Brian Starkey
2016-10-11 14:54   ` Brian Starkey
2016-10-11 14:54 ` [RFC PATCH 06/11] drm: Expose fb_id property for writeback connectors Brian Starkey
2016-10-11 14:54   ` Brian Starkey
2016-10-11 14:54 ` [RFC PATCH 07/11] drm: Add writeback-connector pixel format properties Brian Starkey
2016-10-11 14:54   ` Brian Starkey
2016-10-11 15:48   ` Daniel Vetter
2016-10-11 15:48     ` Daniel Vetter
2016-10-11 14:54 ` [RFC PATCH 08/11] drm: mali-dp: Rename malidp_input_format Brian Starkey
2016-10-11 14:54   ` Brian Starkey
2016-10-11 14:54 ` [RFC PATCH 09/11] drm: mali-dp: Add RGB writeback formats for DP550/DP650 Brian Starkey
2016-10-11 14:54   ` Brian Starkey
2016-10-11 14:54 ` [RFC PATCH 10/11] drm: mali-dp: Add support for writeback on DP550/DP650 Brian Starkey
2016-10-11 14:54   ` Brian Starkey
2016-10-11 14:54 ` [RFC PATCH 11/11] drm: mali-dp: Add writeback connector Brian Starkey
2016-10-11 14:54   ` Brian Starkey
2016-10-11 15:43 ` [RFC PATCH 00/11] Introduce writeback connectors Daniel Vetter
2016-10-11 15:43   ` Daniel Vetter
2016-10-11 16:43   ` Brian Starkey
2016-10-11 16:43     ` Brian Starkey
2016-10-11 17:01     ` Daniel Vetter
2016-10-11 17:01       ` Daniel Vetter
2016-10-11 19:44       ` Brian Starkey
2016-10-11 19:44         ` Brian Starkey
2016-10-11 20:02         ` Daniel Vetter
2016-10-11 20:02           ` Daniel Vetter
2016-10-11 21:24           ` Brian Starkey
2016-10-11 21:24             ` Brian Starkey
2016-10-12  6:56             ` Daniel Vetter [this message]
2016-10-12  6:56               ` Daniel Vetter
2016-10-13  9:47               ` [PATCH] drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset Brian Starkey
2016-10-13 14:54                 ` Alex Deucher
2016-10-13 14:54                   ` Alex Deucher
2016-10-17  6:07                   ` Daniel Vetter
2016-10-17  6:07                     ` Daniel Vetter
2016-10-11 16:25 ` [RFC PATCH 00/11] Introduce writeback connectors Ville Syrjälä
2016-10-11 16:25   ` Ville Syrjälä
2016-10-11 16:29   ` Daniel Vetter
2016-10-11 16:29     ` Daniel Vetter
2016-10-11 19:01 ` Eric Anholt
2016-10-11 19:01   ` Eric Anholt
2016-10-12  7:30   ` Brian Starkey
2016-10-12  7:30     ` Brian Starkey
2016-10-13 17:32     ` Eric Anholt
2016-10-14 10:50 ` Archit Taneja
2016-10-14 10:50   ` Archit Taneja
2016-10-14 12:39   ` Brian Starkey
2016-10-14 12:39     ` Brian Starkey
2016-10-14 12:50     ` Ville Syrjälä
2016-10-14 12:50       ` Ville Syrjälä
2016-10-14 14:56     ` Daniel Vetter
2016-10-14 14:56       ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161012065618.GI20761@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=brian.starkey@arm.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hverkuil@xs4all.nl \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=liviu.dudau@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.