From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v4 3/3] drm/i915: New drm crtc property for varying the size of borders Date: Fri, 11 Apr 2014 14:42:21 +0300 Message-ID: <20140411114221.GB18465@intel.com> References: <1395564976-19870-1-git-send-email-akash.goel@intel.com> <1395806112-21713-1-git-send-email-akash.goel@intel.com> <20140408162816.GC18465@intel.com> <1397115836.2672.6.camel@akashgoe-desktop> <20140410113405.GX18465@intel.com> <1397185448.429.15.camel@akashgoe-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id A57458A628 for ; Fri, 11 Apr 2014 04:42:25 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1397185448.429.15.camel@akashgoe-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Akash Goel Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Apr 11, 2014 at 08:34:08AM +0530, Akash Goel wrote: > On Thu, 2014-04-10 at 14:34 +0300, Ville Syrj=E4l=E4 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=E4l=E4 wrote: > > > > On Wed, Mar 26, 2014 at 09:25:12AM +0530, akash.goel@intel.com wrot= e: > > > > > From: Akash Goel > > > > > = > > > > > 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 > > > > > --- > > > > > 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(-) > > > > > = > > > > > > > @@ -42,6 +57,60 @@ intel_fixed_panel_mode(const struct drm_displa= y_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 =3D &pipe_config->adjusted_mode; > > > > > + > > > > > + src_width =3D pipe_config->pipe_src_w; > > > > > + src_height =3D pipe_config->pipe_src_h; > > > > > + > > > > > + tot_width =3D adjusted_mode->hdisplay; > > > > > + tot_height =3D adjusted_mode->vdisplay; > > > > > + > > > > > + /* > > > > > + * Having non zero borders will reduce the size of 'HACTIVE/VAC= TIVE' > > > > > + * region. So (HACTIVE - Left border - Right Border) * > > > > > + * (VACTIVE - Top Border - Bottom border) will effectively be= the > > > > > + * output rectangle on screen > > > > > + */ > > > > > + dst_width =3D tot_width - > > > > > + (((intel_crtc->border_size >> 16) & 0xffff) * 2); > > > > > + dst_height =3D tot_height - > > > > > + ((intel_crtc->border_size & 0xffff) * 2); > > > > = > > > > I'm thinking that we should allow the user to specify each border w= idth > > > > individually rather than just assume left=3D=3Dright and top=3D=3Db= ottom. > > > > = > > > = > > > 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. > = > > > Tried setting the borders on EDP & HDMI panels by manipulating the Pi= pe > > > 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 aft= er > > 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=3D1920, HBLANK START=3D1920, HSYNC START=3D1968, HYSNC END=3D200= 0, > HBLANK END=3D2080, HTOTAL=3D2080. = > was changed to = > HACTIVE=3D1856, HBLANK START=3D1888, HSYNC START=3D1952, HYSNC END=3D1= 984, > HBLANK END=3D2048, HTOTAL=3D2080. = > = > And Similarly = > VACTIVE=3D1200, VBLANK START=3D1200, VSYNC START=3D1203, VYSNC END=3D1209, > VBLANK END=3D1235, VTOTAL=3D1235. = > was changed to > VACTIVE=3D1140, VBLANK START=3D1170, VSYNC START=3D1185, VYSNC END=3D1191, > VBLANK END=3D1205, VTOTAL=3D1235. = Your sync values seem to be a bit off. AFAICS they should be: hsync_start =3D 1936, hsync_end =3D 1968 vsync_start =3D 1173, vsync_end =3D 1179 > = > 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. > = > > > = > > > > > + > > > > > + if ((dst_width =3D=3D 0) || (dst_height =3D=3D 0)) { > > > > > + DRM_ERROR("Invalid border size input\n"); > > > > > + return; > > > > = > > > > This is clear sign here that we should do all this stuff in the com= pute > > > > 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 func= tions > so that if there is an error in panel fitter configuration, it can be com= municated > 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 =3D panel_fitter_scaling(src_width, dst_wid= th); > > > > > + pf_vertical_ratio =3D panel_fitter_scaling(src_height, dst_he= ight); > > > > > + > > > > > + 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 =3D (adjusted_mode->hdisplay - dst_width + 1)/2; > > > > > + y =3D (adjusted_mode->vdisplay - dst_height + 1)/2; > > > > > + > > > > > + pipe_config->pch_pfit.pos =3D (x << 16) | y; > > > > > + pipe_config->pch_pfit.size =3D (dst_width << 16) | dst_height; > > > > > + pipe_config->pch_pfit.enabled =3D pipe_config->pch_pfit.size != =3D 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 *in= tel_crtc, > > > > > = > > > > > x =3D y =3D width =3D height =3D 0; > > > > > = > > > > > + /* check if User wants to apply the borders, or wants to forcef= ully > > > > > + 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 =3D=3D pipe_config->pipe_src_w && > > > > > adjusted_mode->vdisplay =3D=3D pipe_config->pipe_src_h) > > > > > @@ -157,19 +233,6 @@ centre_vertically(struct drm_display_mode *m= ode, > > > > > mode->crtc_vsync_end =3D 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 =3D source * FACTOR / target; > > > > > - return (FACTOR * ratio + FACTOR/2) / FACTOR; > > > > > -} > > > > > - > > > > > static void i965_scale_aspect(struct intel_crtc_config *pipe_con= fig, > > > > > u32 *pfit_control) > > > > > { > > > > > @@ -247,6 +310,86 @@ static void i9xx_scale_aspect(struct intel_c= rtc_config *pipe_config, > > > > > } > > > > > } > > > > > = > > > > > +void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_cr= tc, > > > > > + struct intel_crtc_config *pipe_config) > > > > > +{ > > > > > + struct drm_device *dev =3D intel_crtc->base.dev; > > > > > + u32 pfit_control =3D 0, border =3D 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 =3D &pipe_config->adjusted_mode; > > > > > + > > > > > + src_width =3D pipe_config->pipe_src_w; > > > > > + src_height =3D pipe_config->pipe_src_h; > > > > > + > > > > > + tot_width =3D adjusted_mode->hdisplay; > > > > > + tot_height =3D adjusted_mode->vdisplay; > > > > > + > > > > > + /* > > > > > + * Having non zero borders will reduce the size of 'HACTIVE/VAC= TIVE' > > > > > + * region. So (HACTIVE - Left border - Right Border) * > > > > > + * (VACTIVE - Top Border - Bottom border) will effectively be= the > > > > > + * output rectangle on screen > > > > > + */ > > > > > + dst_width =3D tot_width - > > > > > + (((intel_crtc->border_size >> 16) & 0xffff) * 2); > > > > > + dst_height =3D tot_height - > > > > > + ((intel_crtc->border_size & 0xffff) * 2); > > > > > + > > > > > + if ((dst_width =3D=3D 0) || (dst_height =3D=3D 0)) { > > > > > + DRM_ERROR("Invalid border size input\n"); > > > > > + return; > > > > > + } > > > > > + > > > > > + pf_horizontal_ratio =3D panel_fitter_scaling(src_width, dst_wid= th); > > > > > + pf_vertical_ratio =3D panel_fitter_scaling(src_height, dst_he= ight); > > > > > + > > > > > + 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 !=3D tot_width) > > > > > + centre_horizontally(adjusted_mode, dst_width); > > > > > + if (dst_height !=3D 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 =3D=3D pipe_config->pipe_src_w= && > > > > > + adjusted_mode->crtc_vdisplay =3D=3D pipe_config->pipe_src_h= ) { > > > > > + BUG_ON(!intel_crtc->pfit_enabled); > > > > > + DRM_DEBUG_KMS("Forcefully enabling the Panel fitter\n"); > > > > > + } > > > > > + > > > > > + border =3D LVDS_BORDER_ENABLE; > > > > > + > > > > > + if (INTEL_INFO(dev)->gen >=3D 4) { > > > > > + /* PFIT_SCALING_PROGRAMMED is de-featured on BYT */ > > > > > + pfit_control |=3D PFIT_ENABLE | PFIT_SCALING_AUTO; > > > > > + pfit_control |=3D ((intel_crtc->pipe << PFIT_PIPE_SHIFT) | PFI= T_FILTER_FUZZY); > > > > > + } else { > > > > > + pfit_control |=3D (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 =3D=3D 18) > > > > > + pfit_control |=3D PANEL_8TO6_DITHER_ENABLE; > > > > > + > > > > > + pipe_config->gmch_pfit.control =3D pfit_control; > > > > > + pipe_config->gmch_pfit.lvds_border_bits =3D 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_c= rtc *intel_crtc, > > > > > = > > > > > adjusted_mode =3D &pipe_config->adjusted_mode; > > > > > = > > > > > + /* check if User wants to apply the borders, or wants to forcef= ully > > > > > + 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 =3D=3D pipe_config->pipe_src_w && > > > > > adjusted_mode->vdisplay =3D=3D pipe_config->pipe_src_h) > > > > > -- = > > > > > 1.8.5.2 > > > > > = > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > = > > > = > > = > = -- = Ville Syrj=E4l=E4 Intel OTC