diff mbox

[xf86-video-intel] sna: Add XV_COLORSPACE attribute support for sprite Xv adaptors

Message ID 20180214192108.2744-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä Feb. 14, 2018, 7:21 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use the new "COLOR_ENCODING" plane property to implement the
XV_COLORSPACE port attribute for sprite Xv adaptors.

Cc: Jyri Sarha <jsarha@ti.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 src/sna/sna.h              |   1 +
 src/sna/sna_display.c      | 148 +++++++++++++++++++++++++++++++++++----------
 src/sna/sna_video.h        |   3 +
 src/sna/sna_video_sprite.c |  22 ++++++-
 4 files changed, 142 insertions(+), 32 deletions(-)

Comments

Chris Wilson Feb. 14, 2018, 7:42 p.m. UTC | #1 
Quoting Ville Syrjala (2018-02-14 19:21:08)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Use the new "COLOR_ENCODING" plane property to implement the
> XV_COLORSPACE port attribute for sprite Xv adaptors.
> 
> Cc: Jyri Sarha <jsarha@ti.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> +void sna_crtc_set_sprite_colorspace(xf86CrtcPtr crtc,
> +                                   unsigned idx, int colorspace)
> +{
> +       struct plane *p;
> +
> +       assert(to_sna_crtc(crtc));

assert(colorspace < ARRAY_SIZE(p->color_encoding.values));

> +
> +       p = lookup_sprite(to_sna_crtc(crtc), idx);
> +
> +       if (!p->color_encoding.prop)
> +               return;
> +
> +       drmModeObjectSetProperty(to_sna(crtc->scrn)->kgem.fd,
> +                                p->id, DRM_MODE_OBJECT_PLANE,
> +                                p->color_encoding.prop,
> +                                p->color_encoding.values[colorspace]);

Does changing the property trigger an immediate update, or is that
deferred until the next SetPlane (or atomic update)?

It makes no difference to this patch, except we might redisplay the
image immediately rather than wait for the client?
-Chris
Ville Syrjälä Feb. 14, 2018, 8:06 p.m. UTC | #2 
On Wed, Feb 14, 2018 at 07:42:59PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-02-14 19:21:08)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Use the new "COLOR_ENCODING" plane property to implement the
> > XV_COLORSPACE port attribute for sprite Xv adaptors.
> > 
> > Cc: Jyri Sarha <jsarha@ti.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > +void sna_crtc_set_sprite_colorspace(xf86CrtcPtr crtc,
> > +                                   unsigned idx, int colorspace)
> > +{
> > +       struct plane *p;
> > +
> > +       assert(to_sna_crtc(crtc));
> 
> assert(colorspace < ARRAY_SIZE(p->color_encoding.values));
> 
> > +
> > +       p = lookup_sprite(to_sna_crtc(crtc), idx);
> > +
> > +       if (!p->color_encoding.prop)
> > +               return;
> > +
> > +       drmModeObjectSetProperty(to_sna(crtc->scrn)->kgem.fd,
> > +                                p->id, DRM_MODE_OBJECT_PLANE,
> > +                                p->color_encoding.prop,
> > +                                p->color_encoding.values[colorspace]);
> 
> Does changing the property trigger an immediate update, or is that
> deferred until the next SetPlane (or atomic update)?

Legacy setprop so it'll trigger an update. So if the plane is
already enabled I suppose we'll get an extra frame of lag here.
Same deal as for the colorkey ioctl actually.

I think generally one can assume the the attribute won't be changed
very often with the plane already enabled. Generally I'd expect the
client to set this up before the first putimage.

Converting this to atomic would avoid that extra lag. But we'd need
to convert the colorkey ioctl to properties as well. And the next step
after that would tearfree+atomic for making it actually nice :)

> 
> It makes no difference to this patch, except we might redisplay the
> image immediately rather than wait for the client?

We call this from the putimage hook, so we need the client to
actually repaint before anything happens. And since we don't have
reputimage manipulating the window/clipping won't update the
sprite unless the client repaints (which eg. mplayer won't do
when paused unless it gets expose events).
Chris Wilson Feb. 14, 2018, 8:16 p.m. UTC | #3 
Quoting Ville Syrjälä (2018-02-14 20:06:17)
> On Wed, Feb 14, 2018 at 07:42:59PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjala (2018-02-14 19:21:08)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Use the new "COLOR_ENCODING" plane property to implement the
> > > XV_COLORSPACE port attribute for sprite Xv adaptors.
> > > 
> > > Cc: Jyri Sarha <jsarha@ti.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > > +void sna_crtc_set_sprite_colorspace(xf86CrtcPtr crtc,
> > > +                                   unsigned idx, int colorspace)
> > > +{
> > > +       struct plane *p;
> > > +
> > > +       assert(to_sna_crtc(crtc));
> > 
> > assert(colorspace < ARRAY_SIZE(p->color_encoding.values));
> > 
> > > +
> > > +       p = lookup_sprite(to_sna_crtc(crtc), idx);
> > > +
> > > +       if (!p->color_encoding.prop)
> > > +               return;
> > > +
> > > +       drmModeObjectSetProperty(to_sna(crtc->scrn)->kgem.fd,
> > > +                                p->id, DRM_MODE_OBJECT_PLANE,
> > > +                                p->color_encoding.prop,
> > > +                                p->color_encoding.values[colorspace]);
> > 
> > Does changing the property trigger an immediate update, or is that
> > deferred until the next SetPlane (or atomic update)?
> 
> Legacy setprop so it'll trigger an update. So if the plane is
> already enabled I suppose we'll get an extra frame of lag here.
> Same deal as for the colorkey ioctl actually.
> 
> I think generally one can assume the the attribute won't be changed
> very often with the plane already enabled. Generally I'd expect the
> client to set this up before the first putimage.

Agreed.

> Converting this to atomic would avoid that extra lag. But we'd need
> to convert the colorkey ioctl to properties as well. And the next step
> after that would tearfree+atomic for making it actually nice :)

If we wanted nice, we wouldn't start with the X11 protocol! Oh well, if
all the shiny new features are going to be hidden being atomic, probably
going to have to bite the bullet sooner or later.

> > It makes no difference to this patch, except we might redisplay the
> > image immediately rather than wait for the client?
> 
> We call this from the putimage hook, so we need the client to
> actually repaint before anything happens. And since we don't have
> reputimage manipulating the window/clipping won't update the
> sprite unless the client repaints (which eg. mplayer won't do
> when paused unless it gets expose events).

I knew you were going to mention the lack of ReputImage! :)
-Chris
diff mbox

Patch

diff --git a/src/sna/sna.h b/src/sna/sna.h
index 5283ce436a3b..c9097d3db158 100644
--- a/src/sna/sna.h
+++ b/src/sna/sna.h
@@ -634,6 +634,7 @@  static inline void sna_present_cancel_flip(struct sna *sna) { }
 
 extern unsigned sna_crtc_count_sprites(xf86CrtcPtr crtc);
 extern bool sna_crtc_set_sprite_rotation(xf86CrtcPtr crtc, unsigned idx, uint32_t rotation);
+extern void sna_crtc_set_sprite_colorspace(xf86CrtcPtr crtc, unsigned idx, int colorspace);
 extern uint32_t sna_crtc_to_sprite(xf86CrtcPtr crtc, unsigned idx);
 extern bool sna_crtc_is_transformed(xf86CrtcPtr crtc);
 
diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
index ea2f148d213c..86aa711f886d 100644
--- a/src/sna/sna_display.c
+++ b/src/sna/sna_display.c
@@ -222,6 +222,10 @@  struct sna_crtc {
 			uint32_t supported;
 			uint32_t current;
 		} rotation;
+		struct {
+			uint32_t prop;
+			uint64_t values[2];
+		} color_encoding;
 		struct list link;
 	} primary;
 	struct list sprites;
@@ -3293,17 +3297,124 @@  static const xf86CrtcFuncsRec sna_crtc_funcs = {
 #endif
 };
 
-inline static bool prop_is_rotation(struct drm_mode_get_property *prop)
+inline static bool prop_has_type_and_name(const struct drm_mode_get_property *prop,
+					  unsigned int type, const char *name)
 {
-	if ((prop->flags & (1 << 5)) == 0)
+	if ((prop->flags & (1 << type)) == 0)
 		return false;
 
-	if (strcmp(prop->name, "rotation"))
+	if (strcmp(prop->name, name))
 		return false;
 
 	return true;
 }
 
+inline static bool prop_is_rotation(const struct drm_mode_get_property *prop)
+{
+	return prop_has_type_and_name(prop, 5, "rotation");
+}
+
+static void parse_rotation_prop(struct sna *sna, struct plane *p,
+				struct drm_mode_get_property *prop,
+				uint64_t value)
+{
+	struct drm_mode_property_enum *enums;
+	int j;
+
+	p->rotation.prop = prop->prop_id;
+	p->rotation.current = value;
+
+	DBG(("%s: found rotation property .id=%d, value=%ld, num_enums=%d\n",
+	     __FUNCTION__, prop->prop_id, value, prop->count_enum_blobs));
+
+	enums = malloc(prop->count_enum_blobs * sizeof(struct drm_mode_property_enum));
+	if (!enums)
+		return;
+
+	prop->count_values = 0;
+	prop->enum_blob_ptr = (uintptr_t)enums;
+
+	if (drmIoctl(sna->kgem.fd, DRM_IOCTL_MODE_GETPROPERTY, prop)) {
+		free(enums);
+		return;
+	}
+
+	/* XXX we assume that the mapping between kernel enum and
+	 * RandR remains fixed for our lifetimes.
+	 */
+	VG(VALGRIND_MAKE_MEM_DEFINED(enums, sizeof(*enums)*prop->count_enum_blobs));
+	for (j = 0; j < prop->count_enum_blobs; j++) {
+		DBG(("%s: rotation[%d] = %s [%lx]\n", __FUNCTION__,
+		     j, enums[j].name, (long)enums[j].value));
+		p->rotation.supported |= 1 << enums[j].value;
+	}
+
+	free(enums);
+}
+
+inline static bool prop_is_color_encoding(const struct drm_mode_get_property *prop)
+{
+	return prop_has_type_and_name(prop, 3, "COLOR_ENCODING");
+}
+
+static void parse_color_encoding_prop(struct sna *sna, struct plane *p,
+				      struct drm_mode_get_property *prop,
+				      uint64_t value)
+{
+	struct drm_mode_property_enum *enums;
+	unsigned int supported = 0;
+	int j;
+
+	DBG(("%s: found color encoding property .id=%d, value=%ld, num_enums=%d\n",
+	     __FUNCTION__, prop->prop_id, (long)value, prop->count_enum_blobs));
+
+	enums = malloc(prop->count_enum_blobs * sizeof(struct drm_mode_property_enum));
+	if (!enums)
+		return;
+
+	prop->count_values = 0;
+	prop->enum_blob_ptr = (uintptr_t)enums;
+
+	if (drmIoctl(sna->kgem.fd, DRM_IOCTL_MODE_GETPROPERTY, prop)) {
+		free(enums);
+		return;
+	}
+
+	VG(VALGRIND_MAKE_MEM_DEFINED(enums, sizeof(*enums)*prop->count_enum_blobs));
+	for (j = 0; j < prop->count_enum_blobs; j++) {
+		if (!strcmp(enums[j].name, "ITU-R BT.601 YCbCr")) {
+			p->color_encoding.values[0] = enums[j].value;
+			supported |= 1 << 0;
+		} else if (!strcmp(enums[j].name, "ITU-R BT.709 YCbCr")) {
+			p->color_encoding.values[1] = enums[j].value;
+			supported |= 1 << 1;
+		}
+	}
+
+	free(enums);
+
+	if (supported == 3)
+		p->color_encoding.prop = prop->prop_id;
+}
+
+void sna_crtc_set_sprite_colorspace(xf86CrtcPtr crtc,
+				    unsigned idx, int colorspace)
+{
+	struct plane *p;
+
+	assert(to_sna_crtc(crtc));
+
+	p = lookup_sprite(to_sna_crtc(crtc), idx);
+
+	if (!p->color_encoding.prop)
+		return;
+
+	drmModeObjectSetProperty(to_sna(crtc->scrn)->kgem.fd,
+				 p->id, DRM_MODE_OBJECT_PLANE,
+				 p->color_encoding.prop,
+				 p->color_encoding.values[colorspace]);
+}
+
 static int plane_details(struct sna *sna, struct plane *p)
 {
 #define N_STACK_PROPS 32 /* must be a multiple of 2 */
@@ -3360,34 +3471,9 @@  static int plane_details(struct sna *sna, struct plane *p)
 		if (strcmp(prop.name, "type") == 0) {
 			type = values[i];
 		} else if (prop_is_rotation(&prop)) {
-			struct drm_mode_property_enum *enums;
-
-			p->rotation.prop = props[i];
-			p->rotation.current = values[i];
-
-			DBG(("%s: found rotation property .id=%d, value=%ld, num_enums=%d\n",
-			     __FUNCTION__, prop.prop_id, (long)values[i], prop.count_enum_blobs));
-			enums = malloc(prop.count_enum_blobs * sizeof(struct drm_mode_property_enum));
-			if (enums != NULL) {
-				prop.count_values = 0;
-				prop.enum_blob_ptr = (uintptr_t)enums;
-
-				if (drmIoctl(sna->kgem.fd, DRM_IOCTL_MODE_GETPROPERTY, &prop) == 0) {
-					int j;
-
-					/* XXX we assume that the mapping between kernel enum and
-					 * RandR remains fixed for our lifetimes.
-					 */
-					VG(VALGRIND_MAKE_MEM_DEFINED(enums, sizeof(*enums)*prop.count_enum_blobs));
-					for (j = 0; j < prop.count_enum_blobs; j++) {
-						DBG(("%s: rotation[%d] = %s [%lx]\n", __FUNCTION__,
-						     j, enums[j].name, (long)enums[j].value));
-						p->rotation.supported |= 1 << enums[j].value;
-					}
-				}
-
-				free(enums);
-			}
+			parse_rotation_prop(sna, p, &prop, values[i]);
+		} else if (prop_is_color_encoding(&prop)) {
+			parse_color_encoding_prop(sna, p, &prop, values[i]);
 		}
 	}
 
