[Intel-gfx] [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 at 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