diff mbox

[RFC,01/12] drm: Add a new mode flag: DRM_MODE_FLAG_PREFER_ONE_SHOT

Message ID 1431308311-4470-2-git-send-email-markz@nvidia.com
State Deferred
Headers show

Commit Message

Mark Zhang May 11, 2015, 1:38 a.m. UTC
Normally this flag is set by panel driver so that crtc can enable
the "one-shot" mode(not scan frames continuously).

Signed-off-by: Mark Zhang <markz@nvidia.com>
---
 include/uapi/drm/drm_mode.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Daniel Vetter May 11, 2015, 9:27 a.m. UTC | #1
On Mon, May 11, 2015 at 09:38:20AM +0800, Mark Zhang wrote:
> Normally this flag is set by panel driver so that crtc can enable
> the "one-shot" mode(not scan frames continuously).
> 
> Signed-off-by: Mark Zhang <markz@nvidia.com>
> ---
>  include/uapi/drm/drm_mode.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index dbeba949462a..5447a338e893 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -72,6 +72,7 @@
>  #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
>  #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
>  #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
> +#define DRM_MODE_FLAG_PREFER_ONE_SHOT		(1<<19)

tbh this doesn't sound like a mode flag, but something which should be
attached to the drm_panel. Especially since all the single-frame modes are
highly sink/link specific. Why was this added here instead of to the
drm_panel metadata?
-Daniel
Mark Zhang May 11, 2015, 4:34 p.m. UTC | #2
I just want to make things easier. If we adding this in panel's meta data, it will be harder to make crtc gets this, since normally encoder talks with panel and crtc talks with encoder. But yes, adding this in panel's metadata makes more sense so if there is a better way to do that, I'm happy to do the changes.

Mark
Daniel Vetter May 12, 2015, 6:35 a.m. UTC | #3
On Mon, May 11, 2015 at 04:34:57PM +0000, Mark Zhang wrote:
> I just want to make things easier. If we adding this in panel's meta
> data, it will be harder to make crtc gets this, since normally encoder
> talks with panel and crtc talks with encoder. But yes, adding this in
> panel's metadata makes more sense so if there is a better way to do
> that, I'm happy to do the changes.

