[PATCH RFC xf86-video-intel 1/2] Update the rotation property whenever we change the rotation.

[PATCH RFC xf86-video-intel 1/2] Update the rotation property whenever we change the rotation.

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Don't worry if that fails: only the KVMr feature will be affected.

We still need to change the sna/ code.
We also need to add the dependency on libdrm.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 src/intel.h         |    3 ++
 src/intel_display.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/intel_driver.c  |    2 +
 3 files changed, 85 insertions(+), 0 deletions(-)

This is just an RFC. Read comment from Kernel patch 0007.

diff --git a/src/intel.h b/src/intel.h
index f806aea..29df531 100644
--- a/src/intel.h
+++ b/src/intel.h
@@ -644,4 +644,7 @@ static inline Bool intel_pixmap_is_offscreen(PixmapPtr pixmap)
 	return priv && priv->offscreen;
 }
 
+/* intel_display.c */
+void intel_crtc_leave_vt(ScrnInfoPtr scrn);
+
 #endif /* _I830_H_ */
diff --git a/src/intel_display.c b/src/intel_display.c
index abdc372..0dd85b8 100644
--- a/src/intel_display.c
+++ b/src/intel_display.c
@@ -82,6 +82,7 @@ struct intel_crtc {
 	uint32_t rotate_fb_id;
 	xf86CrtcPtr crtc;
 	struct list link;
+	drmModePropertyPtr rotation_prop;
 };
 
 struct intel_property {
@@ -418,6 +419,44 @@ done:
 	return ret;
 }
 
+static void
+intel_crtc_inform_rotation(xf86CrtcPtr crtc, Rotation rotation)
+{
+	struct intel_crtc *intel_crtc = crtc->driver_private;
+	int ret, int_rotation;
+
+	/* Try to inform the Kernel about our current rotation but don't
+	 * worry if that fails */
+	if (!intel_crtc->rotation_prop)
+		return;
+
+	switch (rotation) {
+	case RR_Rotate_0:
+		int_rotation = 0;
+		break;
+	case RR_Rotate_90:
+		int_rotation = 90;
+		break;
+	case RR_Rotate_180:
+		int_rotation = 180;
+		break;
+	case RR_Rotate_270:
+		int_rotation = 270;
+		break;
+	default:
+		int_rotation = 0;
+	}
+
+	ret = drmModeObjectSetProperty(intel_crtc->mode->fd,
+				       crtc_id(intel_crtc),
+				       DRM_MODE_OBJECT_CRTC,
+				       intel_crtc->rotation_prop->prop_id,
+				       int_rotation);
+	if (ret)
+		xf86DrvMsg(crtc->scrn->scrnIndex, X_WARNING,
+			   "Failed to set CRTC rotation: %s\n", strerror(-ret));
+}
+
 static Bool
 intel_crtc_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 			  Rotation rotation, int x, int y)
@@ -464,6 +503,8 @@ intel_crtc_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 		crtc->y = saved_y;
 		crtc->rotation = saved_rotation;
 		crtc->mode = saved_mode;
+	} else {
+		intel_crtc_inform_rotation(crtc, rotation);
 	}
 	return ret;
 }
@@ -640,12 +681,49 @@ intel_crtc_destroy(xf86CrtcPtr crtc)
 		intel_crtc->cursor = NULL;
 	}
 
+	if (intel_crtc->rotation_prop)
+		drmModeFreeProperty(intel_crtc->rotation_prop);
+
 	list_del(&intel_crtc->link);
 	free(intel_crtc);
 
 	crtc->driver_private = NULL;
 }
 
