[RFC] drm/i915/bxt: Add pipe_src size property

[RFC] drm/i915/bxt: Add pipe_src size property

[Nabendu Maiti]

This patch is adding pipesource size as property as intel property.User
application is allowed to change the pipe source size in runtime on BXT/SKL.
Added the property as inteli crtc property.

Comments and suggestions are requested for whether we can keep the
property as intel crtc property or move to drm level.

Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_atomic.c  | 66 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 46 +++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |  9 +++++
 4 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cf7e0fc..d789841 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1867,6 +1867,7 @@ struct drm_i915_private {
 
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
+	struct drm_property *crtc_src_size_prop;
 
 	/* hda/i915 audio component */
 	struct i915_audio_component *audio_component;
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 4625f8a..b386ba9 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -73,6 +73,72 @@ intel_connector_atomic_get_property(struct drm_connector *connector,
 	return -EINVAL;
 }
 
+/* intel_crtc_atomic_set_property - duplicate crtc state
+ * @crtc: drm crtc
+ *
+ * Allocates and returns a copy of the crtc state (both common and
+ * Intel-specific) for the specified crtc.
+ *
+ * Returns: The newly allocated crtc state, or NULL on failure.
+ */
+int
+intel_crtc_atomic_set_property(struct drm_crtc *crtc,
+			       struct drm_crtc_state *state,
+			       struct drm_property *property,
+			       uint64_t val)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(state);
+	u32 pipe_src_size = ((intel_crtc_state->pipe_src_w << 16) |
+			     (intel_crtc_state->pipe_src_h));
+	struct drm_display_mode *adjusted_mode =
+		&intel_crtc->config->base.adjusted_mode;
+
+	if (property == dev_priv->crtc_src_size_prop) {
+		if (val != pipe_src_size) {
+			if (val) {
+				intel_crtc_state->pipe_src_w = (val >> 16);
+				intel_crtc_state->pipe_src_h = val & 0x0000ffff;
+			} else {
+
+				/* for 0 set standard modeset calculated values */
+				intel_crtc_state->pipe_src_w = adjusted_mode->hdisplay;
+				intel_crtc_state->pipe_src_h = adjusted_mode->vdisplay;
+			}
+			intel_crtc_state->update_pipe = true;
+		}
+	}
+	return 0;
+}
+
+/*
+ * intel_crtc_atomic_get_property - duplicate crtc state
+ * @crtc: drm crtc
+ *
+ * Get Crtc properties.
+ *
+ * Returns: The newly allocated crtc state, or NULL on failure.
+ */
+int
+intel_crtc_atomic_get_property(struct drm_crtc *crtc,
+			       const struct drm_crtc_state *state,
+			       struct drm_property *property,
+			       uint64_t *val)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(state);
+	u32 pipe_src_size = ((intel_crtc_state->pipe_src_w << 16) |
+		(intel_crtc_state->pipe_src_h));
+
+	if (property == dev_priv->crtc_src_size_prop) {
+		*val = pipe_src_size;
+	}
+	return 0;
+}
+
 /*
  * intel_crtc_duplicate_state - duplicate crtc state
  * @crtc: drm crtc
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2d0b006..ca4e26b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3340,6 +3340,12 @@ static void intel_update_pipe_config(struct intel_crtc *crtc,
 	 * sized surface.
 	 */
 
+	/* Restore modeset values */
+	if (!crtc->config->pipe_src_w && crtc->config->pipe_src_h) {
+		crtc->config->pipe_src_w = adjusted_mode->crtc_hdisplay;
+		crtc->config->pipe_src_h = adjusted_mode->crtc_vdisplay;
+	}
+
 	I915_WRITE(PIPESRC(crtc->pipe),
 		   ((pipe_config->pipe_src_w - 1) << 16) |
 		   (pipe_config->pipe_src_h - 1));