Adding something to the userspace ABI (which you've done here) because the
kernel-internals are designed in an awkward way right now is definitely
the wrong thing to do. With atomic you can easily add a bool
prefer_oneshot to drm_crtc_state to encode this. But I fear that with the
plain crtc helpers this just doesn't work properly. You could add a
driver-private internal in drm_display_mode->private_flags, but that might
clash with drivers existing use of this field.

In any way, this is definitely not something to add to uapi headers. Hence
Nacked-by: me.

Thanks, Daniel
> 
> Mark
> ________________________________________
> From: Daniel Vetter <daniel.vetter@ffwll.ch> on behalf of Daniel Vetter <daniel@ffwll.ch>
> Sent: Monday, May 11, 2015 5:27 PM
> To: Mark Zhang
> Cc: thierry.reding@gmail.com; linux-tegra@vger.kernel.org; dri-devel@lists.freedesktop.org
> Subject: Re: [RFC PATCH 01/12] drm: Add a new mode flag: DRM_MODE_FLAG_PREFER_ONE_SHOT
> 
> On Mon, May 11, 2015 at 09:38:20AM +0800, Mark Zhang wrote:
> > Normally this flag is set by panel driver so that crtc can enable
> > the "one-shot" mode(not scan frames continuously).
> >
> > Signed-off-by: Mark Zhang <markz@nvidia.com>
> > ---
> >  include/uapi/drm/drm_mode.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index dbeba949462a..5447a338e893 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -72,6 +72,7 @@
> >  #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH      (6<<14)
> >  #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM     (7<<14)
> >  #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF  (8<<14)
> > +#define DRM_MODE_FLAG_PREFER_ONE_SHOT                (1<<19)
> 
> tbh this doesn't sound like a mode flag, but something which should be
> attached to the drm_panel. Especially since all the single-frame modes are
> highly sink/link specific. Why was this added here instead of to the
> drm_panel metadata?
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Thierry Reding May 12, 2015, 8:23 a.m. UTC | #4
On Tue, May 12, 2015 at 08:35:58AM +0200, Daniel Vetter wrote:
> On Mon, May 11, 2015 at 04:34:57PM +0000, Mark Zhang wrote:
> > I just want to make things easier. If we adding this in panel's meta
> > data, it will be harder to make crtc gets this, since normally encoder
> > talks with panel and crtc talks with encoder. But yes, adding this in
> > panel's metadata makes more sense so if there is a better way to do
> > that, I'm happy to do the changes.
> 
> Adding something to the userspace ABI (which you've done here) because the
> kernel-internals are designed in an awkward way right now is definitely
> the wrong thing to do. With atomic you can easily add a bool
> prefer_oneshot to drm_crtc_state to encode this. But I fear that with the
> plain crtc helpers this just doesn't work properly. You could add a
> driver-private internal in drm_display_mode->private_flags, but that might
> clash with drivers existing use of this field.
> 
> In any way, this is definitely not something to add to uapi headers. Hence
> Nacked-by: me.

Are there use-cases where one-shot mode is worse than continuous mode?
I'm thinking games that run at full FPS and such. If so, exposing this
to userspace is perhaps not a bad idea, albeit not via a mode flag. If
userspace had a way to set the preference, it could do so depending on
use-case.

Thierry

> > ________________________________________
> > From: Daniel Vetter <daniel.vetter@ffwll.ch> on behalf of Daniel Vetter <daniel@ffwll.ch>
> > Sent: Monday, May 11, 2015 5:27 PM
> > To: Mark Zhang
> > Cc: thierry.reding@gmail.com; linux-tegra@vger.kernel.org; dri-devel@lists.freedesktop.org
> > Subject: Re: [RFC PATCH 01/12] drm: Add a new mode flag: DRM_MODE_FLAG_PREFER_ONE_SHOT
> > 
> > On Mon, May 11, 2015 at 09:38:20AM +0800, Mark Zhang wrote:
> > > Normally this flag is set by panel driver so that crtc can enable
> > > the "one-shot" mode(not scan frames continuously).
> > >
> > > Signed-off-by: Mark Zhang <markz@nvidia.com>
> > > ---
> > >  include/uapi/drm/drm_mode.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index dbeba949462a..5447a338e893 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -72,6 +72,7 @@
> > >  #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH      (6<<14)
> > >  #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM     (7<<14)
> > >  #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF  (8<<14)
> > > +#define DRM_MODE_FLAG_PREFER_ONE_SHOT                (1<<19)
> > 
> > tbh this doesn't sound like a mode flag, but something which should be
> > attached to the drm_panel. Especially since all the single-frame modes are
> > highly sink/link specific. Why was this added here instead of to the
> > drm_panel metadata?
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter May 12, 2015, 11:31 a.m. UTC | #5
On Tue, May 12, 2015 at 10:23 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, May 12, 2015 at 08:35:58AM +0200, Daniel Vetter wrote:
>> On Mon, May 11, 2015 at 04:34:57PM +0000, Mark Zhang wrote:
>> > I just want to make things easier. If we adding this in panel's meta
>> > data, it will be harder to make crtc gets this, since normally encoder
>> > talks with panel and crtc talks with encoder. But yes, adding this in
>> > panel's metadata makes more sense so if there is a better way to do
>> > that, I'm happy to do the changes.
>>
>> Adding something to the userspace ABI (which you've done here) because the
>> kernel-internals are designed in an awkward way right now is definitely
>> the wrong thing to do. With atomic you can easily add a bool
>> prefer_oneshot to drm_crtc_state to encode this. But I fear that with the
>> plain crtc helpers this just doesn't work properly. You could add a
>> driver-private internal in drm_display_mode->private_flags, but that might
>> clash with drivers existing use of this field.
>>
>> In any way, this is definitely not something to add to uapi headers. Hence
>> Nacked-by: me.
>
> Are there use-cases where one-shot mode is worse than continuous mode?
> I'm thinking games that run at full FPS and such. If so, exposing this
> to userspace is perhaps not a bad idea, albeit not via a mode flag. If
> userspace had a way to set the preference, it could do so depending on
> use-case.

If you have a case with jitter when the pageflips run at not-quite
60fps (just an example) then I think the kernel should cope with that
by disabling oneshot mode on its own. At least that's what i915 does.
Different story once we'll get variable refresh-rate support, but I
think that's an entirely different beast.

Also we don't need this as a mode flag, an atomic prop seems better
suited. Especially since the atomic prop avoids the mode_changed logic
of the helpers (which we want, no need to flicker when updating such a
hint), wheres anything that changes the mode will force a full modeset
by default.

Also if you expose this to userspace, you need userspace to use it
before merging.

drm_display_mode->private_flags still looks like the most suitable place.
-Daniel
diff mbox

Patch

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index dbeba949462a..5447a338e893 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -72,6 +72,7 @@ 
 #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
 #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
 #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
+#define DRM_MODE_FLAG_PREFER_ONE_SHOT		(1<<19)
 
 
 /* DPMS flags */