+static void
+intel_crtc_load_properties(struct intel_mode *mode,
+			   struct intel_crtc *crtc)
+{
+	unsigned int i;
+	drmModeObjectPropertiesPtr props;
+	drmModePropertyPtr prop;
+
+	crtc->rotation_prop = NULL;
+
+	props = drmModeObjectGetProperties(mode->fd, crtc_id(crtc),
+					   DRM_MODE_OBJECT_CRTC);
+	if (props) {
+		for (i = 0; i < props->count_props; i++) {
+			prop = drmModeGetProperty(mode->fd, props->props[i]);
+			if (!strcmp(prop->name, "rotation"))
+				crtc->rotation_prop = prop;
+			else
+				drmModeFreeProperty(prop);
+		}
+		drmModeFreeObjectProperties(props);
+	}
+}
+
+void
+intel_crtc_leave_vt(ScrnInfoPtr scrn)
+{
+	int i;
+	xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
+
+	for (i = 0; i < xf86_config->num_crtc; i++)
+		intel_crtc_inform_rotation(xf86_config->crtc[i], RR_Rotate_0);
+}
+
 static const xf86CrtcFuncsRec intel_crtc_funcs = {
 	.dpms = intel_crtc_dpms,
 	.set_mode_major = intel_crtc_set_mode_major,
@@ -692,6 +770,8 @@ intel_crtc_init(ScrnInfoPtr scrn, struct intel_mode *mode, int num)
 
 	intel_crtc->crtc = crtc;
 	list_add(&intel_crtc->link, &mode->crtcs);
+
+	intel_crtc_load_properties(mode, intel_crtc);
 }
 
 static Bool
diff --git a/src/intel_driver.c b/src/intel_driver.c
index 4265de8..98466a3 100644
--- a/src/intel_driver.c
+++ b/src/intel_driver.c
@@ -1115,6 +1115,8 @@ static void I830LeaveVT(int scrnIndex, int flags)
 	intel_screen_private *intel = intel_get_screen_private(scrn);
 	int ret;
 
+	intel_crtc_leave_vt(scrn);
+
 	xf86RotateFreeShadow(scrn);
 
 	xf86_hide_cursors(scrn);
-- 
1.7.9.1

[PATCH RFC xf86-video-intel 2/2] Add underscan properties

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

In the Kernel side, these are crtc properties (since in the hardware,
underscan use the panel fitters, which are attached to the pipes).
Ideally we should make these as crtc properties too, but since xrandr
doesn't have support for them, we expose the atoms as output
properties. This is not the best solution, but making the kernel
underscan properties be output properties instead of crtc properties
would be even more wrong.

We still need to change the sna/ code.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 src/intel_display.c |  116 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 115 insertions(+), 1 deletions(-)

diff --git a/src/intel_display.c b/src/intel_display.c
index 0dd85b8..c669981 100644
--- a/src/intel_display.c
+++ b/src/intel_display.c
@@ -83,6 +83,8 @@ struct intel_crtc {
 	xf86CrtcPtr crtc;
 	struct list link;
 	drmModePropertyPtr rotation_prop;
+	drmModePropertyPtr underscan_h_prop;
+	drmModePropertyPtr underscan_v_prop;
 };
 
 struct intel_property {
@@ -111,6 +113,8 @@ struct intel_output {
 	int backlight_max;
 	xf86OutputPtr output;
 	struct list link;
+	Atom underscan_h_atom;
+	Atom underscan_v_atom;
 };
 
 static void
@@ -683,6 +687,10 @@ intel_crtc_destroy(xf86CrtcPtr crtc)
 
 	if (intel_crtc->rotation_prop)
 		drmModeFreeProperty(intel_crtc->rotation_prop);
+	if (intel_crtc->underscan_h_prop)
+		drmModeFreeProperty(intel_crtc->underscan_h_prop);
+	if (intel_crtc->underscan_v_prop)
+		drmModeFreeProperty(intel_crtc->underscan_v_prop);
 
 	list_del(&intel_crtc->link);
 	free(intel_crtc);
@@ -699,6 +707,8 @@ intel_crtc_load_properties(struct intel_mode *mode,
 	drmModePropertyPtr prop;
 
 	crtc->rotation_prop = NULL;
+	crtc->underscan_h_prop = NULL;
+	crtc->underscan_v_prop = NULL;
 
 	props = drmModeObjectGetProperties(mode->fd, crtc_id(crtc),
 					   DRM_MODE_OBJECT_CRTC);
@@ -707,6 +717,10 @@ intel_crtc_load_properties(struct intel_mode *mode,
 			prop = drmModeGetProperty(mode->fd, props->props[i]);
 			if (!strcmp(prop->name, "rotation"))
 				crtc->rotation_prop = prop;
+			else if (!strcmp(prop->name, "underscan hborder"))
+				crtc->underscan_h_prop = prop;
+			else if (!strcmp(prop->name, "underscan vborder"))
+				crtc->underscan_v_prop = prop;
 			else
 				drmModeFreeProperty(prop);
 		}
@@ -1109,6 +1123,8 @@ intel_output_create_resources(xf86OutputPtr output)
 	struct intel_output *intel_output = output->driver_private;
 	drmModeConnectorPtr mode_output = intel_output->mode_output;
 	struct intel_mode *mode = intel_output->mode;
+	struct intel_crtc *crtc;
+	Bool has_underscan_props = FALSE;
 	int i, j, err;
 
 	intel_output->props = calloc(mode_output->count_props,
@@ -1201,6 +1217,33 @@ intel_output_create_resources(xf86OutputPtr output)
 					intel_output->backlight_active_level,
 					FALSE);
 	}
+
+	list_for_each_entry(crtc, &mode->crtcs, link) {
+		if (crtc->underscan_h_prop) {
+			has_underscan_props = TRUE;
+			break;
+		}
+	}
+
+	if (has_underscan_props) {
+		intel_output_create_ranged_atom(output,
+						&intel_output->underscan_h_atom,
+						"underscan hborder", 0, 100, 0,
+						FALSE);
+		intel_output_create_ranged_atom(output,
+						&intel_output->underscan_v_atom,
+						"underscan vborder", 0, 100, 0,
+						FALSE);
+	}
+}
+
+static struct intel_crtc *
+intel_output_get_crtc(xf86OutputPtr output)
+{
+	if (output->crtc)
+		return output->crtc->driver_private;
+	else
+		return 0;
 }
 
 static Bool