@@ -12068,9 +12074,23 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	}
 
 	if (INTEL_INFO(dev)->gen >= 9) {
-		if (mode_changed)
-			ret = skl_update_scaler_crtc(pipe_config);
+		struct intel_connector *intel_connector;
+		for_each_intel_connector(dev, intel_connector) {
+			if (intel_connector->encoder->base.crtc  != crtc)
+				continue;
+
+			if ((intel_connector != NULL) && ((mode_changed) ||
+							  (pipe_config->update_pipe))) {
+				ret = skl_update_scaler_crtc(pipe_config);
 
+				if (HAS_GMCH_DISPLAY(dev))
+					intel_gmch_panel_fitting(intel_crtc, pipe_config,
+						 intel_connector->panel.fitting_mode);
+				else
+					intel_pch_panel_fitting(intel_crtc, pipe_config,
+						 intel_connector->panel.fitting_mode);
+			}
+		}
 		if (!ret)
 			ret = intel_atomic_setup_scalers(dev, intel_crtc,
 							 pipe_config);
@@ -13674,6 +13694,8 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.page_flip = intel_crtc_page_flip,
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
 	.atomic_destroy_state = intel_crtc_destroy_state,
+	.atomic_set_property = intel_crtc_atomic_set_property,
+	.atomic_get_property = intel_crtc_atomic_get_property,
 };
 
 static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
@@ -14097,6 +14119,23 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	return &primary->base;
 }
 
+void intel_create_crtc_properties(struct drm_device *dev,
+				  struct intel_crtc *intel_crtc)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!dev_priv->crtc_src_size_prop) {
+		dev_priv->crtc_src_size_prop =
+			drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
+			"crtc_src_size", 0, UINT_MAX);
+	}
+
+	/*FIXME: Add modeseted values ?*/
+	if (dev_priv->crtc_src_size_prop)
+		drm_object_attach_property(&intel_crtc->base.base,
+				dev_priv->crtc_src_size_prop, 0);
+}
+
 void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
 {
 	if (!dev->mode_config.rotation_property) {
@@ -14319,6 +14358,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	if (ret)
 		goto fail;
 
+	if (INTEL_INFO(dev)->gen >= 9)
+		intel_create_crtc_properties(dev, intel_crtc);
+
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {
 		intel_crtc->lut_r[i] = i;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0438b57..c787f0b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1586,6 +1586,15 @@ int intel_connector_atomic_get_property(struct drm_connector *connector,
 					struct drm_property *property,
 					uint64_t *val);
 struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
+int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
+				   struct drm_crtc_state *state,
+				   struct drm_property *property, uint64_t val);
+int
+intel_crtc_atomic_get_property(struct drm_crtc *crtc,
+			       const struct drm_crtc_state *state,
+			       struct drm_property *property,
+			       uint64_t *val);
+
 void intel_crtc_destroy_state(struct drm_crtc *crtc,
 			       struct drm_crtc_state *state);
 struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] drm/i915/bxt: Add pipe_src size property

[Maarten Lankhorst]

Hey,

Op 23-12-15 om 12:05 schreef Nabendu Maiti:
> This patch is adding pipesource size as property as intel property.User
> application is allowed to change the pipe source size in runtime on BXT/SKL.
> Added the property as inteli crtc property.
>
> Comments and suggestions are requested for whether we can keep the
> property as intel crtc property or move to drm level.
>
This property would get lost on a modeset. But why do you need a pipe_src property? Set i915.fastboot=1 and do an atomic commit with a changed mode and allow_modeset=false.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] drm/i915/bxt: Add pipe_src size property

[Ville Syrjälä]

On Mon, Jan 04, 2016 at 11:16:39AM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> Op 23-12-15 om 12:05 schreef Nabendu Maiti:
> > This patch is adding pipesource size as property as intel property.User
> > application is allowed to change the pipe source size in runtime on BXT/SKL.
> > Added the property as inteli crtc property.
> >
> > Comments and suggestions are requested for whether we can keep the
> > property as intel crtc property or move to drm level.
> >
> This property would get lost on a modeset. But why do you need a pipe_src property?

It's needed if we want to use the panel fitter with non-eDP/LVDS/DSI
displays.

Last time this came up I decided that we want to expose this via a new
"fixed_mode" property. Ie. userspace can choose the actual display
timings by setting the "fixed_mode" property with a specific mode, and
then the normal mode property will basically just control just the pipe
src size just like it already does for eDP/LVDS/DSI where we already have
the fixed_mode internally (just not exposed to usersapce). If the
fixed_mode is not specified, things will work just as they do right
now. Obviously for eDP/LVDS/DSI we will reject any request to
change/unset the fixed mode.

