| Message ID | 1455269165-1564-2-git-send-email-vincent.abriou@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Have you any comment for this proposal? BR Vincent On 02/12/2016 10:26 AM, Vincent Abriou wrote: > From: Fabien Dessenne <fabien.dessenne@st.com> > > If a buffer is interlaced, this "Bottom Field First" flag specifies > which of the top or the bottom field shall be displayed first. > When set, the bottom field shall be displayed first. > When unset the top field shall be displayed first. > > Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com> > --- > drivers/gpu/drm/drm_crtc.c | 3 ++- > include/uapi/drm/drm_mode.h | 1 + > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index d40bab2..64b4fdac 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -3315,7 +3315,8 @@ internal_framebuffer_create(struct drm_device *dev, > struct drm_framebuffer *fb; > int ret; > > - if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) { > + if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS > + | DRM_MODE_FB_BFF)) { > DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags); > return ERR_PTR(-EINVAL); > } > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 50adb46..f7c9111 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -354,6 +354,7 @@ struct drm_mode_fb_cmd { > > #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */ > #define DRM_MODE_FB_MODIFIERS (1<<1) /* enables ->modifer[] */ > +#define DRM_MODE_FB_BFF (1<<2) /* if interlaced, bottom field first */ > > struct drm_mode_fb_cmd2 { > __u32 fb_id; >
On Fri, Feb 26, 2016 at 11:33:08AM +0100, Vincent ABRIOU wrote: > Hi, > > Have you any comment for this proposal? I guess since we don't really have userspace that uses interlaced modes, much less actually bothers to get the fields correct I think just have some (open-source) userspace somewhere (does gstreamer care enough about this?) which needs this and it's good. Without userspace this is a hard sell. Thanks, Daniel > > BR > Vincent > > On 02/12/2016 10:26 AM, Vincent Abriou wrote: > > From: Fabien Dessenne <fabien.dessenne@st.com> > > > > If a buffer is interlaced, this "Bottom Field First" flag specifies > > which of the top or the bottom field shall be displayed first. > > When set, the bottom field shall be displayed first. > > When unset the top field shall be displayed first. > > > > Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com> > > --- > > drivers/gpu/drm/drm_crtc.c | 3 ++- > > include/uapi/drm/drm_mode.h | 1 + > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index d40bab2..64b4fdac 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -3315,7 +3315,8 @@ internal_framebuffer_create(struct drm_device *dev, > > struct drm_framebuffer *fb; > > int ret; > > > > - if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) { > > + if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS > > + | DRM_MODE_FB_BFF)) { > > DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags); > > return ERR_PTR(-EINVAL); > > } > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > > index 50adb46..f7c9111 100644 > > --- a/include/uapi/drm/drm_mode.h > > +++ b/include/uapi/drm/drm_mode.h > > @@ -354,6 +354,7 @@ struct drm_mode_fb_cmd { > > > > #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */ > > #define DRM_MODE_FB_MODIFIERS (1<<1) /* enables ->modifer[] */ > > +#define DRM_MODE_FB_BFF (1<<2) /* if interlaced, bottom field first */ > > > > struct drm_mode_fb_cmd2 { > > __u32 fb_id; > >
On 02/29/2016 04:32 PM, Daniel Vetter wrote: > On Fri, Feb 26, 2016 at 11:33:08AM +0100, Vincent ABRIOU wrote: >> Hi, >> >> Have you any comment for this proposal? > > I guess since we don't really have userspace that uses interlaced modes, > much less actually bothers to get the fields correct I think just have > some (open-source) userspace somewhere (does gstreamer care enough about > this?) which needs this and it's good. > Hi Daniel, At Gstreamer side, an interlaced GstBuffer could be tagged with flags: https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gstvideo.html#GstVideoFrameFlags GST_VIDEO_FRAME_FLAG_TFF is one of them and it is used to warn that the buffer is interlaced with Top Field First (TFF). At kernel side, we preferred to tag the interlaced buffer with Bottom Field First (BFF) because most of the interlaced buffer are TFF and that BFF are the exception. Further, we made tests with weston to validate the full path and it is working fine. BR Vincent > Without userspace this is a hard sell. > > Thanks, Daniel > >> >> BR >> Vincent >> >> On 02/12/2016 10:26 AM, Vincent Abriou wrote: >>> From: Fabien Dessenne <fabien.dessenne@st.com> >>> >>> If a buffer is interlaced, this "Bottom Field First" flag specifies >>> which of the top or the bottom field shall be displayed first. >>> When set, the bottom field shall be displayed first. >>> When unset the top field shall be displayed first. >>> >>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com> >>> --- >>> drivers/gpu/drm/drm_crtc.c | 3 ++- >>> include/uapi/drm/drm_mode.h | 1 + >>> 2 files changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >>> index d40bab2..64b4fdac 100644 >>> --- a/drivers/gpu/drm/drm_crtc.c >>> +++ b/drivers/gpu/drm/drm_crtc.c >>> @@ -3315,7 +3315,8 @@ internal_framebuffer_create(struct drm_device *dev, >>> struct drm_framebuffer *fb; >>> int ret; >>> >>> - if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) { >>> + if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS >>> + | DRM_MODE_FB_BFF)) { >>> DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags); >>> return ERR_PTR(-EINVAL); >>> } >>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >>> index 50adb46..f7c9111 100644 >>> --- a/include/uapi/drm/drm_mode.h >>> +++ b/include/uapi/drm/drm_mode.h >>> @@ -354,6 +354,7 @@ struct drm_mode_fb_cmd { >>> >>> #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */ >>> #define DRM_MODE_FB_MODIFIERS (1<<1) /* enables ->modifer[] */ >>> +#define DRM_MODE_FB_BFF (1<<2) /* if interlaced, bottom field first */ >>> >>> struct drm_mode_fb_cmd2 { >>> __u32 fb_id; >>> >
On Mon, Feb 29, 2016 at 05:16:13PM +0100, Vincent ABRIOU wrote: > > > On 02/29/2016 04:32 PM, Daniel Vetter wrote: > > On Fri, Feb 26, 2016 at 11:33:08AM +0100, Vincent ABRIOU wrote: > >> Hi, > >> > >> Have you any comment for this proposal? > > > > I guess since we don't really have userspace that uses interlaced modes, > > much less actually bothers to get the fields correct I think just have > > some (open-source) userspace somewhere (does gstreamer care enough about > > this?) which needs this and it's good. > > > > Hi Daniel, > > At Gstreamer side, an interlaced GstBuffer could be tagged with flags: > https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gstvideo.html#GstVideoFrameFlags > > GST_VIDEO_FRAME_FLAG_TFF is one of them and it is used to warn that the > buffer is interlaced with Top Field First (TFF). > > At kernel side, we preferred to tag the interlaced buffer with Bottom > Field First (BFF) because most of the interlaced buffer are TFF and that > BFF are the exception. > > Further, we made tests with weston to validate the full path and it is > working fine. Cool, so it's all there. Please add link to the patches/support for this (e.g. Weston patches need to be on the m-l, reviewed by weston maintainers to be considered "ready"). Then once the userspace is all ready we can pull in the kernel patch. After that userspace side can land. Yes this is a bit of work to orchestrated, but this process is the result of getting it wrong a few too many times in DRM-land. ABI is hard ;-) So please resend your patch once all that is done, with links to gstreamer and weston patches with their r-b tags from maintainers. Thanks, Daniel > > BR > Vincent > > > Without userspace this is a hard sell. > > > > Thanks, Daniel > > > >> > >> BR > >> Vincent > >> > >> On 02/12/2016 10:26 AM, Vincent Abriou wrote: > >>> From: Fabien Dessenne <fabien.dessenne@st.com> > >>> > >>> If a buffer is interlaced, this "Bottom Field First" flag specifies > >>> which of the top or the bottom field shall be displayed first. > >>> When set, the bottom field shall be displayed first. > >>> When unset the top field shall be displayed first. > >>> > >>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com> > >>> --- > >>> drivers/gpu/drm/drm_crtc.c | 3 ++- > >>> include/uapi/drm/drm_mode.h | 1 + > >>> 2 files changed, 3 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > >>> index d40bab2..64b4fdac 100644 > >>> --- a/drivers/gpu/drm/drm_crtc.c > >>> +++ b/drivers/gpu/drm/drm_crtc.c > >>> @@ -3315,7 +3315,8 @@ internal_framebuffer_create(struct drm_device *dev, > >>> struct drm_framebuffer *fb; > >>> int ret; > >>> > >>> - if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) { > >>> + if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS > >>> + | DRM_MODE_FB_BFF)) { > >>> DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags); > >>> return ERR_PTR(-EINVAL); > >>> } > >>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > >>> index 50adb46..f7c9111 100644 > >>> --- a/include/uapi/drm/drm_mode.h > >>> +++ b/include/uapi/drm/drm_mode.h > >>> @@ -354,6 +354,7 @@ struct drm_mode_fb_cmd { > >>> > >>> #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */ > >>> #define DRM_MODE_FB_MODIFIERS (1<<1) /* enables ->modifer[] */ > >>> +#define DRM_MODE_FB_BFF (1<<2) /* if interlaced, bottom field first */ > >>> > >>> struct drm_mode_fb_cmd2 { > >>> __u32 fb_id; > >>> > >
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d40bab2..64b4fdac 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3315,7 +3315,8 @@ internal_framebuffer_create(struct drm_device *dev, struct drm_framebuffer *fb; int ret; - if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) { + if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS + | DRM_MODE_FB_BFF)) { DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags); return ERR_PTR(-EINVAL); } diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 50adb46..f7c9111 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -354,6 +354,7 @@ struct drm_mode_fb_cmd { #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */ #define DRM_MODE_FB_MODIFIERS (1<<1) /* enables ->modifer[] */ +#define DRM_MODE_FB_BFF (1<<2) /* if interlaced, bottom field first */ struct drm_mode_fb_cmd2 { __u32 fb_id;