On Fri, 2014-04-11 at 14:42 +0300, Ville Syrjälä wrote:
> 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.
>
ok so should define 4 new properties like "left margin", "right margin",
"top margin", "bottom margin" already defined for TV.
> >
> > > > 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
>
Actually these are the values outputted by 'centre_horizontally' &
'centre_vertically' functions, where the logic is to position the
hsyc/vsync pulses in the middle of the corresponding hblank/vblank
intervals.
Thus the HSYNC START=1952, HYSNC END=1984 (pulse of 32 pixels) is lying
in the middle of HBLANK START=1888, HBLANK END=2048 interval (160
pixels).
> >
> > 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.
>
Yes, the 'Center' Panel fitting mode of "scaling mode" property is not
working on VLV (for HDMI/EDP panels).
> >
> > > >
> > > > > > +
> > > > > > + 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
> > > > >
> > > >
> > >
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
