Hi Ville,
Please could you respond to my last mail.
This will enable me to make further progress on this patch.
Bes Regards
Akash
On Sun, 2014-05-04 at 15:51 +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@xxxxxxxxx wrote:
> > > From: Akash Goel <akash.goel@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