The benefit of that approach is that things will work exactly the same
way as before unless the user explicitly sets the fixed_mode, and once
it's set thigns will work exactly the same way as they have worked so
far for eDP/LVDS/DSI. Also it keeps the policy of choosing the fixed
mode strictly is userspace for external displays.

And as a bonus we will also expose the eDP/LVDS/DSI fixed_mode to
userspace giving userspace more information on what the actual panel
timings are. Currently userspace has to more or less guess that one
of the modes the connector claims to support has the actual panel
timings.

So far I've not heard any opposing opinions to this plan.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] drm/i915/bxt: Add pipe_src size property

[Daniel Vetter]

On Mon, Jan 04, 2016 at 07:06:15PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 04, 2016 at 11:16:39AM +0100, Maarten Lankhorst wrote:
> > Hey,
> > 
> > Op 23-12-15 om 12:05 schreef Nabendu Maiti:
> > > This patch is adding pipesource size as property as intel property.User
> > > application is allowed to change the pipe source size in runtime on BXT/SKL.
> > > Added the property as inteli crtc property.
> > >
> > > Comments and suggestions are requested for whether we can keep the
> > > property as intel crtc property or move to drm level.
> > >
> > This property would get lost on a modeset. But why do you need a pipe_src property?
> 
> It's needed if we want to use the panel fitter with non-eDP/LVDS/DSI
> displays.
> 
> Last time this came up I decided that we want to expose this via a new
> "fixed_mode" property. Ie. userspace can choose the actual display
> timings by setting the "fixed_mode" property with a specific mode, and
> then the normal mode property will basically just control just the pipe
> src size just like it already does for eDP/LVDS/DSI where we already have
> the fixed_mode internally (just not exposed to usersapce). If the
> fixed_mode is not specified, things will work just as they do right
> now. Obviously for eDP/LVDS/DSI we will reject any request to
> change/unset the fixed mode.
> 
> The benefit of that approach is that things will work exactly the same
> way as before unless the user explicitly sets the fixed_mode, and once
> it's set thigns will work exactly the same way as they have worked so
> far for eDP/LVDS/DSI. Also it keeps the policy of choosing the fixed
> mode strictly is userspace for external displays.
> 
> And as a bonus we will also expose the eDP/LVDS/DSI fixed_mode to
> userspace giving userspace more information on what the actual panel
> timings are. Currently userspace has to more or less guess that one
> of the modes the connector claims to support has the actual panel
> timings.
> 
> So far I've not heard any opposing opinions to this plan.

Hm yeah, pipe_src would run into all kinds of fun in conjunction with the
existing fixed mode stuff we have. Just exposing the fixed as a prop might
be simpler. But then we need to figure out what to do wrt the clock in the
mode passed in through userspace - currently the fixed mode always wins,
but for manual DRRS it would be nice if userspace could control it, and
doesn't have to know that there's a fixed_mode.

So semantically I think only exposing the pipe src to expose panel fitters
would be cleaner. Then we'd need to deal with some internal troubles to
make sure we combine everything correctly, but that shouldn't be too hard
really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] drm/i915/bxt: Add pipe_src size property

[Ville Syrjälä]

On Tue, Jan 05, 2016 at 03:50:38PM +0100, Daniel Vetter wrote:
> On Mon, Jan 04, 2016 at 07:06:15PM +0200, Ville Syrjälä wrote:
> > On Mon, Jan 04, 2016 at 11:16:39AM +0100, Maarten Lankhorst wrote:
> > > Hey,
> > > 
> > > Op 23-12-15 om 12:05 schreef Nabendu Maiti:
> > > > This patch is adding pipesource size as property as intel property.User
> > > > application is allowed to change the pipe source size in runtime on BXT/SKL.
> > > > Added the property as inteli crtc property.
> > > >
> > > > Comments and suggestions are requested for whether we can keep the
> > > > property as intel crtc property or move to drm level.
> > > >
> > > This property would get lost on a modeset. But why do you need a pipe_src property?
> > 
> > It's needed if we want to use the panel fitter with non-eDP/LVDS/DSI
> > displays.
> > 
> > Last time this came up I decided that we want to expose this via a new
> > "fixed_mode" property. Ie. userspace can choose the actual display
> > timings by setting the "fixed_mode" property with a specific mode, and
> > then the normal mode property will basically just control just the pipe
> > src size just like it already does for eDP/LVDS/DSI where we already have
> > the fixed_mode internally (just not exposed to usersapce). If the
> > fixed_mode is not specified, things will work just as they do right
> > now. Obviously for eDP/LVDS/DSI we will reject any request to
> > change/unset the fixed mode.
> > 
> > The benefit of that approach is that things will work exactly the same
> > way as before unless the user explicitly sets the fixed_mode, and once
> > it's set thigns will work exactly the same way as they have worked so
> > far for eDP/LVDS/DSI. Also it keeps the policy of choosing the fixed
> > mode strictly is userspace for external displays.
> > 
> > And as a bonus we will also expose the eDP/LVDS/DSI fixed_mode to
> > userspace giving userspace more information on what the actual panel
> > timings are. Currently userspace has to more or less guess that one
> > of the modes the connector claims to support has the actual panel
> > timings.
> > 
> > So far I've not heard any opposing opinions to this plan.
> 
> Hm yeah, pipe_src would run into all kinds of fun in conjunction with the
> existing fixed mode stuff we have. Just exposing the fixed as a prop might
> be simpler. But then we need to figure out what to do wrt the clock in the
> mode passed in through userspace - currently the fixed mode always wins,
> but for manual DRRS it would be nice if userspace could control it, and
> doesn't have to know that there's a fixed_mode.

We could have the user mode vrefresh indicate the desired refresh rate
of the fixed mode as well. In fact I've been wanting to add a check to
make sure the user mode refresh rate isn't too far off from the
fixed/downlclock mode vrefresh, but didn't get around to it yet.

> 
> So semantically I think only exposing the pipe src to expose panel fitters
> would be cleaner. Then we'd need to deal with some internal troubles to
> make sure we combine everything correctly, but that shouldn't be too hard
> really.

I think it's quite a bit more work since we have to redo all the fb size
checks and whatnot to use the property when specified. I once outlined
a more detailed plan for his approach too (too lazy to dig up the mail),
but I think the fixed mode prop is a simpler approach and won't actually
require much in the way of userspace changes either. It'll be enough to
set the property once and then even the legacy modeset ioctls will work
exactly like they do now for eDP/LVDS/DSI.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] drm/i915/bxt: Add pipe_src size property

[Daniel Vetter]

On Tue, Jan 05, 2016 at 05:18:40PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 05, 2016 at 03:50:38PM +0100, Daniel Vetter wrote:
> > On Mon, Jan 04, 2016 at 07:06:15PM +0200, Ville Syrjälä wrote:
> > > On Mon, Jan 04, 2016 at 11:16:39AM +0100, Maarten Lankhorst wrote:
> > > > Hey,
> > > > 
> > > > Op 23-12-15 om 12:05 schreef Nabendu Maiti:
> > > > > This patch is adding pipesource size as property as intel property.User
> > > > > application is allowed to change the pipe source size in runtime on BXT/SKL.
> > > > > Added the property as inteli crtc property.
> > > > >
> > > > > Comments and suggestions are requested for whether we can keep the
> > > > > property as intel crtc property or move to drm level.
> > > > >
> > > > This property would get lost on a modeset. But why do you need a pipe_src property?
> > > 
> > > It's needed if we want to use the panel fitter with non-eDP/LVDS/DSI
> > > displays.
> > > 
> > > Last time this came up I decided that we want to expose this via a new
> > > "fixed_mode" property. Ie. userspace can choose the actual display
> > > timings by setting the "fixed_mode" property with a specific mode, and
> > > then the normal mode property will basically just control just the pipe
> > > src size just like it already does for eDP/LVDS/DSI where we already have
> > > the fixed_mode internally (just not exposed to usersapce). If the
> > > fixed_mode is not specified, things will work just as they do right
> > > now. Obviously for eDP/LVDS/DSI we will reject any request to
> > > change/unset the fixed mode.
> > > 
> > > The benefit of that approach is that things will work exactly the same
> > > way as before unless the user explicitly sets the fixed_mode, and once
> > > it's set thigns will work exactly the same way as they have worked so
> > > far for eDP/LVDS/DSI. Also it keeps the policy of choosing the fixed
> > > mode strictly is userspace for external displays.
> > > 
> > > And as a bonus we will also expose the eDP/LVDS/DSI fixed_mode to
> > > userspace giving userspace more information on what the actual panel
> > > timings are. Currently userspace has to more or less guess that one
> > > of the modes the connector claims to support has the actual panel
> > > timings.
> > > 
> > > So far I've not heard any opposing opinions to this plan.
> > 
> > Hm yeah, pipe_src would run into all kinds of fun in conjunction with the
> > existing fixed mode stuff we have. Just exposing the fixed as a prop might
> > be simpler. But then we need to figure out what to do wrt the clock in the
> > mode passed in through userspace - currently the fixed mode always wins,
> > but for manual DRRS it would be nice if userspace could control it, and
> > doesn't have to know that there's a fixed_mode.
> 
> We could have the user mode vrefresh indicate the desired refresh rate
> of the fixed mode as well. In fact I've been wanting to add a check to
> make sure the user mode refresh rate isn't too far off from the
> fixed/downlclock mode vrefresh, but didn't get around to it yet.

