| Message ID | 20180913200140.14073-6-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: drm/i915: GTT remapping for display | expand |
On Thu, Sep 13, 2018 at 11:01:40PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > With gtt remapping in place we can use arbitraily large framebuffers. > Let's bump the limits as high as we can (32k-1). Going beyond that > would require switching our s16.16 src coordinate representation to > something with more spare bits. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 346572cf734a..0ee6255cd040 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15527,8 +15527,8 @@ int intel_modeset_init(struct drm_device *dev) > dev->mode_config.max_width = 4096; > dev->mode_config.max_height = 4096; > } else { > - dev->mode_config.max_width = 8192; > - dev->mode_config.max_height = 8192; > + dev->mode_config.max_width = 32767; > + dev->mode_config.max_height = 32767; It appears that neither Mesa nor glamor will check whether window system buffers exceed the capabilities of the 3D engine. So trying to use a >16k trips an assert when genxml tries to pack the surface_state. So looks like we'll need to limit this to 16k for gen7+, and leave it at 8k for gen4+. If userspace gets smarter later on we could add a new client cap to expose higher limits. If I'm reading the spec correctly gen4+ render engine has a stride limit of 512KiB and gen7+ has 256KiB. So my choice of 256KiB seems good enough for both. > } > > if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) { > -- > 2.16.4
Quoting Ville Syrjälä (2018-09-19 17:59:51) > On Thu, Sep 13, 2018 at 11:01:40PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > With gtt remapping in place we can use arbitraily large framebuffers. > > Let's bump the limits as high as we can (32k-1). Going beyond that > > would require switching our s16.16 src coordinate representation to > > something with more spare bits. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 346572cf734a..0ee6255cd040 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -15527,8 +15527,8 @@ int intel_modeset_init(struct drm_device *dev) > > dev->mode_config.max_width = 4096; > > dev->mode_config.max_height = 4096; > > } else { > > - dev->mode_config.max_width = 8192; > > - dev->mode_config.max_height = 8192; > > + dev->mode_config.max_width = 32767; > > + dev->mode_config.max_height = 32767; > > It appears that neither Mesa nor glamor will check whether window system > buffers exceed the capabilities of the 3D engine. So trying to use a >16k > trips an assert when genxml tries to pack the surface_state. > > So looks like we'll need to limit this to 16k for gen7+, and leave it > at 8k for gen4+. If userspace gets smarter later on we could add a new > client cap to expose higher limits. At which point, the client can just ignore this field and just use rejection criteria from addfb2 and/or setcrtc (or the atomic variant). Or we can just keep this field as meaning the maximum size of a single CRTC and just ignore it entirely in -modesetting for fb size as we do elsewhere. -Chris
On Thu, Sep 20, 2018 at 09:09:03AM +0100, Chris Wilson wrote: > Quoting Ville Syrjälä (2018-09-19 17:59:51) > > On Thu, Sep 13, 2018 at 11:01:40PM +0300, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > With gtt remapping in place we can use arbitraily large framebuffers. > > > Let's bump the limits as high as we can (32k-1). Going beyond that > > > would require switching our s16.16 src coordinate representation to > > > something with more spare bits. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 346572cf734a..0ee6255cd040 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -15527,8 +15527,8 @@ int intel_modeset_init(struct drm_device *dev) > > > dev->mode_config.max_width = 4096; > > > dev->mode_config.max_height = 4096; > > > } else { > > > - dev->mode_config.max_width = 8192; > > > - dev->mode_config.max_height = 8192; > > > + dev->mode_config.max_width = 32767; > > > + dev->mode_config.max_height = 32767; > > > > It appears that neither Mesa nor glamor will check whether window system > > buffers exceed the capabilities of the 3D engine. So trying to use a >16k > > trips an assert when genxml tries to pack the surface_state. > > > > So looks like we'll need to limit this to 16k for gen7+, and leave it > > at 8k for gen4+. If userspace gets smarter later on we could add a new > > client cap to expose higher limits. > > At which point, the client can just ignore this field and just use > rejection criteria from addfb2 and/or setcrtc (or the atomic variant). I suppose. Though probing the max size using addfb might be a bit tedious. That's assuming the client wants to report the max in some way, as X does. > > Or we can just keep this field as meaning the maximum size of a single > CRTC and just ignore it entirely in -modesetting for fb size as we do > elsewhere. Would require changing the core addfb code to ignore these limits for i915 but keep chekcing them for the other drivers. So a bit of work, and I'm not really sure what the actual benefit for i915 would be.
Quoting Ville Syrjälä (2018-09-20 20:41:30) > On Thu, Sep 20, 2018 at 09:09:03AM +0100, Chris Wilson wrote: > > Quoting Ville Syrjälä (2018-09-19 17:59:51) > > > On Thu, Sep 13, 2018 at 11:01:40PM +0300, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > With gtt remapping in place we can use arbitraily large framebuffers. > > > > Let's bump the limits as high as we can (32k-1). Going beyond that > > > > would require switching our s16.16 src coordinate representation to > > > > something with more spare bits. > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index 346572cf734a..0ee6255cd040 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -15527,8 +15527,8 @@ int intel_modeset_init(struct drm_device *dev) > > > > dev->mode_config.max_width = 4096; > > > > dev->mode_config.max_height = 4096; > > > > } else { > > > > - dev->mode_config.max_width = 8192; > > > > - dev->mode_config.max_height = 8192; > > > > + dev->mode_config.max_width = 32767; > > > > + dev->mode_config.max_height = 32767; > > > > > > It appears that neither Mesa nor glamor will check whether window system > > > buffers exceed the capabilities of the 3D engine. So trying to use a >16k > > > trips an assert when genxml tries to pack the surface_state. > > > > > > So looks like we'll need to limit this to 16k for gen7+, and leave it > > > at 8k for gen4+. If userspace gets smarter later on we could add a new > > > client cap to expose higher limits. > > > > At which point, the client can just ignore this field and just use > > rejection criteria from addfb2 and/or setcrtc (or the atomic variant). > > I suppose. Though probing the max size using addfb might be a bit > tedious. That's assuming the client wants to report the max in some > way, as X does. > > > > > Or we can just keep this field as meaning the maximum size of a single > > CRTC and just ignore it entirely in -modesetting for fb size as we do > > elsewhere. > > Would require changing the core addfb code to ignore these > limits for i915 but keep chekcing them for the other drivers. > So a bit of work, and I'm not really sure what the actual > benefit for i915 would be. Why is the core addfb using these fields? Since when did they *stop* being per-CRTC limits? -Chris
On Thu, Sep 20, 2018 at 08:46:42PM +0100, Chris Wilson wrote: > Quoting Ville Syrjälä (2018-09-20 20:41:30) > > On Thu, Sep 20, 2018 at 09:09:03AM +0100, Chris Wilson wrote: > > > Quoting Ville Syrjälä (2018-09-19 17:59:51) > > > > On Thu, Sep 13, 2018 at 11:01:40PM +0300, Ville Syrjala wrote: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > With gtt remapping in place we can use arbitraily large framebuffers. > > > > > Let's bump the limits as high as we can (32k-1). Going beyond that > > > > > would require switching our s16.16 src coordinate representation to > > > > > something with more spare bits. > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > > index 346572cf734a..0ee6255cd040 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -15527,8 +15527,8 @@ int intel_modeset_init(struct drm_device *dev) > > > > > dev->mode_config.max_width = 4096; > > > > > dev->mode_config.max_height = 4096; > > > > > } else { > > > > > - dev->mode_config.max_width = 8192; > > > > > - dev->mode_config.max_height = 8192; > > > > > + dev->mode_config.max_width = 32767; > > > > > + dev->mode_config.max_height = 32767; > > > > > > > > It appears that neither Mesa nor glamor will check whether window system > > > > buffers exceed the capabilities of the 3D engine. So trying to use a >16k > > > > trips an assert when genxml tries to pack the surface_state. > > > > > > > > So looks like we'll need to limit this to 16k for gen7+, and leave it > > > > at 8k for gen4+. If userspace gets smarter later on we could add a new > > > > client cap to expose higher limits. > > > > > > At which point, the client can just ignore this field and just use > > > rejection criteria from addfb2 and/or setcrtc (or the atomic variant). > > > > I suppose. Though probing the max size using addfb might be a bit > > tedious. That's assuming the client wants to report the max in some > > way, as X does. > > > > > > > > Or we can just keep this field as meaning the maximum size of a single > > > CRTC and just ignore it entirely in -modesetting for fb size as we do > > > elsewhere. > > > > Would require changing the core addfb code to ignore these > > limits for i915 but keep chekcing them for the other drivers. > > So a bit of work, and I'm not really sure what the actual > > benefit for i915 would be. > > Why is the core addfb using these fields? Since when did they *stop* > being per-CRTC limits? Looks like addfb has been using them since the beginning: commit f453ba0460742ad027ae0c4c7d61e62817b3e7ef Author: Dave Airlie <airlied@redhat.com> Date: Fri Nov 7 14:05:41 2008 -0800 DRM: add mode setting support And looks like they never matched the per-crtc limits for i915 either. Until HSW hdisplay limit was 4k but max_width was already set to 8k in: commit 79e539453b34e35f39299a899d263b0a1f1670bd Author: Jesse Barnes <jbarnes@virtuousgeek.org> Date: Fri Nov 7 14:24:08 2008 -0800 DRM: i915: add mode setting support
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 346572cf734a..0ee6255cd040 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15527,8 +15527,8 @@ int intel_modeset_init(struct drm_device *dev) dev->mode_config.max_width = 4096; dev->mode_config.max_height = 4096; } else { - dev->mode_config.max_width = 8192; - dev->mode_config.max_height = 8192; + dev->mode_config.max_width = 32767; + dev->mode_config.max_height = 32767; } if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) {