diff mbox

[1/2] drm: Add DRM_MODE_FB_BFF flag definition

Message ID 1455269165-1564-2-git-send-email-vincent.abriou@st.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vincent Abriou Feb. 12, 2016, 9:26 a.m. UTC 
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(-)

Comments

Vincent Abriou Feb. 26, 2016, 10:33 a.m. UTC | #1 
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;
>
Daniel Vetter Feb. 29, 2016, 3:32 p.m. UTC | #2 
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;
> >
Vincent Abriou Feb. 29, 2016, 4:16 p.m. UTC | #3 
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;
>>>
>
Daniel Vetter Feb. 29, 2016, 4:37 p.m. UTC | #4 
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 mbox

Patch

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;