[Intel-gfx] [PATCH v4 2/4] drm/i915: New drm crtc property for varying the Pipe Src size
Akash Goel
akash.goel at intel.com
Sun May 4 12:21:51 CEST 2014
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.
> > + }
> > +
> > + 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.
> 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.
> > +
> > + 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 ?
> 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
> > +
> > + /* 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
>
More information about the Intel-gfx
mailing list