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

From: Daniel Vetter <daniel@ffwll.ch>
To: Mark Zhang <markz@nvidia.com>
Cc: "linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [RFC PATCH 01/12] drm: Add a new mode flag: DRM_MODE_FLAG_PREFER_ONE_SHOT
Date: Tue, 12 May 2015 08:35:58 +0200	[thread overview]
Message-ID: <20150512063558.GP15256@phenom.ffwll.local> (raw)
In-Reply-To: <1431362096439.71223@nvidia.com>

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel