[PATCH 1/4] drm/i915: Adding intel_panel_scale_none() helper function

[PATCH 1/4] drm/i915: Adding intel_panel_scale_none() helper function

[Xiong Zhang]

From: dell <dell@localhost.localdomain>

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h   |  1 +
 drivers/gpu/drm/i915/intel_panel.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 47cef0e..f57a0b4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1287,6 +1287,7 @@ int intel_panel_init(struct intel_panel *panel,
 void intel_panel_fini(struct intel_panel *panel);
 void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
 			    struct drm_display_mode *adjusted_mode);
+bool intel_panel_scale_none(struct intel_panel *panel);
 void intel_pch_panel_fitting(struct intel_crtc *crtc,
 			     struct intel_crtc_state *pipe_config,
 			     int fitting_mode);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index e2ab3f6..4a573ac 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -46,6 +46,16 @@ intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
 	drm_mode_set_crtcinfo(adjusted_mode, 0);
 }
 
+bool
+intel_panel_scale_none(struct intel_panel *panel)
+{
+	if (panel->fitting_mode == DRM_MODE_SCALE_NONE ||
+	    panel->fixed_mode == NULL)
+		return true;
+	else
+		return false;
+}
+
 /**
  * intel_find_panel_downclock - find the reduced downclock for LVDS in EDID
  * @dev: drm device
-- 
1.8.2.1

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

[PATCH 2/4] drm/i915: Adding Panel Filter function for DP

[Xiong Zhang]

Only internal eDP, LVDS, DVI screen could set scalling mode, some
customers need to set scalling mode for external DP, HDMI, VGA
screen. Let's fulfill this.

bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90989

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 63 ++++++++++++++++++++++++++++-------------
 1 file changed, 44 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f1b9f93..2da334b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -207,7 +207,13 @@ intel_dp_mode_valid(struct drm_connector *connector,
 	int target_clock = mode->clock;
 	int max_rate, mode_rate, max_lanes, max_link_clock;
 
-	if (is_edp(intel_dp) && fixed_mode) {
+	if (mode->clock < 10000)
+		return MODE_CLOCK_LOW;
+
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
+		return MODE_H_ILLEGAL;
+
+	if (!intel_panel_scale_none(&intel_connector->panel)) {
 		if (mode->hdisplay > fixed_mode->hdisplay)
 			return MODE_PANEL;
 
@@ -226,12 +232,6 @@ intel_dp_mode_valid(struct drm_connector *connector,
 	if (mode_rate > max_rate)
 		return MODE_CLOCK_HIGH;
 
-	if (mode->clock < 10000)
-		return MODE_CLOCK_LOW;
-
-	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
-		return MODE_H_ILLEGAL;
-
 	return MODE_OK;
 }
 
@@ -1378,7 +1378,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	pipe_config->has_drrs = false;
 	pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
 
-	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
+	if (!intel_panel_scale_none(&intel_connector->panel)) {
 		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
 				       adjusted_mode);
 
@@ -4592,6 +4592,23 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 	edid = intel_connector->detect_edid;
 	if (edid) {
 		int ret = intel_connector_update_modes(connector, edid);
+
+		if (ret && intel_connector->panel.fixed_mode == NULL) {
+			/* init fixed mode as preferred mode for DP */
+			struct drm_display_mode *fixed_mode = NULL;
+			struct drm_display_mode *scan;
+
+			list_for_each_entry(scan, &connector->probed_modes, head) {
+				if (scan->type & DRM_MODE_TYPE_PREFERRED)
+					fixed_mode = drm_mode_duplicate(connector->dev,
+									scan);
+			}
+
+			if (fixed_mode)
+				intel_panel_init(&intel_connector->panel,
+						 fixed_mode, NULL);
+		}
+
 		if (ret)
 			return ret;
 	}
@@ -4688,15 +4705,14 @@ intel_dp_set_property(struct drm_connector *connector,
 		goto done;
 	}
 
-	if (is_edp(intel_dp) &&
-	    property == connector->dev->mode_config.scaling_mode_property) {
-		if (val == DRM_MODE_SCALE_NONE) {
-			DRM_DEBUG_KMS("no scaling not supported\n");
+	if (property == connector->dev->mode_config.scaling_mode_property) {
+		if (is_edp(intel_dp) && val == DRM_MODE_SCALE_NONE) {
+			DRM_DEBUG_KMS("eDP: no scaling not supported\n");
 			return -EINVAL;
 		}
 
 		if (intel_connector->panel.fitting_mode == val) {
-			/* the eDP scaling property is not changed */
+			/* the connector scaling property is not changed */
 			return 0;
 		}
 		intel_connector->panel.fitting_mode = val;
@@ -4989,13 +5005,22 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 	intel_attach_broadcast_rgb_property(connector);
 	intel_dp->color_range_auto = true;
 
-	if (is_edp(intel_dp)) {
+	/* Each pipe has panel filter since Ironlake. */
+	if (INTEL_INFO(connector->dev)->gen >= 5) {
 		drm_mode_create_scaling_mode_property(connector->dev);
-		drm_object_attach_property(
-			&connector->base,
-			connector->dev->mode_config.scaling_mode_property,
-			DRM_MODE_SCALE_ASPECT);
-		intel_connector->panel.fitting_mode = DRM_MODE_SCALE_ASPECT;
+		if (is_edp(intel_dp)) {
+			drm_object_attach_property(
+				&connector->base,
+				connector->dev->mode_config.scaling_mode_property,
+				DRM_MODE_SCALE_ASPECT);
+			intel_connector->panel.fitting_mode = DRM_MODE_SCALE_ASPECT;
+		} else {
+			drm_object_attach_property(
+				&connector->base,
+				connector->dev->mode_config.scaling_mode_property,
+				DRM_MODE_SCALE_NONE);
+			intel_connector->panel.fitting_mode = DRM_MODE_SCALE_NONE;
+		}
 	}
 }
 
-- 
1.8.2.1

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

Re: [PATCH 2/4] drm/i915: Adding Panel Filter function for DP

[Ville Syrjälä]

On Mon, Aug 10, 2015 at 03:26:09PM +0800, Xiong Zhang wrote:
> Only internal eDP, LVDS, DVI screen could set scalling mode, some
> customers need to set scalling mode for external DP, HDMI, VGA
> screen. Let's fulfill this.
> 
> bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90989
> 
> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 63 ++++++++++++++++++++++++++++-------------
>  1 file changed, 44 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f1b9f93..2da334b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -207,7 +207,13 @@ intel_dp_mode_valid(struct drm_connector *connector,
>  	int target_clock = mode->clock;
>  	int max_rate, mode_rate, max_lanes, max_link_clock;
>  
> -	if (is_edp(intel_dp) && fixed_mode) {
> +	if (mode->clock < 10000)
> +		return MODE_CLOCK_LOW;
> +
> +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> +		return MODE_H_ILLEGAL;
> +
> +	if (!intel_panel_scale_none(&intel_connector->panel)) {
>  		if (mode->hdisplay > fixed_mode->hdisplay)
>  			return MODE_PANEL;
>  
> @@ -226,12 +232,6 @@ intel_dp_mode_valid(struct drm_connector *connector,
>  	if (mode_rate > max_rate)
>  		return MODE_CLOCK_HIGH;
>  
> -	if (mode->clock < 10000)
> -		return MODE_CLOCK_LOW;
> -
> -	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> -		return MODE_H_ILLEGAL;
> -
>  	return MODE_OK;
>  }
>  
> @@ -1378,7 +1378,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	pipe_config->has_drrs = false;
>  	pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
>  
> -	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> +	if (!intel_panel_scale_none(&intel_connector->panel)) {
>  		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
>  				       adjusted_mode);
>  
> @@ -4592,6 +4592,23 @@ static int intel_dp_get_modes(struct drm_connector *connector)
>  	edid = intel_connector->detect_edid;
>  	if (edid) {
>  		int ret = intel_connector_update_modes(connector, edid);
> +
> +		if (ret && intel_connector->panel.fixed_mode == NULL) {
> +			/* init fixed mode as preferred mode for DP */
> +			struct drm_display_mode *fixed_mode = NULL;
> +			struct drm_display_mode *scan;
> +
> +			list_for_each_entry(scan, &connector->probed_modes, head) {
> +				if (scan->type & DRM_MODE_TYPE_PREFERRED)
> +					fixed_mode = drm_mode_duplicate(connector->dev,
> +									scan);
> +			}
> +
> +			if (fixed_mode)
> +				intel_panel_init(&intel_connector->panel,
> +						 fixed_mode, NULL);
> +		}

How are we supposed to get rid of a stale fixed_mode when some other
display gets plugged in?

Also what would happen if the preferred mode can't be supported due to
some source limitation?

In general I'm not entirely happy with having this kind of policy in
the kernel. I'd much prefer if we could get crtc size and border properties
done so that userspace could set up the scaling any which way it chooses.

> +
>  		if (ret)
>  			return ret;
>  	}
> @@ -4688,15 +4705,14 @@ intel_dp_set_property(struct drm_connector *connector,
>  		goto done;
>  	}
>  
> -	if (is_edp(intel_dp) &&
> -	    property == connector->dev->mode_config.scaling_mode_property) {
> -		if (val == DRM_MODE_SCALE_NONE) {
> -			DRM_DEBUG_KMS("no scaling not supported\n");
> +	if (property == connector->dev->mode_config.scaling_mode_property) {
> +		if (is_edp(intel_dp) && val == DRM_MODE_SCALE_NONE) {
> +			DRM_DEBUG_KMS("eDP: no scaling not supported\n");
>  			return -EINVAL;
>  		}
>  
>  		if (intel_connector->panel.fitting_mode == val) {
> -			/* the eDP scaling property is not changed */
> +			/* the connector scaling property is not changed */
>  			return 0;
>  		}
>  		intel_connector->panel.fitting_mode = val;
> @@ -4989,13 +5005,22 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
>  	intel_attach_broadcast_rgb_property(connector);
>  	intel_dp->color_range_auto = true;
>  
> -	if (is_edp(intel_dp)) {
> +	/* Each pipe has panel filter since Ironlake. */
> +	if (INTEL_INFO(connector->dev)->gen >= 5) {
>  		drm_mode_create_scaling_mode_property(connector->dev);
> -		drm_object_attach_property(
> -			&connector->base,
> -			connector->dev->mode_config.scaling_mode_property,
> -			DRM_MODE_SCALE_ASPECT);
> -		intel_connector->panel.fitting_mode = DRM_MODE_SCALE_ASPECT;
> +		if (is_edp(intel_dp)) {
> +			drm_object_attach_property(
> +				&connector->base,
> +				connector->dev->mode_config.scaling_mode_property,
> +				DRM_MODE_SCALE_ASPECT);
> +			intel_connector->panel.fitting_mode = DRM_MODE_SCALE_ASPECT;
> +		} else {
> +			drm_object_attach_property(
> +				&connector->base,
> +				connector->dev->mode_config.scaling_mode_property,
> +				DRM_MODE_SCALE_NONE);
> +			intel_connector->panel.fitting_mode = DRM_MODE_SCALE_NONE;
> +		}
>  	}
>  }
>  
> -- 
> 1.8.2.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/4] drm/i915: Adding Panel Filter function for DP

[Zhang, Xiong Y]

> On Mon, Aug 10, 2015 at 03:26:09PM +0800, Xiong Zhang wrote:
> > Only internal eDP, LVDS, DVI screen could set scalling mode, some
> > customers need to set scalling mode for external DP, HDMI, VGA screen.
> > Let's fulfill this.
> >
> > bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90989
> >
> > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 63
> > ++++++++++++++++++++++++++++-------------
> >  1 file changed, 44 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c index f1b9f93..2da334b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -207,7 +207,13 @@ intel_dp_mode_valid(struct drm_connector
> *connector,
> >  	int target_clock = mode->clock;
> >  	int max_rate, mode_rate, max_lanes, max_link_clock;
> >
> > -	if (is_edp(intel_dp) && fixed_mode) {
> > +	if (mode->clock < 10000)
> > +		return MODE_CLOCK_LOW;
> > +
> > +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > +		return MODE_H_ILLEGAL;
> > +
> > +	if (!intel_panel_scale_none(&intel_connector->panel)) {
> >  		if (mode->hdisplay > fixed_mode->hdisplay)
> >  			return MODE_PANEL;
> >
> > @@ -226,12 +232,6 @@ intel_dp_mode_valid(struct drm_connector
> *connector,
> >  	if (mode_rate > max_rate)
> >  		return MODE_CLOCK_HIGH;
> >
> > -	if (mode->clock < 10000)
> > -		return MODE_CLOCK_LOW;
> > -
> > -	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > -		return MODE_H_ILLEGAL;
> > -
> >  	return MODE_OK;
> >  }
> >
> > @@ -1378,7 +1378,7 @@ intel_dp_compute_config(struct intel_encoder
> *encoder,
> >  	pipe_config->has_drrs = false;
> >  	pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
> >
> > -	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> > +	if (!intel_panel_scale_none(&intel_connector->panel)) {
> >  		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
> >  				       adjusted_mode);
> >
> > @@ -4592,6 +4592,23 @@ static int intel_dp_get_modes(struct
> drm_connector *connector)
> >  	edid = intel_connector->detect_edid;
> >  	if (edid) {
> >  		int ret = intel_connector_update_modes(connector, edid);
> > +
> > +		if (ret && intel_connector->panel.fixed_mode == NULL) {
> > +			/* init fixed mode as preferred mode for DP */
> > +			struct drm_display_mode *fixed_mode = NULL;
> > +			struct drm_display_mode *scan;
> > +
> > +			list_for_each_entry(scan, &connector->probed_modes, head) {
> > +				if (scan->type & DRM_MODE_TYPE_PREFERRED)
> > +					fixed_mode = drm_mode_duplicate(connector->dev,
> > +									scan);
> > +			}
> > +
> > +			if (fixed_mode)
> > +				intel_panel_init(&intel_connector->panel,
> > +						 fixed_mode, NULL);
> > +		}
> 
> How are we supposed to get rid of a stale fixed_mode when some other
> display gets plugged in?
[Zhang, Xiong Y] Thanks so much for your good question. Yes, we should clear the
stale fitting_mode and fixed_mode when display is disconnect in 
intel_dp_hpd_pulse() function.
> 
> Also what would happen if the preferred mode can't be supported due to some
> source limitation?
[Zhang, Xiong Y] In this case, which mode should be selected as fixed_mode ?
As you said maybe kernel isn't the right place to do such decision.
> 
> In general I'm not entirely happy with having this kind of policy in the kernel.
> I'd much prefer if we could get crtc size and border properties done so that
> userspace could set up the scaling any which way it chooses.
[Zhang, Xiong Y] Could you give more detail about your preference ?

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

Re: [PATCH 2/4] drm/i915: Adding Panel Filter function for DP

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 05:12:57AM +0000, Zhang, Xiong Y wrote:
> > On Mon, Aug 10, 2015 at 03:26:09PM +0800, Xiong Zhang wrote:
> > > Only internal eDP, LVDS, DVI screen could set scalling mode, some
> > > customers need to set scalling mode for external DP, HDMI, VGA screen.
> > > Let's fulfill this.
> > >
> > > bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90989
> > >
> > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 63
> > > ++++++++++++++++++++++++++++-------------
> > >  1 file changed, 44 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c index f1b9f93..2da334b 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -207,7 +207,13 @@ intel_dp_mode_valid(struct drm_connector
> > *connector,
> > >  	int target_clock = mode->clock;
> > >  	int max_rate, mode_rate, max_lanes, max_link_clock;
> > >
> > > -	if (is_edp(intel_dp) && fixed_mode) {
> > > +	if (mode->clock < 10000)
> > > +		return MODE_CLOCK_LOW;
> > > +
> > > +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > +		return MODE_H_ILLEGAL;
> > > +
> > > +	if (!intel_panel_scale_none(&intel_connector->panel)) {
> > >  		if (mode->hdisplay > fixed_mode->hdisplay)
> > >  			return MODE_PANEL;
> > >
> > > @@ -226,12 +232,6 @@ intel_dp_mode_valid(struct drm_connector
> > *connector,
> > >  	if (mode_rate > max_rate)
> > >  		return MODE_CLOCK_HIGH;
> > >
> > > -	if (mode->clock < 10000)
> > > -		return MODE_CLOCK_LOW;
> > > -
> > > -	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > -		return MODE_H_ILLEGAL;
> > > -
> > >  	return MODE_OK;
> > >  }
> > >
> > > @@ -1378,7 +1378,7 @@ intel_dp_compute_config(struct intel_encoder
> > *encoder,
> > >  	pipe_config->has_drrs = false;
> > >  	pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
> > >
> > > -	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> > > +	if (!intel_panel_scale_none(&intel_connector->panel)) {
> > >  		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
> > >  				       adjusted_mode);
> > >
> > > @@ -4592,6 +4592,23 @@ static int intel_dp_get_modes(struct
> > drm_connector *connector)
> > >  	edid = intel_connector->detect_edid;
> > >  	if (edid) {
> > >  		int ret = intel_connector_update_modes(connector, edid);
> > > +
> > > +		if (ret && intel_connector->panel.fixed_mode == NULL) {
> > > +			/* init fixed mode as preferred mode for DP */
> > > +			struct drm_display_mode *fixed_mode = NULL;
> > > +			struct drm_display_mode *scan;
> > > +
> > > +			list_for_each_entry(scan, &connector->probed_modes, head) {
> > > +				if (scan->type & DRM_MODE_TYPE_PREFERRED)
> > > +					fixed_mode = drm_mode_duplicate(connector->dev,
> > > +									scan);
> > > +			}
> > > +
> > > +			if (fixed_mode)
> > > +				intel_panel_init(&intel_connector->panel,
> > > +						 fixed_mode, NULL);
> > > +		}
> > 
> > How are we supposed to get rid of a stale fixed_mode when some other
> > display gets plugged in?
> [Zhang, Xiong Y] Thanks so much for your good question. Yes, we should clear the
> stale fitting_mode and fixed_mode when display is disconnect in 
> intel_dp_hpd_pulse() function.
> > 
> > Also what would happen if the preferred mode can't be supported due to some
> > source limitation?
> [Zhang, Xiong Y] In this case, which mode should be selected as fixed_mode ?

At the very least we should make sure it's a mode we can use.

> As you said maybe kernel isn't the right place to do such decision.

There are a lot of options how we could pick the mode. Eg. might want to
pick the next largest mode, and if there is none try to pick the largest
smaller mode (since pfit can't downscale by much). Also should we try to
pick an intelaced mode if the user requested one etc. Lots of open
questions how this policy should be handled. Would be easier to punt it
all to userspace, which would also avoid the kernel policy doing the
wrong thing when userspace knows what it wants.

> > 
> > In general I'm not entirely happy with having this kind of policy in the kernel.
> > I'd much prefer if we could get crtc size and border properties done so that
> > userspace could set up the scaling any which way it chooses.
> [Zhang, Xiong Y] Could you give more detail about your preference ?

The idea would be to expose some sort of crtc size properties that would
provide pipe_src_{w,h}, and the mode would provide the actual display
timings as it does today. And if we add some kind of border properties
to control the output size of the pfit (since the mode doesn't have that
information) userspace could do whatever it pleased.

I think the last attempt just sort of died out during review:
http://lists.freedesktop.org/archives/intel-gfx/2014-August/050732.html

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

Re: [PATCH 2/4] drm/i915: Adding Panel Filter function for DP

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 07:28:44PM +0300, Ville Syrjälä wrote:
> On Fri, Aug 14, 2015 at 05:12:57AM +0000, Zhang, Xiong Y wrote:
> > > On Mon, Aug 10, 2015 at 03:26:09PM +0800, Xiong Zhang wrote:
> > > > Only internal eDP, LVDS, DVI screen could set scalling mode, some
> > > > customers need to set scalling mode for external DP, HDMI, VGA screen.
> > > > Let's fulfill this.
> > > >
> > > > bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90989
> > > >
> > > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 63
> > > > ++++++++++++++++++++++++++++-------------
> > > >  1 file changed, 44 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c index f1b9f93..2da334b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -207,7 +207,13 @@ intel_dp_mode_valid(struct drm_connector
> > > *connector,
> > > >  	int target_clock = mode->clock;
> > > >  	int max_rate, mode_rate, max_lanes, max_link_clock;
> > > >
> > > > -	if (is_edp(intel_dp) && fixed_mode) {
> > > > +	if (mode->clock < 10000)
> > > > +		return MODE_CLOCK_LOW;
> > > > +
> > > > +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > > +		return MODE_H_ILLEGAL;
> > > > +
> > > > +	if (!intel_panel_scale_none(&intel_connector->panel)) {
> > > >  		if (mode->hdisplay > fixed_mode->hdisplay)
> > > >  			return MODE_PANEL;
> > > >
> > > > @@ -226,12 +232,6 @@ intel_dp_mode_valid(struct drm_connector
> > > *connector,
> > > >  	if (mode_rate > max_rate)
> > > >  		return MODE_CLOCK_HIGH;
> > > >
> > > > -	if (mode->clock < 10000)
> > > > -		return MODE_CLOCK_LOW;
> > > > -
> > > > -	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > > -		return MODE_H_ILLEGAL;
> > > > -
> > > >  	return MODE_OK;
> > > >  }
> > > >
> > > > @@ -1378,7 +1378,7 @@ intel_dp_compute_config(struct intel_encoder
> > > *encoder,
> > > >  	pipe_config->has_drrs = false;
> > > >  	pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
> > > >
> > > > -	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> > > > +	if (!intel_panel_scale_none(&intel_connector->panel)) {
> > > >  		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
> > > >  				       adjusted_mode);
> > > >
> > > > @@ -4592,6 +4592,23 @@ static int intel_dp_get_modes(struct
> > > drm_connector *connector)
> > > >  	edid = intel_connector->detect_edid;
> > > >  	if (edid) {
> > > >  		int ret = intel_connector_update_modes(connector, edid);
> > > > +
> > > > +		if (ret && intel_connector->panel.fixed_mode == NULL) {
> > > > +			/* init fixed mode as preferred mode for DP */
> > > > +			struct drm_display_mode *fixed_mode = NULL;
> > > > +			struct drm_display_mode *scan;
> > > > +
> > > > +			list_for_each_entry(scan, &connector->probed_modes, head) {
> > > > +				if (scan->type & DRM_MODE_TYPE_PREFERRED)
> > > > +					fixed_mode = drm_mode_duplicate(connector->dev,
> > > > +									scan);
> > > > +			}
> > > > +
> > > > +			if (fixed_mode)
> > > > +				intel_panel_init(&intel_connector->panel,
> > > > +						 fixed_mode, NULL);
> > > > +		}
> > > 
> > > How are we supposed to get rid of a stale fixed_mode when some other
> > > display gets plugged in?
> > [Zhang, Xiong Y] Thanks so much for your good question. Yes, we should clear the
> > stale fitting_mode and fixed_mode when display is disconnect in 
> > intel_dp_hpd_pulse() function.
> > > 
> > > Also what would happen if the preferred mode can't be supported due to some
> > > source limitation?
> > [Zhang, Xiong Y] In this case, which mode should be selected as fixed_mode ?
> 
> At the very least we should make sure it's a mode we can use.
> 
> > As you said maybe kernel isn't the right place to do such decision.
> 
> There are a lot of options how we could pick the mode. Eg. might want to
> pick the next largest mode, and if there is none try to pick the largest
> smaller mode (since pfit can't downscale by much). Also should we try to
> pick an intelaced mode if the user requested one etc. Lots of open
> questions how this policy should be handled. Would be easier to punt it
> all to userspace, which would also avoid the kernel policy doing the
> wrong thing when userspace knows what it wants.
> 
> > > 
> > > In general I'm not entirely happy with having this kind of policy in the kernel.
> > > I'd much prefer if we could get crtc size and border properties done so that
> > > userspace could set up the scaling any which way it chooses.
> > [Zhang, Xiong Y] Could you give more detail about your preference ?
> 
> The idea would be to expose some sort of crtc size properties that would
> provide pipe_src_{w,h}, and the mode would provide the actual display
> timings as it does today. And if we add some kind of border properties
> to control the output size of the pfit (since the mode doesn't have that
> information) userspace could do whatever it pleased.
> 
> I think the last attempt just sort of died out during review:
> http://lists.freedesktop.org/archives/intel-gfx/2014-August/050732.html

It occurs to me that we might actually go about this in a way that's
more like what you did. That is, we could expose the fixed mode as a
separate property from the user mode. That way the user could choose any
mode as the fixed mode, keeping the policy out from the kernel, and it
would be more in line with that we already do on LVDS/eDP. Obviously
we'd still need to add new border properties to give userspace full
freedom in setting up the pfit.

Another thing we could do with this approach is expose the pipe PF-ID
mode when the fixed mode is interlaced and the user mode is progressive.
And if both are interlaced, or there's just an interlaced used mode w/o
a fixed mode, we'd keep using IF-ID like we do today.

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

Re: [PATCH 2/4] drm/i915: Adding Panel Filter function for DP

[Zhang, Xiong Y]

> On Fri, Aug 14, 2015 at 07:28:44PM +0300, Ville Syrjälä wrote:
> > On Fri, Aug 14, 2015 at 05:12:57AM +0000, Zhang, Xiong Y wrote:
> > > > On Mon, Aug 10, 2015 at 03:26:09PM +0800, Xiong Zhang wrote:
> > > > > Only internal eDP, LVDS, DVI screen could set scalling mode, some
> > > > > customers need to set scalling mode for external DP, HDMI, VGA screen.
> > > > > Let's fulfill this.
> > > > >
> > > > > bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90989
> > > > >
> > > > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dp.c | 63
> > > > > ++++++++++++++++++++++++++++-------------
> > > > >  1 file changed, 44 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c index f1b9f93..2da334b 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -207,7 +207,13 @@ intel_dp_mode_valid(struct drm_connector
> > > > *connector,
> > > > >  	int target_clock = mode->clock;
> > > > >  	int max_rate, mode_rate, max_lanes, max_link_clock;
> > > > >
> > > > > -	if (is_edp(intel_dp) && fixed_mode) {
> > > > > +	if (mode->clock < 10000)
> > > > > +		return MODE_CLOCK_LOW;
> > > > > +
> > > > > +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > > > +		return MODE_H_ILLEGAL;
> > > > > +
> > > > > +	if (!intel_panel_scale_none(&intel_connector->panel)) {
> > > > >  		if (mode->hdisplay > fixed_mode->hdisplay)
> > > > >  			return MODE_PANEL;
> > > > >
> > > > > @@ -226,12 +232,6 @@ intel_dp_mode_valid(struct drm_connector
> > > > *connector,
> > > > >  	if (mode_rate > max_rate)
> > > > >  		return MODE_CLOCK_HIGH;
> > > > >
> > > > > -	if (mode->clock < 10000)
> > > > > -		return MODE_CLOCK_LOW;
> > > > > -
> > > > > -	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > > > -		return MODE_H_ILLEGAL;
> > > > > -
> > > > >  	return MODE_OK;
> > > > >  }
> > > > >
> > > > > @@ -1378,7 +1378,7 @@ intel_dp_compute_config(struct
> intel_encoder
> > > > *encoder,
> > > > >  	pipe_config->has_drrs = false;
> > > > >  	pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
> > > > >
> > > > > -	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> > > > > +	if (!intel_panel_scale_none(&intel_connector->panel)) {
> > > > >  		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
> > > > >  				       adjusted_mode);
> > > > >
> > > > > @@ -4592,6 +4592,23 @@ static int intel_dp_get_modes(struct
> > > > drm_connector *connector)
> > > > >  	edid = intel_connector->detect_edid;
> > > > >  	if (edid) {
> > > > >  		int ret = intel_connector_update_modes(connector, edid);
> > > > > +
> > > > > +		if (ret && intel_connector->panel.fixed_mode == NULL) {
> > > > > +			/* init fixed mode as preferred mode for DP */
> > > > > +			struct drm_display_mode *fixed_mode = NULL;
> > > > > +			struct drm_display_mode *scan;
> > > > > +
> > > > > +			list_for_each_entry(scan, &connector->probed_modes,
> head) {
> > > > > +				if (scan->type & DRM_MODE_TYPE_PREFERRED)
> > > > > +					fixed_mode =
> drm_mode_duplicate(connector->dev,
> > > > > +									scan);
> > > > > +			}
> > > > > +
> > > > > +			if (fixed_mode)
> > > > > +				intel_panel_init(&intel_connector->panel,
> > > > > +						 fixed_mode, NULL);
> > > > > +		}
> > > >
> > > > How are we supposed to get rid of a stale fixed_mode when some other
> > > > display gets plugged in?
> > > [Zhang, Xiong Y] Thanks so much for your good question. Yes, we should
> clear the
> > > stale fitting_mode and fixed_mode when display is disconnect in
> > > intel_dp_hpd_pulse() function.
> > > >
> > > > Also what would happen if the preferred mode can't be supported due to
> some
> > > > source limitation?
> > > [Zhang, Xiong Y] In this case, which mode should be selected as
> fixed_mode ?
> >
> > At the very least we should make sure it's a mode we can use.
> >
> > > As you said maybe kernel isn't the right place to do such decision.
> >
> > There are a lot of options how we could pick the mode. Eg. might want to
> > pick the next largest mode, and if there is none try to pick the largest
> > smaller mode (since pfit can't downscale by much). Also should we try to
> > pick an intelaced mode if the user requested one etc. Lots of open
> > questions how this policy should be handled. Would be easier to punt it
> > all to userspace, which would also avoid the kernel policy doing the
> > wrong thing when userspace knows what it wants.
> >
> > > >
> > > > In general I'm not entirely happy with having this kind of policy in the
> kernel.
> > > > I'd much prefer if we could get crtc size and border properties done so
> that
> > > > userspace could set up the scaling any which way it chooses.
> > > [Zhang, Xiong Y] Could you give more detail about your preference ?
> >
> > The idea would be to expose some sort of crtc size properties that would
> > provide pipe_src_{w,h}, and the mode would provide the actual display
> > timings as it does today. And if we add some kind of border properties
> > to control the output size of the pfit (since the mode doesn't have that
> > information) userspace could do whatever it pleased.
> >
> > I think the last attempt just sort of died out during review:
> > http://lists.freedesktop.org/archives/intel-gfx/2014-August/050732.html
> 
> It occurs to me that we might actually go about this in a way that's
> more like what you did. That is, we could expose the fixed mode as a
> separate property from the user mode. That way the user could choose any
> mode as the fixed mode, keeping the policy out from the kernel, and it
> would be more in line with that we already do on LVDS/eDP. 
> we'd still need to add new border properties to give userspace full
> freedom in setting up the pfit.
[Zhang, Xiong Y] Does border properties mean Center, Aspect ratio or Full ?
This sounds an available solution and let the user do the policy.
In this way, user will set two properties (fixed mode and border) to enable
Panel filter. It's a little inconvenient.
I'm afraid common user doesn't know how to choose the fixed mode and
What mean for fixed mode,  Common user only know the 
"Center" "Tile" like in windows.
> 
> Another thing we could do with this approach is expose the pipe PF-ID
> mode when the fixed mode is interlaced and the user mode is progressive.
> And if both are interlaced, or there's just an interlaced used mode w/o
> a fixed mode, we'd keep using IF-ID like we do today.
[Zhang, Xiong Y] I checked the B spec, there isn't PF-ID / IF-ID control bit in 
PIPE_CONF since BDW.
I never see a monitor supporting interlaced mode. Does a monitor could support
both Interlaced and progressive mode ?
> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Adding Panel Filter function for DP

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 09:11:11AM +0000, Zhang, Xiong Y wrote:
> > On Fri, Aug 14, 2015 at 07:28:44PM +0300, Ville Syrjälä wrote:
> > > On Fri, Aug 14, 2015 at 05:12:57AM +0000, Zhang, Xiong Y wrote:
> > > > > On Mon, Aug 10, 2015 at 03:26:09PM +0800, Xiong Zhang wrote:
> > > > > > Only internal eDP, LVDS, DVI screen could set scalling mode, some
> > > > > > customers need to set scalling mode for external DP, HDMI, VGA screen.
> > > > > > Let's fulfill this.
> > > > > >
> > > > > > bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90989
> > > > > >
> > > > > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_dp.c | 63
> > > > > > ++++++++++++++++++++++++++++-------------
> > > > > >  1 file changed, 44 insertions(+), 19 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > b/drivers/gpu/drm/i915/intel_dp.c index f1b9f93..2da334b 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -207,7 +207,13 @@ intel_dp_mode_valid(struct drm_connector
> > > > > *connector,
> > > > > >  	int target_clock = mode->clock;
> > > > > >  	int max_rate, mode_rate, max_lanes, max_link_clock;
> > > > > >
> > > > > > -	if (is_edp(intel_dp) && fixed_mode) {
> > > > > > +	if (mode->clock < 10000)
> > > > > > +		return MODE_CLOCK_LOW;
> > > > > > +
> > > > > > +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > > > > +		return MODE_H_ILLEGAL;
> > > > > > +
> > > > > > +	if (!intel_panel_scale_none(&intel_connector->panel)) {
> > > > > >  		if (mode->hdisplay > fixed_mode->hdisplay)
> > > > > >  			return MODE_PANEL;
> > > > > >
> > > > > > @@ -226,12 +232,6 @@ intel_dp_mode_valid(struct drm_connector
> > > > > *connector,
> > > > > >  	if (mode_rate > max_rate)
> > > > > >  		return MODE_CLOCK_HIGH;
> > > > > >
> > > > > > -	if (mode->clock < 10000)
> > > > > > -		return MODE_CLOCK_LOW;
> > > > > > -
> > > > > > -	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > > > > -		return MODE_H_ILLEGAL;
> > > > > > -
> > > > > >  	return MODE_OK;
> > > > > >  }
> > > > > >
> > > > > > @@ -1378,7 +1378,7 @@ intel_dp_compute_config(struct
> > intel_encoder
> > > > > *encoder,
> > > > > >  	pipe_config->has_drrs = false;
> > > > > >  	pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
> > > > > >
> > > > > > -	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> > > > > > +	if (!intel_panel_scale_none(&intel_connector->panel)) {
> > > > > >  		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
> > > > > >  				       adjusted_mode);
> > > > > >
> > > > > > @@ -4592,6 +4592,23 @@ static int intel_dp_get_modes(struct
> > > > > drm_connector *connector)
> > > > > >  	edid = intel_connector->detect_edid;
> > > > > >  	if (edid) {
> > > > > >  		int ret = intel_connector_update_modes(connector, edid);
> > > > > > +
> > > > > > +		if (ret && intel_connector->panel.fixed_mode == NULL) {
> > > > > > +			/* init fixed mode as preferred mode for DP */
> > > > > > +			struct drm_display_mode *fixed_mode = NULL;
> > > > > > +			struct drm_display_mode *scan;
> > > > > > +
> > > > > > +			list_for_each_entry(scan, &connector->probed_modes,
> > head) {
> > > > > > +				if (scan->type & DRM_MODE_TYPE_PREFERRED)
> > > > > > +					fixed_mode =
> > drm_mode_duplicate(connector->dev,
> > > > > > +									scan);
> > > > > > +			}
> > > > > > +
> > > > > > +			if (fixed_mode)
> > > > > > +				intel_panel_init(&intel_connector->panel,
> > > > > > +						 fixed_mode, NULL);
> > > > > > +		}
> > > > >
> > > > > How are we supposed to get rid of a stale fixed_mode when some other
> > > > > display gets plugged in?
> > > > [Zhang, Xiong Y] Thanks so much for your good question. Yes, we should
> > clear the
> > > > stale fitting_mode and fixed_mode when display is disconnect in
> > > > intel_dp_hpd_pulse() function.
> > > > >
> > > > > Also what would happen if the preferred mode can't be supported due to
> > some
> > > > > source limitation?
> > > > [Zhang, Xiong Y] In this case, which mode should be selected as
> > fixed_mode ?
> > >
> > > At the very least we should make sure it's a mode we can use.
> > >
> > > > As you said maybe kernel isn't the right place to do such decision.
> > >
> > > There are a lot of options how we could pick the mode. Eg. might want to
> > > pick the next largest mode, and if there is none try to pick the largest
> > > smaller mode (since pfit can't downscale by much). Also should we try to
> > > pick an intelaced mode if the user requested one etc. Lots of open
> > > questions how this policy should be handled. Would be easier to punt it
> > > all to userspace, which would also avoid the kernel policy doing the
> > > wrong thing when userspace knows what it wants.
> > >
> > > > >
> > > > > In general I'm not entirely happy with having this kind of policy in the
> > kernel.
> > > > > I'd much prefer if we could get crtc size and border properties done so
> > that
> > > > > userspace could set up the scaling any which way it chooses.
> > > > [Zhang, Xiong Y] Could you give more detail about your preference ?
> > >
> > > The idea would be to expose some sort of crtc size properties that would
> > > provide pipe_src_{w,h}, and the mode would provide the actual display
> > > timings as it does today. And if we add some kind of border properties
> > > to control the output size of the pfit (since the mode doesn't have that
> > > information) userspace could do whatever it pleased.
> > >
> > > I think the last attempt just sort of died out during review:
> > > http://lists.freedesktop.org/archives/intel-gfx/2014-August/050732.html
> > 
> > It occurs to me that we might actually go about this in a way that's
> > more like what you did. That is, we could expose the fixed mode as a
> > separate property from the user mode. That way the user could choose any
> > mode as the fixed mode, keeping the policy out from the kernel, and it
> > would be more in line with that we already do on LVDS/eDP. 
> > we'd still need to add new border properties to give userspace full
> > freedom in setting up the pfit.
> [Zhang, Xiong Y] Does border properties mean Center, Aspect ratio or Full ?

No, the border properties would be something like "border
left/right/top/bottom" in pixels. So userspace could explicitly control
the position and size of the pfit output on the screen.

> This sounds an available solution and let the user do the policy.
> In this way, user will set two properties (fixed mode and border) to enable
> Panel filter. It's a little inconvenient.
> I'm afraid common user doesn't know how to choose the fixed mode and
> What mean for fixed mode,  Common user only know the 
> "Center" "Tile" like in windows.

IMO that's a problem for userspace tools to solve. The kernel just
provides the mecanism, and userspace can do with it what it wants.

> > 
> > Another thing we could do with this approach is expose the pipe PF-ID
> > mode when the fixed mode is interlaced and the user mode is progressive.
> > And if both are interlaced, or there's just an interlaced used mode w/o
> > a fixed mode, we'd keep using IF-ID like we do today.
> [Zhang, Xiong Y] I checked the B spec, there isn't PF-ID / IF-ID control bit in 
> PIPE_CONF since BDW.

I don't see any restriction on it? Are you looking at the right
register (TRANSCONF bits 22:21)? I only see some SKL+ Y-tiling and
90/270 rotation restrictions.

> I never see a monitor supporting interlaced mode. Does a monitor could support
> both Interlaced and progressive mode ?

I would expect practically every TV to have a few of them. With computer
monitors I guess they don't bother anymore.

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

Re: [PATCH 2/4] drm/i915: Adding Panel Filter function for DP

[Simon Farnsworth]

On Wednesday 19 August 2015 09:11:11 Zhang, Xiong Y wrote:
> > On Fri, Aug 14, 2015 at 07:28:44PM +0300, Ville Syrjälä wrote:
> > Another thing we could do with this approach is expose the pipe PF-ID
> > mode when the fixed mode is interlaced and the user mode is progressive.
> > And if both are interlaced, or there's just an interlaced used mode w/o
> > a fixed mode, we'd keep using IF-ID like we do today.
> [Zhang, Xiong Y] I checked the B spec, there isn't PF-ID / IF-ID control bit in 
> PIPE_CONF since BDW.
> I never see a monitor supporting interlaced mode. Does a monitor could support
> both Interlaced and progressive mode ?

Most of the computer monitors I see support interlaced modes on HDMI; I
usually see CEA-861 VIC 5 (1920x1080i60) and VIC 20 (1920x1080i50) supported
on HDMI by computer monitors.
-- 
Simon Farnsworth
Software Engineer
ONELAN Ltd
http://www.onelan.com

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

[PATCH 3/4] drm/i915: Adding panel filter for HDMI

[Xiong Zhang]

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |  2 +
 drivers/gpu/drm/i915/intel_hdmi.c | 79 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 76 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f57a0b4..0a4632b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -672,6 +672,8 @@ struct intel_hdmi {
 			       bool enable,
 			       struct drm_display_mode *adjusted_mode);
 	bool (*infoframe_enabled)(struct drm_encoder *encoder);
+
+	struct intel_connector *attached_connector;
 };
 
 struct intel_dp_mst_encoder;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 70bad5b..6f1638c 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1191,6 +1191,8 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
 {
 	struct intel_hdmi *hdmi = intel_attached_hdmi(connector);
 	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
 	enum drm_mode_status status;
 	int clock;
 
@@ -1201,6 +1203,18 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
 	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
 		clock *= 2;
 
+	if (!intel_panel_scale_none(&intel_connector->panel)) {
+		if (mode->hdisplay > fixed_mode->hdisplay)
+			return MODE_PANEL;
+
+		if (mode->vdisplay > fixed_mode->vdisplay)
+			return MODE_PANEL;
+
+		clock = fixed_mode->clock;
+		if (fixed_mode->flags & DRM_MODE_FLAG_DBLCLK)
+			clock *= 2;
+	}
+
 	/* check if we can do 8bpc */
 	status = hdmi_port_clock_valid(hdmi, clock, true);
 
@@ -1249,8 +1263,9 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
-	int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
-	int clock_12bpc = clock_8bpc * 3 / 2;
+	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
+	struct intel_connector *intel_connector = intel_hdmi->attached_connector;
+	int clock_8bpc, clock_12bpc;
 	int desired_bpp;
 
 	pipe_config->has_hdmi_sink = intel_hdmi->has_hdmi_sink;
@@ -1258,6 +1273,18 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 	if (pipe_config->has_hdmi_sink)
 		pipe_config->has_infoframe = true;
 
+	if (!intel_panel_scale_none(&intel_connector->panel)) {
+		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
+				       adjusted_mode);
+
+		if (!HAS_PCH_SPLIT(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 (intel_hdmi->color_range_auto) {
 		/* See CEA-861-E - 5.1 Default Encoding Parameters */
 		if (pipe_config->has_hdmi_sink &&
@@ -1267,6 +1294,8 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 			intel_hdmi->color_range = 0;
 	}
 
+	clock_8bpc = adjusted_mode->crtc_clock;
+	clock_12bpc = clock_8bpc * 3 / 2;
 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) {
 		pipe_config->pixel_multiplier = 2;
 		clock_8bpc *= 2;
@@ -1410,13 +1439,30 @@ intel_hdmi_force(struct drm_connector *connector)
 
 static int intel_hdmi_get_modes(struct drm_connector *connector)
 {
-	struct edid *edid;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct edid *edid = intel_connector->detect_edid;
+	int ret;
 
-	edid = to_intel_connector(connector)->detect_edid;
 	if (edid == NULL)
 		return 0;
 
-	return intel_connector_update_modes(connector, edid);
+	ret = intel_connector_update_modes(connector, edid);
+	if (ret && intel_connector->panel.fixed_mode == NULL) {
+		struct drm_display_mode *fixed_mode = NULL;
+		struct drm_display_mode *scan;
+
+		list_for_each_entry(scan, &connector->probed_modes, head) {
+			if (scan->type & DRM_MODE_TYPE_PREFERRED)
+				fixed_mode = drm_mode_duplicate(connector->dev,
+								scan);
+		}
+
+		if (fixed_mode)
+			intel_panel_init(&intel_connector->panel,
+					 fixed_mode, NULL);
+	}
+
+	return ret;
 }
 
 static bool
@@ -1441,6 +1487,7 @@ intel_hdmi_set_property(struct drm_connector *connector,
 	struct intel_digital_port *intel_dig_port =
 		hdmi_to_dig_port(intel_hdmi);
 	struct drm_i915_private *dev_priv = connector->dev->dev_private;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
 	int ret;
 
 	ret = drm_object_property_set_value(&connector->base, property, val);
@@ -1512,6 +1559,15 @@ intel_hdmi_set_property(struct drm_connector *connector,
 		goto done;
 	}
 
+	if (property == connector->dev->mode_config.scaling_mode_property) {
+		if (intel_connector->panel.fitting_mode == val)
+			return 0;
+
+		intel_connector->panel.fitting_mode = val;
+
+		goto done;
+	}
+
 	return -EINVAL;
 
 done:
@@ -1942,11 +1998,22 @@ intel_attach_aspect_ratio_property(struct drm_connector *connector)
 static void
 intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *connector)
 {
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+
 	intel_attach_force_audio_property(connector);
 	intel_attach_broadcast_rgb_property(connector);
 	intel_hdmi->color_range_auto = true;
 	intel_attach_aspect_ratio_property(connector);
 	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
+
+	/* Each pipe has panel filter since Ironlake. */
+	if (INTEL_INFO(connector->dev)->gen >= 5) {
+		drm_mode_create_scaling_mode_property(connector->dev);
+		drm_object_attach_property(&connector->base,
+				connector->dev->mode_config.scaling_mode_property,
+				DRM_MODE_SCALE_NONE);
+		intel_connector->panel.fitting_mode = DRM_MODE_SCALE_NONE;
+	}
 }
 
 void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
@@ -1959,6 +2026,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum port port = intel_dig_port->port;
 
+	intel_hdmi->attached_connector = intel_connector;
+
 	drm_connector_init(dev, connector, &intel_hdmi_connector_funcs,
 			   DRM_MODE_CONNECTOR_HDMIA);
 	drm_connector_helper_add(connector, &intel_hdmi_connector_helper_funcs);
-- 
1.8.2.1

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

[PATCH 4/4] drm/i915: Adding panel filter for VGA

[Xiong Zhang]

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c | 81 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 521af2c..f8eedfc 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -288,24 +288,37 @@ intel_crt_mode_valid(struct drm_connector *connector,
 		     struct drm_display_mode *mode)
 {
 	struct drm_device *dev = connector->dev;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
+	int max_clock = 0, clock;
 
-	int max_clock = 0;
 	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
 		return MODE_NO_DBLESCAN;
 
-	if (mode->clock < 25000)
+	clock = mode->clock;
+	if (!intel_panel_scale_none(&intel_connector->panel)) {
+		if (mode->hdisplay > fixed_mode->hdisplay)
+			return MODE_PANEL;
+
+		if (mode->vdisplay > fixed_mode->vdisplay)
+			return MODE_PANEL;
+
+		clock = fixed_mode->clock;
+	}
+
+	if (clock < 25000)
 		return MODE_CLOCK_LOW;
 
 	if (IS_GEN2(dev))
 		max_clock = 350000;
 	else
 		max_clock = 400000;
-	if (mode->clock > max_clock)
+	if (clock > max_clock)
 		return MODE_CLOCK_HIGH;
 
 	/* The FDI receiver on LPT only supports 8bpc and only has 2 lanes. */
 	if (HAS_PCH_LPT(dev) &&
-	    (ironlake_get_lanes_required(mode->clock, 270000, 24) > 2))
+	    (ironlake_get_lanes_required(clock, 270000, 24) > 2))
 		return MODE_CLOCK_HIGH;
 
 	return MODE_OK;
@@ -315,6 +328,22 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder,
 				     struct intel_crtc_state *pipe_config)
 {
 	struct drm_device *dev = encoder->base.dev;
+	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
+	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
+	struct intel_crt *intel_crt = intel_encoder_to_crt(encoder);
+	struct intel_connector *intel_connector = intel_crt->connector;
+
+	if (!intel_panel_scale_none(&intel_connector->panel)) {
+		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
+				       adjusted_mode);
+
+		if (!HAS_PCH_SPLIT(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 (HAS_PCH_SPLIT(dev))
 		pipe_config->has_pch_encoder = true;
@@ -489,6 +518,7 @@ static struct edid *intel_crt_get_edid(struct drm_connector *connector,
 static int intel_crt_ddc_get_modes(struct drm_connector *connector,
 				struct i2c_adapter *adapter)
 {
+	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct edid *edid;
 	int ret;
 
@@ -499,6 +529,20 @@ static int intel_crt_ddc_get_modes(struct drm_connector *connector,
 	ret = intel_connector_update_modes(connector, edid);
 	kfree(edid);
 
+	if (ret && intel_connector->panel.fixed_mode == NULL) {
+		struct drm_display_mode *fixed_mode = NULL, *scan;
+
+		list_for_each_entry(scan, &connector->probed_modes, head) {
+			if (scan->type & DRM_MODE_TYPE_PREFERRED)
+				fixed_mode = drm_mode_duplicate(connector->dev,
+								scan);
+		}
+
+		if (fixed_mode)
+			intel_panel_init(&intel_connector->panel,
+					 fixed_mode, NULL);
+	}
+
 	return ret;
 }
 
@@ -768,6 +812,18 @@ static int intel_crt_set_property(struct drm_connector *connector,
 				  struct drm_property *property,
 				  uint64_t value)
 {
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+
+	if (property == connector->dev->mode_config.scaling_mode_property) {
+		if (intel_connector->panel.fitting_mode == value)
+			return 0;
+
+		intel_connector->panel.fitting_mode = value;
+
+		if (connector->encoder && connector->encoder->crtc)
+			intel_crtc_restore_mode(connector->encoder->crtc);
+	}
+
 	return 0;
 }
 
@@ -844,6 +900,21 @@ static const struct dmi_system_id intel_no_crt[] = {
 	{ }
 };
 
+static void
+intel_crt_add_properties(struct drm_connector *connector)
+{
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+
+	/* Each pipe has pannel filter since Ironlake. */
+	if (INTEL_INFO(connector->dev)->gen >= 5) {
+		drm_mode_create_scaling_mode_property(connector->dev);
+		drm_object_attach_property(&connector->base,
+				connector->dev->mode_config.scaling_mode_property,
+				DRM_MODE_SCALE_NONE);
+		intel_connector->panel.fitting_mode = DRM_MODE_SCALE_NONE;
+	}
+}
+
 void intel_crt_init(struct drm_device *dev)
 {
 	struct drm_connector *connector;
@@ -870,6 +941,8 @@ void intel_crt_init(struct drm_device *dev)
 	drm_connector_init(dev, &intel_connector->base,
 			   &intel_crt_connector_funcs, DRM_MODE_CONNECTOR_VGA);
 
+	intel_crt_add_properties(connector);
+
 	drm_encoder_init(dev, &crt->base.base, &intel_crt_enc_funcs,
 			 DRM_MODE_ENCODER_DAC);
 
-- 
1.8.2.1

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

[PATCH 1/4] drm/i915: Adding intel_panel_scale_none() helper function

[Xiong Zhang]

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h   |  1 +
 drivers/gpu/drm/i915/intel_panel.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 47cef0e..f57a0b4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1287,6 +1287,7 @@ int intel_panel_init(struct intel_panel *panel,
 void intel_panel_fini(struct intel_panel *panel);
 void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
 			    struct drm_display_mode *adjusted_mode);
+bool intel_panel_scale_none(struct intel_panel *panel);
 void intel_pch_panel_fitting(struct intel_crtc *crtc,
 			     struct intel_crtc_state *pipe_config,
 			     int fitting_mode);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index e2ab3f6..4a573ac 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -46,6 +46,16 @@ intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
 	drm_mode_set_crtcinfo(adjusted_mode, 0);
 }
 
+bool
+intel_panel_scale_none(struct intel_panel *panel)
+{
+	if (panel->fitting_mode == DRM_MODE_SCALE_NONE ||
+	    panel->fixed_mode == NULL)
+		return true;
+	else
+		return false;
+}
+
 /**
  * intel_find_panel_downclock - find the reduced downclock for LVDS in EDID
  * @dev: drm device
-- 
1.8.2.1

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

Re: [PATCH 1/4] drm/i915: Adding intel_panel_scale_none() helper function

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 2520 bytes --]

I believe this function could be added along with the next patch that is
the first to use it...
Or it would be good to have a good commit message explaining why this
function is needed and what is be used for...

more bikeshedings inline:

On Mon, Aug 10, 2015 at 12:39 AM Xiong Zhang <xiong.y.zhang@intel.com>
wrote:

> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>  drivers/gpu/drm/i915/intel_panel.c | 10 ++++++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 47cef0e..f57a0b4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1287,6 +1287,7 @@ int intel_panel_init(struct intel_panel *panel,
>  void intel_panel_fini(struct intel_panel *panel);
>  void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
>                             struct drm_display_mode *adjusted_mode);
> +bool intel_panel_scale_none(struct intel_panel *panel);
>  void intel_pch_panel_fitting(struct intel_crtc *crtc,
>                              struct intel_crtc_state *pipe_config,
>                              int fitting_mode);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c
> b/drivers/gpu/drm/i915/intel_panel.c
> index e2ab3f6..4a573ac 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -46,6 +46,16 @@ intel_fixed_panel_mode(const struct drm_display_mode
> *fixed_mode,
>         drm_mode_set_crtcinfo(adjusted_mode, 0);
>  }
>
> +bool
> +intel_panel_scale_none(struct intel_panel *panel)
>

double negations always confuses me, when reading next patches it took few
seconds to realize on next patch that !scale_none was == fixed_mode...
but meh, I never have good suggestions to avoid double negations... so up
to you...


> +{
> +       if (panel->fitting_mode == DRM_MODE_SCALE_NONE ||
> +           panel->fixed_mode == NULL)
> +               return true;
> +       else
> +               return false;
>

this could be just return (panel->fitting_mode == DRM_MODE_SCALE_NONE ||
panel->fixed_mode == NULL)
or !<statement> if you remove the double negation...


> +}
> +
>  /**
>   * intel_find_panel_downclock - find the reduced downclock for LVDS in
> EDID
>   * @dev: drm device
> --
> 1.8.2.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 3624 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/4] drm/i915: Adding intel_panel_scale_none() helper function

[Daniel Vetter]

On Mon, Aug 10, 2015 at 06:23:19PM +0000, Rodrigo Vivi wrote:
> I believe this function could be added along with the next patch that is
> the first to use it...
> Or it would be good to have a good commit message explaining why this
> function is needed and what is be used for...

Yes, please don't split up patches so much that you end up adding dead
code or dead structures - always try to have at least a minimal user for
something.

If you want to split things up really tiny then go the other way round:
First add the callers, then add the implementation. That way reviewers can
understand the big scope first and then dig into details. But this one
here really is small enough to just be squashed in.
-Daniel

> 
> more bikeshedings inline:
> 
> On Mon, Aug 10, 2015 at 12:39 AM Xiong Zhang <xiong.y.zhang@intel.com>
> wrote:
> 
> > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> >  drivers/gpu/drm/i915/intel_panel.c | 10 ++++++++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 47cef0e..f57a0b4 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1287,6 +1287,7 @@ int intel_panel_init(struct intel_panel *panel,
> >  void intel_panel_fini(struct intel_panel *panel);
> >  void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
> >                             struct drm_display_mode *adjusted_mode);
> > +bool intel_panel_scale_none(struct intel_panel *panel);
> >  void intel_pch_panel_fitting(struct intel_crtc *crtc,
> >                              struct intel_crtc_state *pipe_config,
> >                              int fitting_mode);
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c
> > b/drivers/gpu/drm/i915/intel_panel.c
> > index e2ab3f6..4a573ac 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -46,6 +46,16 @@ intel_fixed_panel_mode(const struct drm_display_mode
> > *fixed_mode,
> >         drm_mode_set_crtcinfo(adjusted_mode, 0);
> >  }
> >
> > +bool
> > +intel_panel_scale_none(struct intel_panel *panel)
> >
> 
> double negations always confuses me, when reading next patches it took few
> seconds to realize on next patch that !scale_none was == fixed_mode...
> but meh, I never have good suggestions to avoid double negations... so up
> to you...
> 
> 
> > +{
> > +       if (panel->fitting_mode == DRM_MODE_SCALE_NONE ||
> > +           panel->fixed_mode == NULL)
> > +               return true;
> > +       else
> > +               return false;
> >
> 
> this could be just return (panel->fitting_mode == DRM_MODE_SCALE_NONE ||
> panel->fixed_mode == NULL)
> or !<statement> if you remove the double negation...
> 
> 
> > +}
> > +
> >  /**
> >   * intel_find_panel_downclock - find the reduced downclock for LVDS in
> > EDID
> >   * @dev: drm device
> > --
> > 1.8.2.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >

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


-- 
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: [PATCH 1/4] drm/i915: Adding intel_panel_scale_none() helper function

[Zhang, Xiong Y]

> On Mon, Aug 10, 2015 at 06:23:19PM +0000, Rodrigo Vivi wrote:
> > I believe this function could be added along with the next patch that
> > is the first to use it...
> > Or it would be good to have a good commit message explaining why this
> > function is needed and what is be used for...
> 
> Yes, please don't split up patches so much that you end up adding dead code
> or dead structures - always try to have at least a minimal user for something.
> 
> If you want to split things up really tiny then go the other way round:
> First add the callers, then add the implementation. That way reviewers can
> understand the big scope first and then dig into details. But this one here really
> is small enough to just be squashed in.
> -Daniel
> 
[Zhang, Xiong Y] Thanks for teaching me how to handle this. I'll follow it in the next version.
thanks
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Adding intel_panel_scale_none() helper function

[Timo Aaltonen]

On 14.08.2015 08:18, Zhang, Xiong Y wrote:
>> On Mon, Aug 10, 2015 at 06:23:19PM +0000, Rodrigo Vivi wrote:
>>> I believe this function could be added along with the next patch that
>>> is the first to use it...
>>> Or it would be good to have a good commit message explaining why this
>>> function is needed and what is be used for...
>>
>> Yes, please don't split up patches so much that you end up adding dead code
>> or dead structures - always try to have at least a minimal user for something.
>>
>> If you want to split things up really tiny then go the other way round:
>> First add the callers, then add the implementation. That way reviewers can
>> understand the big scope first and then dig into details. But this one here really
>> is small enough to just be squashed in.
>> -Daniel
>>
> [Zhang, Xiong Y] Thanks for teaching me how to handle this. I'll follow it in the next version.
> thanks

Was there a second version that got submitted? I can't find it if so.


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