diff --git a/src/sna/sna_video.h b/src/sna/sna_video.h
index 6b1c864ce9d8..6abf6d540b18 100644
--- a/src/sna/sna_video.h
+++ b/src/sna/sna_video.h
@@ -100,6 +100,9 @@  struct sna_video {
 	unsigned color_key_changed;
 	bool has_color_key;
 
+	unsigned colorspace;
+	unsigned colorspace_changed;
+
 	/** YUV data buffers */
 	struct kgem_bo *old_buf[2];
 	struct kgem_bo *buf;
diff --git a/src/sna/sna_video_sprite.c b/src/sna/sna_video_sprite.c
index 44898a748e1c..f713ba356b41 100644
--- a/src/sna/sna_video_sprite.c
+++ b/src/sna/sna_video_sprite.c
@@ -69,11 +69,12 @@  struct local_mode_set_plane {
 
 #define MAKE_ATOM(a) MakeAtom(a, sizeof(a) - 1, true)
 
-static Atom xvColorKey, xvAlwaysOnTop, xvSyncToVblank;
+static Atom xvColorKey, xvAlwaysOnTop, xvSyncToVblank, xvColorspace;
 
 static XvFormatRec formats[] = { {15}, {16}, {24} };
 static const XvImageRec images[] = { XVIMAGE_YUY2, XVIMAGE_UYVY, XVMC_RGB888, XVMC_RGB565 };
 static const XvAttributeRec attribs[] = {
+	{ XvSettable | XvGettable, 0, 1, (char *)"XV_COLORSPACE" }, /* BT.601, BT.709 */
 	{ XvSettable | XvGettable, 0, 0xffffff, (char *)"XV_COLORKEY" },
 	{ XvSettable | XvGettable, 0, 1, (char *)"XV_ALWAYS_ON_TOP" },
 };
@@ -119,6 +120,10 @@  static int sna_video_sprite_set_attr(ddSetPortAttribute_ARGS)
 		video->color_key = value;
 		RegionEmpty(&video->clip);
 		DBG(("COLORKEY = %ld\n", (long)value));
+	} else if (attribute == xvColorspace) {
+		video->colorspace_changed = ~0;
+		video->colorspace = value;
+		DBG(("COLORSPACE = %ld\n", (long)value));
 	} else if (attribute == xvSyncToVblank) {
 		DBG(("%s: SYNC_TO_VBLANK: %d -> %d\n", __FUNCTION__,
 		     video->SyncToVblank, !!value));
@@ -140,6 +145,8 @@  static int sna_video_sprite_get_attr(ddGetPortAttribute_ARGS)
 
 	if (attribute == xvColorKey)
 		*value = video->color_key;
+	else if (attribute == xvColorspace)
+		*value = video->colorspace;
 	else if (attribute == xvAlwaysOnTop)
 		*value = video->AlwaysOnTop;
 	else if (attribute == xvSyncToVblank)
@@ -265,6 +272,16 @@  sna_video_sprite_show(struct sna *sna,
 		video->color_key_changed &= ~(1 << pipe);
 	}
 
+	if (video->colorspace_changed & (1 << pipe)) {
+		DBG(("%s: updating colorspace: %x\n",
+		     __FUNCTION__, video->colorspace));
+
+		sna_crtc_set_sprite_colorspace(crtc, video->idx,
+					       video->colorspace);
+
+		video->colorspace_changed &= ~(1 << pipe);
+	}
+
 	update_dst_box_to_crtc_coords(sna, crtc, dstBox);
 	if (frame->rotation & (RR_Rotate_90 | RR_Rotate_270)) {
 		int tmp = frame->width;
@@ -769,6 +786,8 @@  void sna_video_sprite_setup(struct sna *sna, ScreenPtr screen)
 		video->alignment = 64;
 		video->color_key = sna_video_sprite_color_key(sna);
 		video->color_key_changed = ~0;
+		video->colorspace = 1; /* BT.709 */
+		video->colorspace_changed = ~0;
 		video->has_color_key = true;
 		video->brightness = -19;	/* (255/219) * -16 */
 		video->contrast = 75;	/* 255/219 * 64 */
@@ -789,6 +808,7 @@  void sna_video_sprite_setup(struct sna *sna, ScreenPtr screen)
 	adaptor->base_id = adaptor->pPorts[0].id;
 
 	xvColorKey = MAKE_ATOM("XV_COLORKEY");
+	xvColorspace = MAKE_ATOM("XV_COLORSPACE");
 	xvAlwaysOnTop = MAKE_ATOM("XV_ALWAYS_ON_TOP");
 	xvSyncToVblank = MAKE_ATOM("XV_SYNC_TO_VBLANK");