| Message ID | 20180615174406.12258-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Ville Syrjala (2018-06-15 18:44:05) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Validate that all display timings fit within the number of bits > we have in the transcoder timing registers. > > The limits are: > hsw+: > 4k: vdisplay, vblank_start > 8k: everything else > gen3+: > 4k: h/vdisplay, h/vblank_start > 8k: everything else > gen2: > 2k: h/vdisplay, h/vblank_start > 4k: everything else > > Also document the fact that the mode_config.max_width/height limits > refer to just the max framebuffer dimensions we support. Which may > be larger than the max hdisplay/vdisplay. In the ddx, I used them to filter max hdisplay/vdisplay... And completely ignored them wrt to framebuffer. -Chris
On Fri, Jun 15, 2018 at 07:44:08PM +0100, Chris Wilson wrote: > Quoting Ville Syrjala (2018-06-15 18:44:05) > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Validate that all display timings fit within the number of bits > > we have in the transcoder timing registers. > > > > The limits are: > > hsw+: > > 4k: vdisplay, vblank_start > > 8k: everything else > > gen3+: > > 4k: h/vdisplay, h/vblank_start > > 8k: everything else > > gen2: > > 2k: h/vdisplay, h/vblank_start > > 4k: everything else > > > > Also document the fact that the mode_config.max_width/height limits > > refer to just the max framebuffer dimensions we support. Which may > > be larger than the max hdisplay/vdisplay. > > In the ddx, I used them to filter max hdisplay/vdisplay... And > completely ignored them wrt to framebuffer. Whatever works :)
Quoting Ville Syrjälä (2018-06-15 20:48:49) > On Fri, Jun 15, 2018 at 07:44:08PM +0100, Chris Wilson wrote: > > Quoting Ville Syrjala (2018-06-15 18:44:05) > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Validate that all display timings fit within the number of bits > > > we have in the transcoder timing registers. > > > > > > The limits are: > > > hsw+: > > > 4k: vdisplay, vblank_start > > > 8k: everything else > > > gen3+: > > > 4k: h/vdisplay, h/vblank_start > > > 8k: everything else > > > gen2: > > > 2k: h/vdisplay, h/vblank_start > > > 4k: everything else > > > > > > Also document the fact that the mode_config.max_width/height limits > > > refer to just the max framebuffer dimensions we support. Which may > > > be larger than the max hdisplay/vdisplay. > > > > In the ddx, I used them to filter max hdisplay/vdisplay... And > > completely ignored them wrt to framebuffer. > > Whatever works :) Yeah, and this doesn't break -intel afaict, since ultimately validation is done by the kernel and we/the client just keeps on trying something until it works (or more often until they just give up). -Chris
On Fri, Jun 15, 2018 at 09:02:52PM +0100, Chris Wilson wrote: > Quoting Ville Syrjälä (2018-06-15 20:48:49) > > On Fri, Jun 15, 2018 at 07:44:08PM +0100, Chris Wilson wrote: > > > Quoting Ville Syrjala (2018-06-15 18:44:05) > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Validate that all display timings fit within the number of bits > > > > we have in the transcoder timing registers. > > > > > > > > The limits are: > > > > hsw+: > > > > 4k: vdisplay, vblank_start > > > > 8k: everything else > > > > gen3+: > > > > 4k: h/vdisplay, h/vblank_start > > > > 8k: everything else > > > > gen2: > > > > 2k: h/vdisplay, h/vblank_start > > > > 4k: everything else > > > > > > > > Also document the fact that the mode_config.max_width/height limits > > > > refer to just the max framebuffer dimensions we support. Which may > > > > be larger than the max hdisplay/vdisplay. > > > > > > In the ddx, I used them to filter max hdisplay/vdisplay... And > > > completely ignored them wrt to framebuffer. > > > > Whatever works :) > > Yeah, and this doesn't break -intel afaict, since ultimately validation > is done by the kernel and we/the client just keeps on trying something > until it works (or more often until they just give up). Yeah. The main issue is the user being presented with a pile of modes that can never actually work. It shouldn't be fatal but at least it's annoying to the user. I think we might want a new ioctl to have the kernel validate user modes as well. I guess we could try to ressurect the old ioctls to add/remove modes to the other list, but I'm thinking we might just want something that takes the connector ID and a pile of modes and returns a good/bad status for each (could snatch one of the mode type or flag bits for that I suppose).
Em Sex, 2018-06-15 às 20:44 +0300, Ville Syrjala escreveu: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Validate that all display timings fit within the number of bits > we have in the transcoder timing registers. > > The limits are: > hsw+: > 4k: vdisplay, vblank_start > 8k: everything else > gen3+: > 4k: h/vdisplay, h/vblank_start > 8k: everything else > gen2: > 2k: h/vdisplay, h/vblank_start > 4k: everything else > > Also document the fact that the mode_config.max_width/height limits > refer to just the max framebuffer dimensions we support. Which may > be larger than the max hdisplay/vdisplay. Verified against the specs. I can also confirm that the gen2+ specs still exist at the old URLs :). Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> While at it, can't you try to implement the other restrictions listed in those active/sync registers? Like hdisplay needs be multiples of 2 on gen2, hblank minimum being 32 on HSW (138 with audio), minimum vblank veing 5 or 8, etc? > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 35 > +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index f6655f482b67..6e3aa6815b30 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14572,6 +14572,10 @@ static enum drm_mode_status > intel_mode_valid(struct drm_device *dev, > const struct drm_display_mode *mode) > { > + struct drm_i915_private *dev_priv = to_i915(dev); > + int hdisplay_max, htotal_max; > + int vdisplay_max, vtotal_max; > + > /* > * Can't reject DBLSCAN here because Xorg ddxen can add > piles > * of DBLSCAN modes to the output's mode list when they > detect > @@ -14601,6 +14605,36 @@ intel_mode_valid(struct drm_device *dev, > DRM_MODE_FLAG_CLKDIV2)) > return MODE_BAD; > > + if (INTEL_GEN(dev_priv) >= 9 || > + IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) { > + hdisplay_max = 8192; /* FDI max 4096 handled > elsewhere */ > + vdisplay_max = 4096; > + htotal_max = 8192; > + vtotal_max = 8192; > + } else if (INTEL_GEN(dev_priv) >= 3) { > + hdisplay_max = 4096; > + vdisplay_max = 4096; > + htotal_max = 8192; > + vtotal_max = 8192; > + } else { > + hdisplay_max = 2048; > + vdisplay_max = 2048; > + htotal_max = 4096; > + vtotal_max = 4096; > + } > + > + if (mode->hdisplay > hdisplay_max || > + mode->hsync_start > htotal_max || > + mode->hsync_end > htotal_max || > + mode->htotal > htotal_max) > + return MODE_H_ILLEGAL; > + > + if (mode->vdisplay > vdisplay_max || > + mode->vsync_start > vtotal_max || > + mode->vsync_end > vtotal_max || > + mode->vtotal > vtotal_max) > + return MODE_V_ILLEGAL; > + > return MODE_OK; > } > > @@ -15039,6 +15073,7 @@ int intel_modeset_init(struct drm_device > *dev) > } > } > > + /* maximum framebuffer dimensions */ > if (IS_GEN2(dev_priv)) { > dev->mode_config.max_width = 2048; > dev->mode_config.max_height = 2048;
On Fri, Jun 15, 2018 at 01:30:24PM -0700, Paulo Zanoni wrote: > Em Sex, 2018-06-15 às 20:44 +0300, Ville Syrjala escreveu: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Validate that all display timings fit within the number of bits > > we have in the transcoder timing registers. > > > > The limits are: > > hsw+: > > 4k: vdisplay, vblank_start > > 8k: everything else > > gen3+: > > 4k: h/vdisplay, h/vblank_start > > 8k: everything else > > gen2: > > 2k: h/vdisplay, h/vblank_start > > 4k: everything else > > > > Also document the fact that the mode_config.max_width/height limits > > refer to just the max framebuffer dimensions we support. Which may > > be larger than the max hdisplay/vdisplay. > > Verified against the specs. I can also confirm that the gen2+ specs > still exist at the old URLs :). Cool. Thanks for digging through them. > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > While at it, can't you try to implement the other restrictions listed > in those active/sync registers? Like hdisplay needs be multiples of 2 > on gen2, hblank minimum being 32 on HSW (138 with audio), minimum > vblank veing 5 or 8, etc? I did more or less collect up the minimum/alignment restrictions as well, but wasn't really convinced checking for them is all that useful. There are probably some we should check like the HDMI/audio hblank/front porch restrictions, and if those aren't met we should probably fall back to driving the link on DVI mode or w/o audio. If we do check all the minimums then we'd have to decide whether to check against the absolute minimum from the spec for each platforms (which would end up being somewhat messy on account of the limits having changed so many times) or if we should just simplify a bit and declare eg. a minimum of 8 (or even higher) for hdisplay/vdisplay across the board. > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 35 > > +++++++++++++++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index f6655f482b67..6e3aa6815b30 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -14572,6 +14572,10 @@ static enum drm_mode_status > > intel_mode_valid(struct drm_device *dev, > > const struct drm_display_mode *mode) > > { > > + struct drm_i915_private *dev_priv = to_i915(dev); > > + int hdisplay_max, htotal_max; > > + int vdisplay_max, vtotal_max; > > + > > /* > > * Can't reject DBLSCAN here because Xorg ddxen can add > > piles > > * of DBLSCAN modes to the output's mode list when they > > detect > > @@ -14601,6 +14605,36 @@ intel_mode_valid(struct drm_device *dev, > > DRM_MODE_FLAG_CLKDIV2)) > > return MODE_BAD; > > > > + if (INTEL_GEN(dev_priv) >= 9 || > > + IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) { > > + hdisplay_max = 8192; /* FDI max 4096 handled > > elsewhere */ > > + vdisplay_max = 4096; > > + htotal_max = 8192; > > + vtotal_max = 8192; > > + } else if (INTEL_GEN(dev_priv) >= 3) { > > + hdisplay_max = 4096; > > + vdisplay_max = 4096; > > + htotal_max = 8192; > > + vtotal_max = 8192; > > + } else { > > + hdisplay_max = 2048; > > + vdisplay_max = 2048; > > + htotal_max = 4096; > > + vtotal_max = 4096; > > + } > > + > > + if (mode->hdisplay > hdisplay_max || > > + mode->hsync_start > htotal_max || > > + mode->hsync_end > htotal_max || > > + mode->htotal > htotal_max) > > + return MODE_H_ILLEGAL; > > + > > + if (mode->vdisplay > vdisplay_max || > > + mode->vsync_start > vtotal_max || > > + mode->vsync_end > vtotal_max || > > + mode->vtotal > vtotal_max) > > + return MODE_V_ILLEGAL; > > + > > return MODE_OK; > > } > > > > @@ -15039,6 +15073,7 @@ int intel_modeset_init(struct drm_device > > *dev) > > } > > } > > > > + /* maximum framebuffer dimensions */ > > if (IS_GEN2(dev_priv)) { > > dev->mode_config.max_width = 2048; > > dev->mode_config.max_height = 2048;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f6655f482b67..6e3aa6815b30 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14572,6 +14572,10 @@ static enum drm_mode_status intel_mode_valid(struct drm_device *dev, const struct drm_display_mode *mode) { + struct drm_i915_private *dev_priv = to_i915(dev); + int hdisplay_max, htotal_max; + int vdisplay_max, vtotal_max; + /* * Can't reject DBLSCAN here because Xorg ddxen can add piles * of DBLSCAN modes to the output's mode list when they detect @@ -14601,6 +14605,36 @@ intel_mode_valid(struct drm_device *dev, DRM_MODE_FLAG_CLKDIV2)) return MODE_BAD; + if (INTEL_GEN(dev_priv) >= 9 || + IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) { + hdisplay_max = 8192; /* FDI max 4096 handled elsewhere */ + vdisplay_max = 4096; + htotal_max = 8192; + vtotal_max = 8192; + } else if (INTEL_GEN(dev_priv) >= 3) { + hdisplay_max = 4096; + vdisplay_max = 4096; + htotal_max = 8192; + vtotal_max = 8192; + } else { + hdisplay_max = 2048; + vdisplay_max = 2048; + htotal_max = 4096; + vtotal_max = 4096; + } + + if (mode->hdisplay > hdisplay_max || + mode->hsync_start > htotal_max || + mode->hsync_end > htotal_max || + mode->htotal > htotal_max) + return MODE_H_ILLEGAL; + + if (mode->vdisplay > vdisplay_max || + mode->vsync_start > vtotal_max || + mode->vsync_end > vtotal_max || + mode->vtotal > vtotal_max) + return MODE_V_ILLEGAL; + return MODE_OK; } @@ -15039,6 +15073,7 @@ int intel_modeset_init(struct drm_device *dev) } } + /* maximum framebuffer dimensions */ if (IS_GEN2(dev_priv)) { dev->mode_config.max_width = 2048; dev->mode_config.max_height = 2048;