@@ -1230,6 +1273,37 @@ intel_output_set_property(xf86OutputPtr output, Atom property,
 		return TRUE;
 	}
 
+	if (property == intel_output->underscan_h_atom ||
+	    property == intel_output->underscan_v_atom) {
+		uint32_t val;
+		struct intel_crtc *intel_crtc;
+		drmModePropertyPtr drm_prop;
+
+		val = *(uint32_t *)value->data;
+		if (value->type != XA_INTEGER || value->format != 32 ||
+		    value->size != 1 || val > 100)
+			return FALSE;
+
+		intel_crtc = intel_output_get_crtc(output);
+		if (intel_crtc) {
+			if (property == intel_output->underscan_h_atom)
+				drm_prop = intel_crtc->underscan_h_prop;
+			else
+				drm_prop = intel_crtc->underscan_v_prop;
+
+			if (!drm_prop)
+				return FALSE;
+
+			drmModeObjectSetProperty(mode->fd,
+						 intel_crtc->mode_crtc->crtc_id,
+						 DRM_MODE_OBJECT_CRTC,
+						 drm_prop->prop_id,
+						 (uint64_t) val);
+
+		}
+		return TRUE;
+	}
+
 	for (i = 0; i < intel_output->num_props; i++) {
 		struct intel_property *p = &intel_output->props[i];
 
@@ -1280,10 +1354,11 @@ static Bool
 intel_output_get_property(xf86OutputPtr output, Atom property)
 {
 	struct intel_output *intel_output = output->driver_private;
+	struct intel_mode *mode = intel_output->mode;
 	int err;
+	INT32 val;
 
 	if (property == backlight_atom || property == backlight_deprecated_atom) {
-		INT32 val;
 
 		if (! intel_output->backlight_iface)
 			return FALSE;
@@ -1304,6 +1379,45 @@ intel_output_get_property(xf86OutputPtr output, Atom property)
 		return TRUE;
 	}
 
+	if (property == intel_output->underscan_h_atom ||
+	    property == intel_output->underscan_v_atom) {
+		struct intel_crtc *intel_crtc = NULL;
+		drmModeObjectPropertiesPtr props;
+		unsigned int i;
+
+		intel_crtc = intel_output_get_crtc(output);
+
+		if (!intel_crtc) {
+			val = 0;
+		} else {
+			props = drmModeObjectGetProperties(mode->fd,
+						intel_crtc->mode_crtc->crtc_id,
+						DRM_MODE_OBJECT_CRTC);
+			if (!props)
+				return FALSE;
+
+			for (i = 0; i < props->count_props; i++) {
+				if (props->props[i] == intel_crtc->underscan_h_prop->prop_id &&
+				    property == intel_output->underscan_h_atom)
+					val = props->prop_values[i];
+				else if (props->props[i] == intel_crtc->underscan_v_prop->prop_id &&
+					 property == intel_output->underscan_v_atom)
+					val = props->prop_values[i];
+			}
+			drmModeFreeObjectProperties(props);
+		}
+
+		err = RRChangeOutputProperty(output->randr_output, property,
+					     XA_INTEGER, 32, PropModeReplace,
+					     1, &val, FALSE, TRUE);
+		if (err != 0) {
+			xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+				   "RRChangeOutputProperty error, %d\n", err);
+			return FALSE;
+		}
+		return TRUE;
+	}
+
 	return FALSE;
 }
 
