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