| Message ID | 1431308311-4470-2-git-send-email-markz@nvidia.com |
|---|---|
| State | Deferred |
| Headers | show |
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
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
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
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
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 --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 */
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(+)