-- 
1.7.9.1

Re: [PATCH RFC xf86-video-intel 2/2] Add underscan properties

[Chris Wilson]

On Thu, 29 Mar 2012 18:30:20 -0300, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> In the Kernel side, these are crtc properties (since in the hardware,
> underscan use the panel fitters, which are attached to the pipes).
> Ideally we should make these as crtc properties too, but since xrandr
> doesn't have support for them, we expose the atoms as output
> properties. This is not the best solution, but making the kernel
> underscan properties be output properties instead of crtc properties
> would be even more wrong.

However, we have set the precedent with panel fitting already and this is
just a variation on that theme? My wish would be to be able describe the
dst box of the framebuffer on the crtc as part of the mode.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH RFC xf86-video-intel 2/2] Add underscan properties

[Ville Syrjälä]

On Thu, Mar 29, 2012 at 11:19:59PM +0100, Chris Wilson wrote:
> On Thu, 29 Mar 2012 18:30:20 -0300, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > In the Kernel side, these are crtc properties (since in the hardware,
> > underscan use the panel fitters, which are attached to the pipes).
> > Ideally we should make these as crtc properties too, but since xrandr
> > doesn't have support for them, we expose the atoms as output
> > properties. This is not the best solution, but making the kernel
> > underscan properties be output properties instead of crtc properties
> > would be even more wrong.
> 
> However, we have set the precedent with panel fitting already and this is
> just a variation on that theme? My wish would be to be able describe the
> dst box of the framebuffer on the crtc as part of the mode.

I'm assuming you mean that you'd like to have the overscan borders
be part of the mode structure itself. Agreed. However there's another
problem with the current mode setting API.

This is the full display pipeline as I see it:

fb area [w0,h0] -> [x1,y1,w1,h1] plane transform [x2,y2,w2,h2] ->
 CRTC area [w3,h3] -> [0,0,w3,h3] crtc transform [x4,y4,w4,h4] ->
 display mode active area [hdisp,vdisp]

Rectangle [x1,y1,w1,h1] is within the fb area [w0,h0]
Rectangle [x2,y2,w2,h2] is within the CRTC area [w3,h3]
Rectangle [x4,y4,w4,h4] is within the display mode active area [hdisp,vdisp]

Currently we're missing the CRTC area and the overscan borders. This
means you have to add some policy into the kernel on how to choose
the display mode, and how to program panel fitters and whatnot. Sure
you can add properties to influence those policy decisions, but what
you end up with is still quite limited.

If the API would allow you to specify both the borders, and the CRTC
area size, you could do away with all mode selection policy, and
panel fitter and border properties, and you would end up with a more
powerful API.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH RFC xf86-video-intel 1/2] Update the rotation property whenever we change the rotation.

[Chris Wilson]

On Thu, 29 Mar 2012 18:30:19 -0300, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Don't worry if that fails: only the KVMr feature will be affected.
> 
> We still need to change the sna/ code.
> We also need to add the dependency on libdrm.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  src/intel.h         |    3 ++
>  src/intel_display.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/intel_driver.c  |    2 +
>  3 files changed, 85 insertions(+), 0 deletions(-)
> 
> This is just an RFC. Read comment from Kernel patch 0007.
> 
> diff --git a/src/intel.h b/src/intel.h
> index f806aea..29df531 100644
> --- a/src/intel.h
> +++ b/src/intel.h
> @@ -644,4 +644,7 @@ static inline Bool intel_pixmap_is_offscreen(PixmapPtr pixmap)
>  	return priv && priv->offscreen;
>  }
>  
> +/* intel_display.c */
> +void intel_crtc_leave_vt(ScrnInfoPtr scrn);

This is the only minor issue I have with this patch (other than its
influx natiure ;). I would rather call this intel_mode_leave_vt() to
be consistent with the other callouts from intel_driver.c
Also consider whether we need to manually restore the rotation parameter
upon CloseScreen (shutdown). However, I strongly doubt that we should be
the ones to be restoring the rotation parameter, as it should be whoever
then assumes control of the vt establishes the mode to its liking. This
also covers the case of abnormal program termination and panics.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre