[Intel-gfx] [PATCH v4 2/4] drm/i915: New drm crtc property for varying the Pipe Src size

[Ville Syrjälä]

On Sun, May 04, 2014 at 03:51:51PM +0530, Akash Goel wrote:
> On Tue, 2014-04-29 at 17:06 +0300, Ville Syrjälä wrote:
> > On Sun, Apr 20, 2014 at 04:14:18PM +0530, akash.goel at intel.com wrote:
> > > From: Akash Goel <akash.goel at intel.com>
> > > 
> > > This patch adds a new drm crtc property for varying the Pipe Src size
> > > or the Panel fitter input size. Pipe Src controls the size that is
> > > scaled from.
> > > This will allow to dynamically flip (without modeset) the frame buffers
> > > of different resolutions
> > > 
> > > v2: Added a check to fail the set property call if Panel fitter is
> > > disabled & new PIPESRC programming do not match with PIPE timings.
> > > Removed the pitch mismatch check on frame buffer, when being flipped.
> > > This is currently done only for VLV/HSW.
> > > 
> > > v3: Modified the check, added in v2, to consider the platforms having
> > > Split PCH.
> > > 
> > > v4: Refactored based on latest codebase.
> > > Used 'UINT_MAX' macro in place of constant.
> > > 
> > > Testcase: igt/kms_panel_fitter_test
> > > 
> > > Signed-off-by: Akash Goel <akash.goel at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      |  6 +++
> > >  drivers/gpu/drm/i915/intel_display.c | 76 +++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 80 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 7d6acb4..104a232 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1441,6 +1441,12 @@ struct drm_i915_private {
> > >  	struct drm_property *broadcast_rgb_property;
> > >  	struct drm_property *force_audio_property;
> > >  
> > > +	/*
> > > +	 * Property to dynamically vary the size of the
> > > +	 * PIPESRC or Panel fitter input size
> > > +	 */
> > > +	struct drm_property *input_size_property;
> > > +
> > >  	uint32_t hw_context_size;
> > >  	struct list_head context_list;
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index f26be4e..5484ae2 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -8902,8 +8902,18 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> > >  	 * Note that pitch changes could also affect these register.
> > >  	 */
> > >  	if (INTEL_INFO(dev)->gen > 3 &&
> > > -	    (fb->offsets[0] != crtc->primary->fb->offsets[0] ||
> > > -	     fb->pitches[0] != crtc->primary->fb->pitches[0]))
> > > +	    (fb->offsets[0] != crtc->primary->fb->offsets[0]))
> > > +		return -EINVAL;
> > > +
> > > +	/*
> > > +	 * Bypassing the fb pitch check for VLV/HSW, as purportedly there
> > > +	 * is a dynamic flip support in VLV/HSW. This will allow to
> > > +	 * flip fbs of different resolutions without doing a modeset.
> > > +	 * TBD, confirm the same for other newer gen platforms also.
> > > +	 */
> > > +	if (INTEL_INFO(dev)->gen > 3 &&
> > > +	    !IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) &&
> > > +	    (fb->pitches[0] != crtc->primary->fb->pitches[0]))
> > 
> > NAK. Please read the comment above the check to understand why we can't
> > just change the stride here. I guess on HSW+ it might be possible since
> > it uses the x/y offsets even with linear FBs so stride changes don't
> > affect the offset. Also such changes sould be in separate patches
> > anyway.
> > 
> 
> Actually as per the Bspec, probably the pitch/stride can be changed for
> synchronous flips. 
> As per the description of MI_DISPLAY_FLIP from the Bspec
> "15:6 Display Buffer Pitch
> For Synchronous Flips and Stereo 3D Flips only, this field specifies the
> 64-byte aligned pitch of the new display buffer. For Asynchronous Flips,
> this parameter is programmed so that all the flips in a flip chain
> should maintain the same pitch as programmed with the last synchronous
> flip or stereo 3D flip or direct through MMIO.
> The Display Buffer Pitch and Tile parameter and Decrypt Request fields
> cannot be changed for asynchronous flips (i.e., the new buffer must have
> the same pitch/tile format as the previous buffer).
> Asynch flips are Supported on X-Tiled Frame buffers only."
> 
> > I guess one option would be update the linear offset with LRI, but
> > since the offset register gets latched independently of the DSPSURF
> > write the changes aren't guaranteed to happen atomically. Also on
> > VLV LRI to display registers is apparently busted.
> > 
> > If you do find a way to make this work on some platforms, then we
> > need to have a test for it to make sure it does the right thing.
> > 
> 
> Actually we have been using MMIO based flips on BYT, instead of command
> streamer based flips, so the DSPSTRIDE & DSPTILEOFF registers are being
> written along with DSPSURF on every flip. 
> As per the description of DSPSTRIDE register in Bspec, 
> "This register is updated either through a command packet passed through
> the command stream or writes to this register. When it is desired to
> update both this and the start register, 
> the stride register must be written first because the write to the start
> register is the trigger that causes the update of both registers on the
> next VBLANK event"
> So once we switch to MMIO flip, then it shall be taken care.
> 
> 
> > >  		return -EINVAL;
> > >  
> > >  	if (i915_terminally_wedged(&dev_priv->gpu_error))
> > > @@ -10422,11 +10432,71 @@ out_config:
> > >  	return ret;
> > >  }
> > >  
> > > +static void intel_create_crtc_properties(struct drm_crtc *crtc)
> > > +{
> > > +	struct drm_device *dev = crtc->dev;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +
> > > +	if (!dev_priv->input_size_property)
> > > +		dev_priv->input_size_property =
> > > +			drm_property_create_range(dev, 0, "input size",
> > > +						  0, UINT_MAX);
> > 
> > I think people would be happier with separate width/height properties
> > instead of sticking both into one property.
> > 
> > Also this sounds like a generally useful property for hardware any which
> > has some kind of full crtc scaler. So maybe it should be in drm core.
> > Also what ever happended to the property documentation effort. Did the
> > patch get in, or was it rejected, or is it in limbo?
> > 
> Fine will split this into 2 properties but will there be any limitation
> in future if a single property is persisted with to specify both height
> & width here.
> 
> Sagar has submitted the patch for property documentation and he got the
> ACK also from the Laurent Pinchart, but I think Daniel has not done the
> same. Please can you kindly once check with Daniel for this.
> 
> > > +
> > > +	if (dev_priv->input_size_property)
> > > +		drm_object_attach_property(&intel_crtc->base.base,
> > > +					   dev_priv->input_size_property,
> > > +					   0);
> > > +}
> > > +
> > >  static int intel_crtc_set_property(struct drm_crtc *crtc,
> > >  		struct drm_property *property, uint64_t val)
> > >  {
> > > +	struct drm_device *dev = crtc->dev;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >  	int ret = -ENOENT;
> > >  
> > > +	if (property == dev_priv->input_size_property) {
> > > +		int new_width = (int)((val >> 16) & 0xffff);
> > > +		int new_height = (int)(val & 0xffff);
> > > +		const struct drm_display_mode *adjusted_mode =
> > > +				&intel_crtc->config.adjusted_mode;
> > > +
> > > +		if ((new_width == intel_crtc->config.pipe_src_w) &&
> > > +		    (new_height == intel_crtc->config.pipe_src_h))
> > > +			return 0;
> > > +
> > > +		if (((adjusted_mode->crtc_hdisplay != new_width) ||
> > > +		     (adjusted_mode->crtc_vdisplay != new_height)) &&
> > > +		    ((HAS_PCH_SPLIT(dev) &&
> > > +			(!intel_crtc->config.pch_pfit.enabled)) ||
> > > +		     (!HAS_PCH_SPLIT(dev) &&
> > > +			(!intel_crtc->config.gmch_pfit.control)))) {
> > > +			DRM_ERROR("PIPESRC mismatch with Pipe timings &"
> > > +				  "PF is disabled\n");
> > > +			return -EINVAL;
> > 
> > This raises an interesting question. If we can't toggle the pfit on/off
> > dynamically should we have some way to force the pfit on at modeset
> > time (even w/ 1:1 scaling) so that it's possibly to change the scaling
> > dynamically w/o a full modeset.
> > 
> > One idea might be to treat 0 as a special value for this property. If
> > both width and height are 0 we do the old thing and disable pfit if
> > the scaling based on user mode vs. adjusted mode is 1:1, but if this
> > property has anything but 0, always enable pfit. Thoughts anyone?
> > 
> 
> Yes in order to be able to dynamically change the resolution of fb's
> being flipped (without a modeset) first Panel fitter has to be enabled.
> 
> So 0 value of this property will considered as disabled and old
> condition of enabling Panel fitter will be followed(requested mode !=
> adjusted mode). 
> But if this property is set to some value, and if Panel fitter is not
> enabled already, it shall be enabled (which will require a modeset).
> Can we do it like this when user sets a non zero value for this
> property.
> 1. Check first if Panel fitter is enabled or not, and if not then enable
> it forcefully bypassing the original condition.
> 2. Update the PIPESRC register later on, with the new values, in the
> subsequent Page flip ioctl (and not in the set property call itself),
> when FB of the new resolution will be passed.

No we need to update everything immediately. For doing things the nice
way we're going to need the atomic modeset ioctl, but for now we need
to comply with the restrictions of the old ioctls. We should not add
any kind of limited special purpose atomic update doohickeys in the
meantime.

>  
> > > +		}
> > > +
> > > +		intel_crtc->config.pipe_src_w = new_width;
> > > +		intel_crtc->config.pipe_src_h = new_height;
> > 
> > This looks like it needs more error checks to make sure the scaling
> > ratio stays within acceptable limits.
> > 
> > Also we need to recheck primary plane FB size vs. new PIPESRC.
> > 
> On setting this property,  FB's being subsequently flipped on primary
> plane shall comply with the new Pipe SRC values.
> And check for this, is already done by the drm_crtc_check_viewport
> function in page flip ioctl.

As stated we need to update everything immediately.

> 
> > And we need to reclip sprites and cursors.
> 
> > 
> > This is starting to be quite a lot of stuff. So for the initial
> > implementation may be it's easier to just do a full modeset here. And
> > once that's done we can figure out how to avoid the modeset. We should
> > anyway be moving towards a world where we compute the new config,
> > validate it, and then figure out if we can get there w/o a full modeset.
> > So maybe we need to start writing the code to just that and not limit
> > ourselves to implementing special code for specific properties.
> > 
> 
> Sorry couldn't fully comprehend your last point here.

The point is that we don't want to duplicate the code that does all this
checking, reclipping etc. So ideally our modeset path should look a bit
like this:

compute_config();
if (can't do it)
	return error;
if (need_full_modeset()) {
	modeset();
} else {
	nuclear_flip();
	// ie. reconfigure planes, reprogram pipesrc, etc.
}

We already have something along those lines for turning a setcrtc ioctl
into just a mmio flip instead of a full modeset. But this should be
extended to handle all kinds of interesting stuff.

Just doing a full modeset for changes in the property value is a reasonable
first step IMO. Adding a short circuit codepath to avoid the full modeset
can be done as a followup patch if needed. But going for the full nuclear
flip thing would seem a better goal than spending time on polishing such
special purpose codepaths. But of course if the code can be written in a
way that contributes to the nuclear flip goal then I see no downsides to
it.

> 
> > > +
> > > +		intel_crtc->config.requested_mode.hdisplay = new_width;
> > > +		intel_crtc->config.requested_mode.vdisplay = new_height;
> > > +
> > > +		crtc->mode.hdisplay = new_width;
> > > +		crtc->mode.vdisplay = new_height;
> > 
> > Hmm. This will cause problems if we have to do a modeset w/o a new user
> > specified mode (eg. atomic modeset where change on another pipe requires
> > us to perform modeset on this pipe as well).
> > 
> > So I think we need to leave the modes alone. One thing to worry about is
> > old userspace that doesn't realize that changes in this property would
> > have ramifications on the crtc viewport. I'm not sure there's anything
> > sane we can do but let old userspace shoot itself in the foot if it plays
> > with this property.
> > 
> 
> Again sorry, but couldn't get your old user space point.
> Should the 'hdisplay', 'vdisplay' fields in mode & pipe config
> structures be also updated here, when this property is set or they shall
> be left unchanged ?

Unchanged.

> 
> > So at modeset time we need to populate pipe_src_{w,h} from the user mode
> > first, and then overwrite those with the values from this property if
> > the property value != 0. Assuming we go with my "0 is special" plan.
> > 
> 
> If this property is once set by User, and then if it subsequently does a
> modeset, then the 'fb' passed with that modeset call, shall conform to
> which resolution, i.e to the mode passed by User or to the height &
> width specified by the current value of this property ? Or User 

Well the user can pass any size fb, but we must make sure that it is
big enough to contain the current primary plane size ie. pipesrc since
the primary plane is always fullscreen. But we also need to make sure
the user requested primary plane coordinates are suitable for the new
pipesrc, or the primary plane must be disabled when changing this
property. That kind of stuff is already checked by the primary plane
clipping code.


> 
> > > +
> > > +		/* pipesrc controls the size that is scaled from, which should
> > > +		* always be the user's requested size.
> > > +		*/
> > > +		I915_WRITE(PIPESRC(intel_crtc->pipe),
> > > +			((intel_crtc->config.pipe_src_w - 1) << 16) |
> > > +			 (intel_crtc->config.pipe_src_h - 1));
> > 
> > And do we need to reprogram the pfit on gmch?
> > 
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > >  	return ret;
> > >  }
> > >  
> > > @@ -10577,6 +10647,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> > >  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
> > >  
> > >  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> > > +
> > > +	intel_create_crtc_properties(&intel_crtc->base);
> > >  }
> > >  
> > >  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> > > -- 
> > > 1.9.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 

-- 
Ville Syrjälä
Intel OTC