Re: [PATCH v4 3/3] drm/i915: New drm crtc property for varying the size of borders

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Apr 11, 2014 at 08:34:08AM +0530, Akash Goel wrote:
> On Thu, 2014-04-10 at 14:34 +0300, Ville Syrjälä wrote:
> > On Thu, Apr 10, 2014 at 01:13:56PM +0530, Akash Goel wrote:
> > > On Tue, 2014-04-08 at 19:28 +0300, Ville Syrjälä wrote:
> > > > On Wed, Mar 26, 2014 at 09:25:12AM +0530, akash.goel@xxxxxxxxx wrote:
> > > > > From: Akash Goel <akash.goel@xxxxxxxxx>
> > > > > 
> > > > > This patch adds a new drm crtc property for varying the size of
> > > > > the horizontal & vertical borers of the output/display window.
> > > > > This will control the output of Panel fitter.
> > > > > 
> > > > > v2: Added a new check for the invalid border size input
> > > > > 
> > > > > v3: Fixed bugs in output window calculation
> > > > > Removed superfluous checks
> > > > > 
> > > > > v4: Added the capability to forecfully enable the Panel fitter.
> > > > > The property value is of 64 bits, first 32 bits are used for
> > > > > border dimensions. The 33rd bit can be used to forcefully
> > > > > enable the panel fitter. This is useful for Panels which
> > > > > do not override the User specified Pipe timings.
> > > > > 
> > > > > Testcase: igt/kms_panel_fitter_test
> > > > > 
> > > > > Signed-off-by: Akash Goel <akash.goel@xxxxxxxxx>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.h      |   7 ++
> > > > >  drivers/gpu/drm/i915/intel_display.c |  39 +++++++-
> > > > >  drivers/gpu/drm/i915/intel_drv.h     |   5 +
> > > > >  drivers/gpu/drm/i915/intel_panel.c   | 176 ++++++++++++++++++++++++++++++++---
> > > > >  4 files changed, 211 insertions(+), 16 deletions(-)
> > > > > 
> > <snip>
> > > > > @@ -42,6 +57,60 @@ intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
> > > > >  	drm_mode_set_crtcinfo(adjusted_mode, 0);
> > > > >  }
> > > > >  
> > > > > +void
> > > > > +intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> > > > > +			struct intel_crtc_config *pipe_config)
> > > > > +{
> > > > > +	struct drm_display_mode *adjusted_mode;
> > > > > +	int x, y;
> > > > > +	u32 pf_horizontal_ratio, pf_vertical_ratio;
> > > > > +	u32 tot_width, tot_height;
> > > > > +	u32 src_width, src_height; /* pipesrc.x, pipesrc.y */
> > > > > +	u32 dst_width, dst_height;
> > > > > +
> > > > > +	adjusted_mode = &pipe_config->adjusted_mode;
> > > > > +
> > > > > +	src_width = pipe_config->pipe_src_w;
> > > > > +	src_height = pipe_config->pipe_src_h;
> > > > > +
> > > > > +	tot_width  = adjusted_mode->hdisplay;
> > > > > +	tot_height = adjusted_mode->vdisplay;
> > > > > +
> > > > > +	/*
> > > > > +	 * Having non zero borders will reduce the size of 'HACTIVE/VACTIVE'
> > > > > +	 * region. So (HACTIVE - Left border - Right Border) *
> > > > > +	 * (VACTIVE  - Top Border  - Bottom border) will effectively be the
> > > > > +	 * output rectangle on screen
> > > > > +	 */
> > > > > +	dst_width = tot_width -
> > > > > +			(((intel_crtc->border_size >> 16) & 0xffff) * 2);
> > > > > +	dst_height = tot_height -
> > > > > +			((intel_crtc->border_size & 0xffff) * 2);
> > > > 
> > > > I'm thinking that we should allow the user to specify each border width
> > > > individually rather than just assume left==right and top==bottom.
> > > > 
> > > 
> > > Sorry I thought that the Top/Bottom & left/Right borders would be
> > > symmetric only.
> > 
> > I don't see a reason to limit ourselves here.
> > 
> 
> Fine, will extend this property to set each border separately. 
> Can I use the 12 bits for each border value, as that shall be
> sufficient.

Maybe just add separate properties for each border value. We already
have similar properties for TV outputs.

> 
> > > Tried setting the borders on EDP & HDMI panels by manipulating the Pipe
> > > timings (through the logic used in 'centre_horizontally' &
> > > 'centre_vertically' functions), but it didn't work.
> > > Is this logic effective for the LVDS panel only ?
> > 
> > Could be. Certainly the border enable bit is there only for LVDS. The
> > gmch panel fitter isn't very flexible so it's possible we can't
> > actually make it do many of the things the pch pfit can do.
> > 
> 
> Yes the GMCH panel fitter function is not equally capable as the PCH
> counterpart. Here except the LVDS panel, it seems that borders cannot be
> realized on any other panel, just via the manipulation of Pipe timings
> (the way it can be done in PCH one).
> For the same reason the 'Center' Panel fitting mode of "scaling mode"
> property is not working on VLV  (at least for HDMI/EDP panels).
> 
> > What happens if we set up the pfit to use manual scaling ratios but
> > configure both scaling ratios so that scaled image won't fill the
> > active video region in either direction? Does it position the scaled
> > image at coordinates 0,0 and simply scan black the rest of the time after
> > it's run out of source pixel data? Or does it automagically center the
> > image and scan black on both sides? Or does it fail in some way?
> > 
> 
> Already tried that, but in vain. As per the VLV spec, the support for
> Manual scaling ratio mode itself has been de-featured, so it didn't work
> at all. So Auto/LetterBox/PillarBox modes are being supported.

I see.

> 
> As a next step tried to manipulate the Pipe timings, so as to produce
> the borders on 4 sides of the panel. Similar to the PCH panel fitting
> logic, kept the HSYNC/VSYNC pulse width same as well as the size of
> HBLANK/VBLANK intervals, just manipulated the HYSNC/VSYNC and
> HBLANK/VBLANK start & end points to create borders around the active
> region. 
> 
> Tried to add Left/Right borders of 32 columns and Top/bottom borders of
> 30 lines. 
> For that
> HACTIVE=1920,  HBLANK START=1920, HSYNC START=1968, HYSNC END=2000,
> HBLANK END=2080, HTOTAL=2080. 
> was changed to 
> HACTIVE=1856,    HBLANK START=1888, HSYNC START=1952, HYSNC END=1984,
> HBLANK END=2048, HTOTAL=2080. 
> 
> And Similarly 
> VACTIVE=1200, VBLANK START=1200, VSYNC START=1203, VYSNC END=1209,
> VBLANK END=1235, VTOTAL=1235. 
> was changed to
> VACTIVE=1140, VBLANK START=1170, VSYNC START=1185, VYSNC END=1191,
> VBLANK END=1205, VTOTAL=1235. 

Your sync values seem to be a bit off. AFAICS they should be:
 hsync_start = 1936, hsync_end = 1968
 vsync_start = 1173, vsync_end = 1179

> 
> After this manipulation, saw that the HMDI panel turned blank and showed
> a "No Signal" message.

Might be good to repeat with fixed sync positions.

There's also some kind of border enable bit in the sdvo/hdmi control
register. No idea if that does anything useful.

> But for the same experiment, observed a different behavior with EDP
> panel, that the display was active but the image was being shown in the
> Top left part of the screen only, with the area outside active rectangle
> having all junk data.

:(

I suppose it's still likely that things won't actually work even w/ the
sync pulses in the correct position. And that would mean we can't support
borders on non-LVDS outputs on gmch platforms.

And that would also mean we already have a bug with the current
scaling_mode property on VLV eDP.

> 
> > > 
> > > > > +
> > > > > +	if ((dst_width == 0) || (dst_height == 0)) {
> > > > > +		DRM_ERROR("Invalid border size input\n");
> > > > > +		return;
> > > > 
> > > > This is clear sign here that we should do all this stuff in the compute
> > > > config stage so that we can fail gracefully and tell userspace that things
> > > > didn't work out.
> > > > 
> > > 
> > > Actually this call to decide the panel fitter config, is being made in
> > > the context of 'compute config' only. But it's originating from the
> > > 'crtc set property' & not from the modeset.
> > 
> > We need to check things in both cases, and return an error if things
> > can't work out.
> > 
> 
> Can this be done in a next patch set i.e. adding return types to the functions
> so that if there is an error in panel fitter configuration, it can be communicated
> back to User space.

I don't see a reason for delaying it.

> 
> > > Do you mean to say that if done from the 'modeset' call, we can report
> > > back the error to User space for an invalid border setting.
> > > Actually currently the return type is 'void' only for the 2 existing
> > > panel fitter config functions. 
> > > Also we don't have a check in place for the max supported upscaling
> > > ratio.
> > 
> > Is there an upscaling limit? I know there's a downscaling limit of IIRC
> > 1.125x or something close to that. I don't think we check that either.
> > 
> Sorry my mistake, it's only the downscaling ratio which has a max value
> of 1.125 i.e. PIPESRC cannot be downscaled by more than a factor of
> 1.125.
> 
> > > 
> > > > > +	}
> > > > > +
> > > > > +	pf_horizontal_ratio = panel_fitter_scaling(src_width, dst_width);
> > > > > +	pf_vertical_ratio   = panel_fitter_scaling(src_height, dst_height);
> > > > > +
> > > > > +	if (pf_horizontal_ratio > MAX_DOWNSCALE_RATIO) {
> > > > > +		DRM_ERROR("width is too small\n");
> > > > > +		return;
> > > > > +	} else if (pf_vertical_ratio > MAX_DOWNSCALE_RATIO) {
> > > > > +		DRM_ERROR("height is too small\n");
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	x = (adjusted_mode->hdisplay - dst_width + 1)/2;
> > > > > +	y = (adjusted_mode->vdisplay - dst_height + 1)/2;
> > > > > +
> > > > > +	pipe_config->pch_pfit.pos = (x << 16) | y;
> > > > > +	pipe_config->pch_pfit.size = (dst_width << 16) | dst_height;
> > > > > +	pipe_config->pch_pfit.enabled = pipe_config->pch_pfit.size != 0;
> > > > > +}
> > > > > +
> > > > >  /* adjusted_mode has been preset to be the panel's fixed mode */
> > > > >  void
> > > > >  intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
> > > > > @@ -55,6 +124,13 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
> > > > >  
> > > > >  	x = y = width = height = 0;
> > > > >  
> > > > > +	/* check if User wants to apply the borders, or wants to forcefully
> > > > > +	   enable the panel fitter, otherwise fall through the regular path */
> > > > > +	if (intel_crtc->pfit_enabled ||
> > > > > +			intel_crtc->border_size)
> > > > > +		return intel_pch_manual_panel_fitting(intel_crtc,
> > > > > +						      pipe_config);
> > > > > +
> > > > >  	/* Native modes don't need fitting */
> > > > >  	if (adjusted_mode->hdisplay == pipe_config->pipe_src_w &&
> > > > >  	    adjusted_mode->vdisplay == pipe_config->pipe_src_h)
> > > > > @@ -157,19 +233,6 @@ centre_vertically(struct drm_display_mode *mode,
> > > > >  	mode->crtc_vsync_end = mode->crtc_vsync_start + sync_width;
> > > > >  }
> > > > >  
> > > > > -static inline u32 panel_fitter_scaling(u32 source, u32 target)
> > > > > -{
> > > > > -	/*
> > > > > -	 * Floating point operation is not supported. So the FACTOR
> > > > > -	 * is defined, which can avoid the floating point computation
> > > > > -	 * when calculating the panel ratio.
> > > > > -	 */
> > > > > -#define ACCURACY 12
> > > > > -#define FACTOR (1 << ACCURACY)
> > > > > -	u32 ratio = source * FACTOR / target;
> > > > > -	return (FACTOR * ratio + FACTOR/2) / FACTOR;
> > > > > -}
> > > > > -
> > > > >  static void i965_scale_aspect(struct intel_crtc_config *pipe_config,
> > > > >  			      u32 *pfit_control)
> > > > >  {
> > > > > @@ -247,6 +310,86 @@ static void i9xx_scale_aspect(struct intel_crtc_config *pipe_config,
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> > > > > +				     struct intel_crtc_config *pipe_config)
> > > > > +{
> > > > > +	struct drm_device *dev = intel_crtc->base.dev;
> > > > > +	u32 pfit_control = 0, border = 0;
> > > > > +	u32 pf_horizontal_ratio, pf_vertical_ratio;
> > > > > +	struct drm_display_mode *adjusted_mode;
> > > > > +	u32 tot_width, tot_height;
> > > > > +	u32 src_width, src_height; /* pipesrc.x, pipesrc.y */
> > > > > +	u32 dst_width, dst_height;
> > > > > +
> > > > > +	adjusted_mode = &pipe_config->adjusted_mode;
> > > > > +
> > > > > +	src_width = pipe_config->pipe_src_w;
> > > > > +	src_height = pipe_config->pipe_src_h;
> > > > > +
> > > > > +	tot_width = adjusted_mode->hdisplay;
> > > > > +	tot_height = adjusted_mode->vdisplay;
> > > > > +
> > > > > +	/*
> > > > > +	 * Having non zero borders will reduce the size of 'HACTIVE/VACTIVE'
> > > > > +	 * region. So (HACTIVE - Left border - Right Border) *
> > > > > +	 * (VACTIVE  - Top Border  - Bottom border) will effectively be the
> > > > > +	 * output rectangle on screen
> > > > > +	 */
> > > > > +	dst_width = tot_width -
> > > > > +			(((intel_crtc->border_size >> 16) & 0xffff) * 2);
> > > > > +	dst_height = tot_height -
> > > > > +			((intel_crtc->border_size & 0xffff) * 2);
> > > > > +
> > > > > +	if ((dst_width == 0) || (dst_height == 0)) {
> > > > > +		DRM_ERROR("Invalid border size input\n");
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	pf_horizontal_ratio = panel_fitter_scaling(src_width, dst_width);
> > > > > +	pf_vertical_ratio   = panel_fitter_scaling(src_height, dst_height);
> > > > > +
> > > > > +	if (pf_horizontal_ratio > MAX_DOWNSCALE_RATIO) {
> > > > > +		DRM_ERROR("width is too small\n");
> > > > > +		return;
> > > > > +	} else if (pf_vertical_ratio > MAX_DOWNSCALE_RATIO) {
> > > > > +		DRM_ERROR("height is too small\n");
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	if (dst_width != tot_width)
> > > > > +		centre_horizontally(adjusted_mode, dst_width);
> > > > > +	if (dst_height != tot_height)
> > > > > +		centre_vertically(adjusted_mode, dst_height);
> > > > > +
> > > > > +	/* No scaling needed now, but still enable the panel fitter,
> > > > > +	   as that will allow the User to subequently do the dynamic
> > > > > +	   flipping of fbs of different resolutions */
> > > > > +	if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
> > > > > +	    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h) {
> > > > > +		BUG_ON(!intel_crtc->pfit_enabled);
> > > > > +		DRM_DEBUG_KMS("Forcefully enabling the Panel fitter\n");
> > > > > +	}
> > > > > +
> > > > > +	border = LVDS_BORDER_ENABLE;
> > > > > +
> > > > > +	if (INTEL_INFO(dev)->gen >= 4) {
> > > > > +		/* PFIT_SCALING_PROGRAMMED is de-featured on BYT */
> > > > > +		pfit_control |= PFIT_ENABLE | PFIT_SCALING_AUTO;
> > > > > +		pfit_control |= ((intel_crtc->pipe << PFIT_PIPE_SHIFT) | PFIT_FILTER_FUZZY);
> > > > > +	} else {
> > > > > +		pfit_control |= (PFIT_ENABLE |
> > > > > +				 VERT_AUTO_SCALE | HORIZ_AUTO_SCALE |
> > > > > +				 VERT_INTERP_BILINEAR | HORIZ_INTERP_BILINEAR);
> > > > > +	}
> > > > > +
> > > > > +	/* Make sure pre-965 set dither correctly for 18bpp panels. */
> > > > > +	if (INTEL_INFO(dev)->gen < 4 && pipe_config->pipe_bpp == 18)
> > > > > +		pfit_control |= PANEL_8TO6_DITHER_ENABLE;
> > > > > +
> > > > > +	pipe_config->gmch_pfit.control = pfit_control;
> > > > > +	pipe_config->gmch_pfit.lvds_border_bits = border;
> > > > > +}
> > > > > +
> > > > >  void intel_gmch_panel_fitting(struct intel_crtc *intel_crtc,
> > > > >  			      struct intel_crtc_config *pipe_config,
> > > > >  			      int fitting_mode)
> > > > > @@ -257,6 +400,13 @@ void intel_gmch_panel_fitting(struct intel_crtc *intel_crtc,
> > > > >  
> > > > >  	adjusted_mode = &pipe_config->adjusted_mode;
> > > > >  
> > > > > +	/* check if User wants to apply the borders, or wants to forcefully
> > > > > +	   enable the panel fitter, otherwise fall through the regular path */
> > > > > +	if (intel_crtc->pfit_enabled ||
> > > > > +			intel_crtc->border_size)
> > > > > +		return intel_gmch_manual_panel_fitting(intel_crtc,
> > > > > +						       pipe_config);
> > > > > +
> > > > >  	/* Native modes don't need fitting */
> > > > >  	if (adjusted_mode->hdisplay == pipe_config->pipe_src_w &&
> > > > >  	    adjusted_mode->vdisplay == pipe_config->pipe_src_h)
> > > > > -- 
> > > > > 1.8.5.2
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > 
> > 
> 

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





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux