[Intel-gfx] [PATCH v4 2/4] drm/i915: New drm crtc property for varying the Pipe Src size
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Apr 29 16:06:59 CEST 2014
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.
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.
> 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?
> +
> + 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?
> + }
> +
> + 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.
And we need to reclip sprites and cursors.
This is starting to be quite a lot of stuff. So for the initial
implementation maye 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.
> +
> + 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.
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.
> +
> + /* 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
More information about the Intel-gfx
mailing list