Yeah agreed, userspace vrefresh should control (or at least be checked
against) the fixed mode vrefresh.

> > So semantically I think only exposing the pipe src to expose panel fitters
> > would be cleaner. Then we'd need to deal with some internal troubles to
> > make sure we combine everything correctly, but that shouldn't be too hard
> > really.
> 
> I think it's quite a bit more work since we have to redo all the fb size
> checks and whatnot to use the property when specified. I once outlined
> a more detailed plan for his approach too (too lazy to dig up the mail),
> but I think the fixed mode prop is a simpler approach and won't actually
> require much in the way of userspace changes either. It'll be enough to
> set the property once and then even the legacy modeset ioctls will work
> exactly like they do now for eDP/LVDS/DSI.

One downside of the fixed mode against an explicit pipe src rectangle is
that the explict rectangle allows us to letter/pillar/stretch/move too.
With just a fixed_mode that's much harder to pull off.

I think for i915 it should be fairly ok-ish to implement this. We just
need to move pipe src rectangle to drm_crtc_state, and adjust all the pfit
config logic to work on the pipe level. Panels would then only replace the
output mode with the fixed panel mode, and leave scaling/pfit selection to
the core crtc code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] drm/i915/bxt: Add pipe_src size property

[Ville Syrjälä]

On Wed, Jan 06, 2016 at 08:57:57AM +0100, Daniel Vetter wrote:
> On Tue, Jan 05, 2016 at 05:18:40PM +0200, Ville Syrjälä wrote:
> > On Tue, Jan 05, 2016 at 03:50:38PM +0100, Daniel Vetter wrote:
> > > On Mon, Jan 04, 2016 at 07:06:15PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Jan 04, 2016 at 11:16:39AM +0100, Maarten Lankhorst wrote:
> > > > > Hey,
> > > > > 
> > > > > Op 23-12-15 om 12:05 schreef Nabendu Maiti:
> > > > > > This patch is adding pipesource size as property as intel property.User
> > > > > > application is allowed to change the pipe source size in runtime on BXT/SKL.
> > > > > > Added the property as inteli crtc property.
> > > > > >
> > > > > > Comments and suggestions are requested for whether we can keep the
> > > > > > property as intel crtc property or move to drm level.
> > > > > >
> > > > > This property would get lost on a modeset. But why do you need a pipe_src property?
> > > > 
> > > > It's needed if we want to use the panel fitter with non-eDP/LVDS/DSI
> > > > displays.
> > > > 
> > > > Last time this came up I decided that we want to expose this via a new
> > > > "fixed_mode" property. Ie. userspace can choose the actual display
> > > > timings by setting the "fixed_mode" property with a specific mode, and
> > > > then the normal mode property will basically just control just the pipe
> > > > src size just like it already does for eDP/LVDS/DSI where we already have
> > > > the fixed_mode internally (just not exposed to usersapce). If the
> > > > fixed_mode is not specified, things will work just as they do right
> > > > now. Obviously for eDP/LVDS/DSI we will reject any request to
> > > > change/unset the fixed mode.
> > > > 
> > > > The benefit of that approach is that things will work exactly the same
> > > > way as before unless the user explicitly sets the fixed_mode, and once
> > > > it's set thigns will work exactly the same way as they have worked so
> > > > far for eDP/LVDS/DSI. Also it keeps the policy of choosing the fixed
> > > > mode strictly is userspace for external displays.
> > > > 
> > > > And as a bonus we will also expose the eDP/LVDS/DSI fixed_mode to
> > > > userspace giving userspace more information on what the actual panel
> > > > timings are. Currently userspace has to more or less guess that one
> > > > of the modes the connector claims to support has the actual panel
> > > > timings.
> > > > 
> > > > So far I've not heard any opposing opinions to this plan.
> > > 
> > > Hm yeah, pipe_src would run into all kinds of fun in conjunction with the
> > > existing fixed mode stuff we have. Just exposing the fixed as a prop might
> > > be simpler. But then we need to figure out what to do wrt the clock in the
> > > mode passed in through userspace - currently the fixed mode always wins,
> > > but for manual DRRS it would be nice if userspace could control it, and
> > > doesn't have to know that there's a fixed_mode.
> > 
> > We could have the user mode vrefresh indicate the desired refresh rate
> > of the fixed mode as well. In fact I've been wanting to add a check to
> > make sure the user mode refresh rate isn't too far off from the
> > fixed/downlclock mode vrefresh, but didn't get around to it yet.
> 
> Yeah agreed, userspace vrefresh should control (or at least be checked
> against) the fixed mode vrefresh.
> 
> > > So semantically I think only exposing the pipe src to expose panel fitters
> > > would be cleaner. Then we'd need to deal with some internal troubles to
> > > make sure we combine everything correctly, but that shouldn't be too hard
> > > really.
> > 
> > I think it's quite a bit more work since we have to redo all the fb size
> > checks and whatnot to use the property when specified. I once outlined
> > a more detailed plan for his approach too (too lazy to dig up the mail),
> > but I think the fixed mode prop is a simpler approach and won't actually
> > require much in the way of userspace changes either. It'll be enough to
> > set the property once and then even the legacy modeset ioctls will work
> > exactly like they do now for eDP/LVDS/DSI.
> 
> One downside of the fixed mode against an explicit pipe src rectangle is
> that the explict rectangle allows us to letter/pillar/stretch/move too.
> With just a fixed_mode that's much harder to pull off.

Nope. It's the same. For both cases we still need some kind of border
property to get real control over the output size/position of the pfit.
The fixed_mode/pipe_src prop would just control the input size.

> 
> I think for i915 it should be fairly ok-ish to implement this. We just
> need to move pipe src rectangle to drm_crtc_state, and adjust all the pfit
> config logic to work on the pipe level. Panels would then only replace the
> output mode with the fixed panel mode, and leave scaling/pfit selection to
> the core crtc code.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] drm/i915/bxt: Add pipe_src size property

[Daniel Vetter]

On Thu, Jan 07, 2016 at 06:56:35PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 06, 2016 at 08:57:57AM +0100, Daniel Vetter wrote:
> > On Tue, Jan 05, 2016 at 05:18:40PM +0200, Ville Syrjälä wrote:
> > > On Tue, Jan 05, 2016 at 03:50:38PM +0100, Daniel Vetter wrote:
> > > > On Mon, Jan 04, 2016 at 07:06:15PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Jan 04, 2016 at 11:16:39AM +0100, Maarten Lankhorst wrote:
> > > > > > Hey,
> > > > > > 
> > > > > > Op 23-12-15 om 12:05 schreef Nabendu Maiti:
> > > > > > > This patch is adding pipesource size as property as intel property.User
> > > > > > > application is allowed to change the pipe source size in runtime on BXT/SKL.
> > > > > > > Added the property as inteli crtc property.
> > > > > > >
> > > > > > > Comments and suggestions are requested for whether we can keep the
> > > > > > > property as intel crtc property or move to drm level.
> > > > > > >
> > > > > > This property would get lost on a modeset. But why do you need a pipe_src property?
> > > > > 
> > > > > It's needed if we want to use the panel fitter with non-eDP/LVDS/DSI
> > > > > displays.
> > > > > 
> > > > > Last time this came up I decided that we want to expose this via a new
> > > > > "fixed_mode" property. Ie. userspace can choose the actual display
> > > > > timings by setting the "fixed_mode" property with a specific mode, and
> > > > > then the normal mode property will basically just control just the pipe
> > > > > src size just like it already does for eDP/LVDS/DSI where we already have
> > > > > the fixed_mode internally (just not exposed to usersapce). If the
> > > > > fixed_mode is not specified, things will work just as they do right
> > > > > now. Obviously for eDP/LVDS/DSI we will reject any request to
> > > > > change/unset the fixed mode.
> > > > > 
> > > > > The benefit of that approach is that things will work exactly the same
> > > > > way as before unless the user explicitly sets the fixed_mode, and once
> > > > > it's set thigns will work exactly the same way as they have worked so
> > > > > far for eDP/LVDS/DSI. Also it keeps the policy of choosing the fixed
> > > > > mode strictly is userspace for external displays.
> > > > > 
> > > > > And as a bonus we will also expose the eDP/LVDS/DSI fixed_mode to
> > > > > userspace giving userspace more information on what the actual panel
> > > > > timings are. Currently userspace has to more or less guess that one
> > > > > of the modes the connector claims to support has the actual panel
> > > > > timings.
> > > > > 
> > > > > So far I've not heard any opposing opinions to this plan.
> > > > 
> > > > Hm yeah, pipe_src would run into all kinds of fun in conjunction with the
> > > > existing fixed mode stuff we have. Just exposing the fixed as a prop might
> > > > be simpler. But then we need to figure out what to do wrt the clock in the
> > > > mode passed in through userspace - currently the fixed mode always wins,
> > > > but for manual DRRS it would be nice if userspace could control it, and
> > > > doesn't have to know that there's a fixed_mode.
> > > 
> > > We could have the user mode vrefresh indicate the desired refresh rate
> > > of the fixed mode as well. In fact I've been wanting to add a check to
> > > make sure the user mode refresh rate isn't too far off from the
> > > fixed/downlclock mode vrefresh, but didn't get around to it yet.
> > 
> > Yeah agreed, userspace vrefresh should control (or at least be checked
> > against) the fixed mode vrefresh.
> > 
> > > > So semantically I think only exposing the pipe src to expose panel fitters
> > > > would be cleaner. Then we'd need to deal with some internal troubles to
> > > > make sure we combine everything correctly, but that shouldn't be too hard
> > > > really.
> > > 
> > > I think it's quite a bit more work since we have to redo all the fb size
> > > checks and whatnot to use the property when specified. I once outlined
> > > a more detailed plan for his approach too (too lazy to dig up the mail),
> > > but I think the fixed mode prop is a simpler approach and won't actually
> > > require much in the way of userspace changes either. It'll be enough to
> > > set the property once and then even the legacy modeset ioctls will work
> > > exactly like they do now for eDP/LVDS/DSI.
> > 
> > One downside of the fixed mode against an explicit pipe src rectangle is
> > that the explict rectangle allows us to letter/pillar/stretch/move too.
> > With just a fixed_mode that's much harder to pull off.
> 
> Nope. It's the same. For both cases we still need some kind of border
> property to get real control over the output size/position of the pfit.
> The fixed_mode/pipe_src prop would just control the input size.

Oh, I thought we'd just go ahead an do a full rectangle, with x/y/w/h for
pipe_src. Then userspace can figure out what exactly it wants.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] drm/i915/bxt: Add pipe_src size property

[Ville Syrjälä]

On Thu, Jan 07, 2016 at 05:58:39PM +0100, Daniel Vetter wrote:
> On Thu, Jan 07, 2016 at 06:56:35PM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 06, 2016 at 08:57:57AM +0100, Daniel Vetter wrote:
> > > On Tue, Jan 05, 2016 at 05:18:40PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Jan 05, 2016 at 03:50:38PM +0100, Daniel Vetter wrote:
> > > > > On Mon, Jan 04, 2016 at 07:06:15PM +0200, Ville Syrjälä wrote:
> > > > > > On Mon, Jan 04, 2016 at 11:16:39AM +0100, Maarten Lankhorst wrote:
> > > > > > > Hey,
> > > > > > > 
> > > > > > > Op 23-12-15 om 12:05 schreef Nabendu Maiti:
> > > > > > > > This patch is adding pipesource size as property as intel property.User
> > > > > > > > application is allowed to change the pipe source size in runtime on BXT/SKL.
> > > > > > > > Added the property as inteli crtc property.
> > > > > > > >
> > > > > > > > Comments and suggestions are requested for whether we can keep the
> > > > > > > > property as intel crtc property or move to drm level.
> > > > > > > >
> > > > > > > This property would get lost on a modeset. But why do you need a pipe_src property?
> > > > > > 
> > > > > > It's needed if we want to use the panel fitter with non-eDP/LVDS/DSI
> > > > > > displays.
> > > > > > 
> > > > > > Last time this came up I decided that we want to expose this via a new
> > > > > > "fixed_mode" property. Ie. userspace can choose the actual display
> > > > > > timings by setting the "fixed_mode" property with a specific mode, and
> > > > > > then the normal mode property will basically just control just the pipe
> > > > > > src size just like it already does for eDP/LVDS/DSI where we already have
> > > > > > the fixed_mode internally (just not exposed to usersapce). If the
> > > > > > fixed_mode is not specified, things will work just as they do right
> > > > > > now. Obviously for eDP/LVDS/DSI we will reject any request to
> > > > > > change/unset the fixed mode.
> > > > > > 
> > > > > > The benefit of that approach is that things will work exactly the same
> > > > > > way as before unless the user explicitly sets the fixed_mode, and once
> > > > > > it's set thigns will work exactly the same way as they have worked so
> > > > > > far for eDP/LVDS/DSI. Also it keeps the policy of choosing the fixed
> > > > > > mode strictly is userspace for external displays.
> > > > > > 
> > > > > > And as a bonus we will also expose the eDP/LVDS/DSI fixed_mode to
> > > > > > userspace giving userspace more information on what the actual panel
> > > > > > timings are. Currently userspace has to more or less guess that one
> > > > > > of the modes the connector claims to support has the actual panel
> > > > > > timings.
> > > > > > 
> > > > > > So far I've not heard any opposing opinions to this plan.
> > > > > 
> > > > > Hm yeah, pipe_src would run into all kinds of fun in conjunction with the
> > > > > existing fixed mode stuff we have. Just exposing the fixed as a prop might
> > > > > be simpler. But then we need to figure out what to do wrt the clock in the
> > > > > mode passed in through userspace - currently the fixed mode always wins,
> > > > > but for manual DRRS it would be nice if userspace could control it, and
> > > > > doesn't have to know that there's a fixed_mode.
> > > > 
> > > > We could have the user mode vrefresh indicate the desired refresh rate
> > > > of the fixed mode as well. In fact I've been wanting to add a check to
> > > > make sure the user mode refresh rate isn't too far off from the
> > > > fixed/downlclock mode vrefresh, but didn't get around to it yet.
> > > 
> > > Yeah agreed, userspace vrefresh should control (or at least be checked
> > > against) the fixed mode vrefresh.
> > > 
> > > > > So semantically I think only exposing the pipe src to expose panel fitters
> > > > > would be cleaner. Then we'd need to deal with some internal troubles to
> > > > > make sure we combine everything correctly, but that shouldn't be too hard
> > > > > really.
> > > > 
> > > > I think it's quite a bit more work since we have to redo all the fb size
> > > > checks and whatnot to use the property when specified. I once outlined
> > > > a more detailed plan for his approach too (too lazy to dig up the mail),
> > > > but I think the fixed mode prop is a simpler approach and won't actually
> > > > require much in the way of userspace changes either. It'll be enough to
> > > > set the property once and then even the legacy modeset ioctls will work
> > > > exactly like they do now for eDP/LVDS/DSI.
> > > 
> > > One downside of the fixed mode against an explicit pipe src rectangle is
> > > that the explict rectangle allows us to letter/pillar/stretch/move too.
> > > With just a fixed_mode that's much harder to pull off.
> > 
> > Nope. It's the same. For both cases we still need some kind of border
> > property to get real control over the output size/position of the pfit.
> > The fixed_mode/pipe_src prop would just control the input size.
> 
> Oh, I thought we'd just go ahead an do a full rectangle, with x/y/w/h for
> pipe_src. Then userspace can figure out what exactly it wants.

Pipe src has no x/y. That'd be pipe dst then. We could define it like
that, or via borders.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx