* [PATCH 0/4] drm/i915: Some sanity checks for modes
@ 2013-10-01 13:13 ville.syrjala
2013-10-01 13:13 ` [PATCH 1/4] drm/i915: Don't populate pipe_src_{w, h} multiple times ville.syrjala
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: ville.syrjala @ 2013-10-01 13:13 UTC (permalink / raw)
To: intel-gfx
I hit a div by zero due to accidentally passing in hdisplay=0, and that
got me looking a bit more at how much adjustment/verification we do to
display modes in general. The answer is not a lot.
So this series starts us off on the right track, and eliminates some
of the potential issues. There are still a lot of hardware specific
limits we don't handle (eg. minimum hblank length), but we have to start
somewhere.
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/4] drm/i915: Don't populate pipe_src_{w, h} multiple times 2013-10-01 13:13 [PATCH 0/4] drm/i915: Some sanity checks for modes ville.syrjala @ 2013-10-01 13:13 ` ville.syrjala 2013-10-01 19:03 ` Daniel Vetter 2013-10-01 13:13 ` [PATCH 2/4] drm/i915: Reject modes where hdisplay or vdisplay is too small ville.syrjala ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: ville.syrjala @ 2013-10-01 13:13 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> If we ever end up doing the retry loop due to bandwidth constraints, we would rewrite pipe_src_{w,n} based on adjusted_mode timings. But by that time the encoder may have already replaced the adjusted_mode with a fixed panel mode, which would then corrupt pipe_src_{w,h}. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a91f20a..a695888 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8472,6 +8472,11 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, if (plane_bpp < 0) goto fail; + /* Determine the real pipe dimensions */ + drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE); + pipe_config->pipe_src_w = pipe_config->adjusted_mode.crtc_hdisplay; + pipe_config->pipe_src_h = pipe_config->adjusted_mode.crtc_vdisplay; + encoder_retry: /* Ensure the port clock defaults are reset when retrying. */ pipe_config->port_clock = 0; @@ -8480,10 +8485,6 @@ encoder_retry: /* Fill in default crtc timings, allow encoders to overwrite them. */ drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE); - /* set_crtcinfo() may have adjusted hdisplay/vdisplay */ - pipe_config->pipe_src_w = pipe_config->adjusted_mode.crtc_hdisplay; - pipe_config->pipe_src_h = pipe_config->adjusted_mode.crtc_vdisplay; - /* Pass our mode to the connectors and the CRTC to give them a chance to * adjust it according to limitations or connector properties, and also * a chance to reject the mode entirely. -- 1.8.1.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] drm/i915: Don't populate pipe_src_{w, h} multiple times 2013-10-01 13:13 ` [PATCH 1/4] drm/i915: Don't populate pipe_src_{w, h} multiple times ville.syrjala @ 2013-10-01 19:03 ` Daniel Vetter 2013-10-01 19:14 ` Ville Syrjälä 2013-10-01 19:52 ` [PATCH v2 " ville.syrjala 0 siblings, 2 replies; 14+ messages in thread From: Daniel Vetter @ 2013-10-01 19:03 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx On Tue, Oct 01, 2013 at 04:13:28PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > If we ever end up doing the retry loop due to bandwidth constraints, we > would rewrite pipe_src_{w,n} based on adjusted_mode timings. But by that > time the encoder may have already replaced the adjusted_mode with a > fixed panel mode, which would then corrupt pipe_src_{w,h}. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index a91f20a..a695888 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8472,6 +8472,11 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > if (plane_bpp < 0) > goto fail; > > + /* Determine the real pipe dimensions */ I think it'll benefit clarity a bit if we'd use the requested mode here (even though we never really care about it's crtc timings anywhere else). This needs more comment imo. What about the following instead? "Determine the real pipe dimensions. Note that stereo modes can increase the actual pipe size due to the frame doubling and insertion of additional space for blanks between the frame. This is stored in the crtc timings. We use the requested mode to do this computation to clearly distinguish it from the adjusted mode, which can be changed by the connectors in the below retry loop." Cheers, Daniel > + drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE); > + pipe_config->pipe_src_w = pipe_config->adjusted_mode.crtc_hdisplay; > + pipe_config->pipe_src_h = pipe_config->adjusted_mode.crtc_vdisplay; > + > encoder_retry: > /* Ensure the port clock defaults are reset when retrying. */ > pipe_config->port_clock = 0; > @@ -8480,10 +8485,6 @@ encoder_retry: > /* Fill in default crtc timings, allow encoders to overwrite them. */ > drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE); > > - /* set_crtcinfo() may have adjusted hdisplay/vdisplay */ > - pipe_config->pipe_src_w = pipe_config->adjusted_mode.crtc_hdisplay; > - pipe_config->pipe_src_h = pipe_config->adjusted_mode.crtc_vdisplay; > - > /* Pass our mode to the connectors and the CRTC to give them a chance to > * adjust it according to limitations or connector properties, and also > * a chance to reject the mode entirely. > -- > 1.8.1.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] drm/i915: Don't populate pipe_src_{w, h} multiple times 2013-10-01 19:03 ` Daniel Vetter @ 2013-10-01 19:14 ` Ville Syrjälä 2013-10-01 19:52 ` [PATCH v2 " ville.syrjala 1 sibling, 0 replies; 14+ messages in thread From: Ville Syrjälä @ 2013-10-01 19:14 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Tue, Oct 01, 2013 at 09:03:52PM +0200, Daniel Vetter wrote: > On Tue, Oct 01, 2013 at 04:13:28PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > If we ever end up doing the retry loop due to bandwidth constraints, we > > would rewrite pipe_src_{w,n} based on adjusted_mode timings. But by that > > time the encoder may have already replaced the adjusted_mode with a > > fixed panel mode, which would then corrupt pipe_src_{w,h}. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index a91f20a..a695888 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -8472,6 +8472,11 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > > if (plane_bpp < 0) > > goto fail; > > > > + /* Determine the real pipe dimensions */ > > I think it'll benefit clarity a bit if we'd use the requested mode > here (even though we never really care about it's crtc timings anywhere > else). Yeah that could be more clear. > > This needs more comment imo. What about the following instead? > > "Determine the real pipe dimensions. Note that stereo modes can increase > the actual pipe size due to the frame doubling and insertion of additional > space for blanks between the frame. This is stored in the crtc timings. We > use the requested mode to do this computation to clearly distinguish it > from the adjusted mode, which can be changed by the connectors in the > below retry loop." > Sure. I'll slap that on and send a v2. > Cheers, Daniel > > > + drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE); > > + pipe_config->pipe_src_w = pipe_config->adjusted_mode.crtc_hdisplay; > > + pipe_config->pipe_src_h = pipe_config->adjusted_mode.crtc_vdisplay; > > + > > encoder_retry: > > /* Ensure the port clock defaults are reset when retrying. */ > > pipe_config->port_clock = 0; > > @@ -8480,10 +8485,6 @@ encoder_retry: > > /* Fill in default crtc timings, allow encoders to overwrite them. */ > > drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE); > > > > - /* set_crtcinfo() may have adjusted hdisplay/vdisplay */ > > - pipe_config->pipe_src_w = pipe_config->adjusted_mode.crtc_hdisplay; > > - pipe_config->pipe_src_h = pipe_config->adjusted_mode.crtc_vdisplay; > > - > > /* Pass our mode to the connectors and the CRTC to give them a chance to > > * adjust it according to limitations or connector properties, and also > > * a chance to reject the mode entirely. > > -- > > 1.8.1.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/4] drm/i915: Don't populate pipe_src_{w, h} multiple times 2013-10-01 19:03 ` Daniel Vetter 2013-10-01 19:14 ` Ville Syrjälä @ 2013-10-01 19:52 ` ville.syrjala 2013-10-01 20:03 ` Daniel Vetter 1 sibling, 1 reply; 14+ messages in thread From: ville.syrjala @ 2013-10-01 19:52 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> If we ever end up doing the retry loop due to bandwidth constraints, we would rewrite pipe_src_{w,n} based on adjusted_mode timings. But by that time the encoder may have already replaced the adjusted_mode with a fixed panel mode, which would then corrupt pipe_src_{w,h}. v2: Use requested_mode and slap on a big comment from Daniel Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a91f20a..4fa97a5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8472,6 +8472,18 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, if (plane_bpp < 0) goto fail; + /* + * Determine the real pipe dimensions. Note that stereo modes can + * increase the actual pipe size due to the frame doubling and + * insertion of additional space for blanks between the frame. This + * is stored in the crtc timings. We use the requested mode to do this + * computation to clearly distinguish it from the adjusted mode, which + * can be changed by the connectors in the below retry loop. + */ + drm_mode_set_crtcinfo(&pipe_config->requested_mode, CRTC_STEREO_DOUBLE); + pipe_config->pipe_src_w = pipe_config->requested_mode.crtc_hdisplay; + pipe_config->pipe_src_h = pipe_config->requested_mode.crtc_vdisplay; + encoder_retry: /* Ensure the port clock defaults are reset when retrying. */ pipe_config->port_clock = 0; @@ -8480,10 +8492,6 @@ encoder_retry: /* Fill in default crtc timings, allow encoders to overwrite them. */ drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE); - /* set_crtcinfo() may have adjusted hdisplay/vdisplay */ - pipe_config->pipe_src_w = pipe_config->adjusted_mode.crtc_hdisplay; - pipe_config->pipe_src_h = pipe_config->adjusted_mode.crtc_vdisplay; - /* Pass our mode to the connectors and the CRTC to give them a chance to * adjust it according to limitations or connector properties, and also * a chance to reject the mode entirely. -- 1.8.1.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/4] drm/i915: Don't populate pipe_src_{w, h} multiple times 2013-10-01 19:52 ` [PATCH v2 " ville.syrjala @ 2013-10-01 20:03 ` Daniel Vetter 0 siblings, 0 replies; 14+ messages in thread From: Daniel Vetter @ 2013-10-01 20:03 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx On Tue, Oct 01, 2013 at 10:52:14PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > If we ever end up doing the retry loop due to bandwidth constraints, we > would rewrite pipe_src_{w,n} based on adjusted_mode timings. But by that > time the encoder may have already replaced the adjusted_mode with a > fixed panel mode, which would then corrupt pipe_src_{w,h}. > > v2: Use requested_mode and slap on a big comment from Daniel > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Queued for -next, thanks for the patch. I'll wait for Rodrigo to sanity check the next patch, and the third one doesn't apply right away (and I'm lazy). -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index a91f20a..4fa97a5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8472,6 +8472,18 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > if (plane_bpp < 0) > goto fail; > > + /* > + * Determine the real pipe dimensions. Note that stereo modes can > + * increase the actual pipe size due to the frame doubling and > + * insertion of additional space for blanks between the frame. This > + * is stored in the crtc timings. We use the requested mode to do this > + * computation to clearly distinguish it from the adjusted mode, which > + * can be changed by the connectors in the below retry loop. > + */ > + drm_mode_set_crtcinfo(&pipe_config->requested_mode, CRTC_STEREO_DOUBLE); > + pipe_config->pipe_src_w = pipe_config->requested_mode.crtc_hdisplay; > + pipe_config->pipe_src_h = pipe_config->requested_mode.crtc_vdisplay; > + > encoder_retry: > /* Ensure the port clock defaults are reset when retrying. */ > pipe_config->port_clock = 0; > @@ -8480,10 +8492,6 @@ encoder_retry: > /* Fill in default crtc timings, allow encoders to overwrite them. */ > drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE); > > - /* set_crtcinfo() may have adjusted hdisplay/vdisplay */ > - pipe_config->pipe_src_w = pipe_config->adjusted_mode.crtc_hdisplay; > - pipe_config->pipe_src_h = pipe_config->adjusted_mode.crtc_vdisplay; > - > /* Pass our mode to the connectors and the CRTC to give them a chance to > * adjust it according to limitations or connector properties, and also > * a chance to reject the mode entirely. > -- > 1.8.1.5 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] drm/i915: Reject modes where hdisplay or vdisplay is too small 2013-10-01 13:13 [PATCH 0/4] drm/i915: Some sanity checks for modes ville.syrjala 2013-10-01 13:13 ` [PATCH 1/4] drm/i915: Don't populate pipe_src_{w, h} multiple times ville.syrjala @ 2013-10-01 13:13 ` ville.syrjala 2013-10-01 20:08 ` Rodrigo Vivi 2013-10-01 13:13 ` [PATCH 3/4] drm/915: Make sure pipe source size isn't zero ville.syrjala 2013-10-01 13:13 ` [PATCH 4/4] drm/i915: Do some basic sanity adjustments on the user provided mode ville.syrjala 3 siblings, 1 reply; 14+ messages in thread From: ville.syrjala @ 2013-10-01 13:13 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> Check the adjusted_mode hdisplay/vdisplay against the limits from BSpec. Move the ctg+ zero front porch check to the same function, and change it to use the crtc_ values as well. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a695888..fa02677 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4197,11 +4197,46 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc, pipe_config->pipe_bpp <= 24; } +static int intel_check_mode(struct drm_crtc *crtc, + const struct drm_display_mode *mode) +{ + struct drm_device *dev = crtc->dev; + + /* + * Cantiga+ cannot handle modes with a hsync front porch of 0. + * WaPruneModeWithIncorrectHsyncOffset:ctg,elk,ilk,snb,ivb,vlv,hsw. + */ + if ((INTEL_INFO(dev)->gen > 4 || IS_G4X(dev)) && + mode->crtc_hsync_start == mode->crtc_hdisplay) + return -EINVAL; + + if (IS_HASWELL(dev)) { + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI) && + mode->crtc_hdisplay < 256) + return -EINVAL; + + if (mode->crtc_hdisplay < 64 || mode->crtc_vdisplay < 7) + return -EINVAL; + } else if (INTEL_INFO(dev)->gen >= 5) { + if (mode->crtc_hdisplay < 64 || mode->crtc_vdisplay < 7) + return -EINVAL; + } else if (INTEL_INFO(dev)->gen >= 4) { + if (mode->crtc_hdisplay < 3 || mode->crtc_vdisplay < 3) + return -EINVAL; + } else { + if (mode->crtc_hdisplay < 2 || mode->crtc_vdisplay < 2) + return -EINVAL; + } + + return 0; +} + static int intel_crtc_compute_config(struct intel_crtc *crtc, struct intel_crtc_config *pipe_config) { struct drm_device *dev = crtc->base.dev; struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode; + int ret; /* FIXME should check pixel clock limits on all platforms */ if (INTEL_INFO(dev)->gen < 4) { @@ -4236,12 +4271,9 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, intel_is_dual_link_lvds(dev)) || pipe_config->double_wide) pipe_config->pipe_src_w &= ~1; - /* Cantiga+ cannot handle modes with a hsync front porch of 0. - * WaPruneModeWithIncorrectHsyncOffset:ctg,elk,ilk,snb,ivb,vlv,hsw. - */ - if ((INTEL_INFO(dev)->gen > 4 || IS_G4X(dev)) && - adjusted_mode->hsync_start == adjusted_mode->hdisplay) - return -EINVAL; + ret = intel_check_mode(&crtc->base, adjusted_mode); + if (ret) + return ret; if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) && pipe_config->pipe_bpp > 10*3) { pipe_config->pipe_bpp = 10*3; /* 12bpc is gen5+ */ -- 1.8.1.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] drm/i915: Reject modes where hdisplay or vdisplay is too small 2013-10-01 13:13 ` [PATCH 2/4] drm/i915: Reject modes where hdisplay or vdisplay is too small ville.syrjala @ 2013-10-01 20:08 ` Rodrigo Vivi 2013-10-01 20:55 ` Ville Syrjälä 0 siblings, 1 reply; 14+ messages in thread From: Rodrigo Vivi @ 2013-10-01 20:08 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Tue, Oct 1, 2013 at 10:13 AM, <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Check the adjusted_mode hdisplay/vdisplay against the limits from BSpec. > > Move the ctg+ zero front porch check to the same function, and change it > to use the crtc_ values as well. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++++++++++++++++----- > 1 file changed, 38 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index a695888..fa02677 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4197,11 +4197,46 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc, > pipe_config->pipe_bpp <= 24; > } > > +static int intel_check_mode(struct drm_crtc *crtc, > + const struct drm_display_mode *mode) > +{ > + struct drm_device *dev = crtc->dev; > + > + /* > + * Cantiga+ cannot handle modes with a hsync front porch of 0. > + * WaPruneModeWithIncorrectHsyncOffset:ctg,elk,ilk,snb,ivb,vlv,hsw. > + */ > + if ((INTEL_INFO(dev)->gen > 4 || IS_G4X(dev)) && > + mode->crtc_hsync_start == mode->crtc_hdisplay) > + return -EINVAL; > + > + if (IS_HASWELL(dev)) { > + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI) && > + mode->crtc_hdisplay < 256) > + return -EINVAL; > + > + if (mode->crtc_hdisplay < 64 || mode->crtc_vdisplay < 7) > + return -EINVAL; > + } else if (INTEL_INFO(dev)->gen >= 5) { > + if (mode->crtc_hdisplay < 64 || mode->crtc_vdisplay < 7) > + return -EINVAL; > + } else if (INTEL_INFO(dev)->gen >= 4) { > + if (mode->crtc_hdisplay < 3 || mode->crtc_vdisplay < 3) > + return -EINVAL; > + } else { I checked all specs and all values above are ok. > + if (mode->crtc_hdisplay < 2 || mode->crtc_vdisplay < 2) Just couldn't understand why this 2 lines as minimun here. Even on 965_g35 they are 3. But maybe I'm missing something so, with this fixed or explained feel free to use: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > + return -EINVAL; > + } > + > + return 0; > +} > + > static int intel_crtc_compute_config(struct intel_crtc *crtc, > struct intel_crtc_config *pipe_config) > { > struct drm_device *dev = crtc->base.dev; > struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode; > + int ret; > > /* FIXME should check pixel clock limits on all platforms */ > if (INTEL_INFO(dev)->gen < 4) { > @@ -4236,12 +4271,9 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, > intel_is_dual_link_lvds(dev)) || pipe_config->double_wide) > pipe_config->pipe_src_w &= ~1; > > - /* Cantiga+ cannot handle modes with a hsync front porch of 0. > - * WaPruneModeWithIncorrectHsyncOffset:ctg,elk,ilk,snb,ivb,vlv,hsw. > - */ > - if ((INTEL_INFO(dev)->gen > 4 || IS_G4X(dev)) && > - adjusted_mode->hsync_start == adjusted_mode->hdisplay) > - return -EINVAL; > + ret = intel_check_mode(&crtc->base, adjusted_mode); > + if (ret) > + return ret; > > if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) && pipe_config->pipe_bpp > 10*3) { > pipe_config->pipe_bpp = 10*3; /* 12bpc is gen5+ */ > -- > 1.8.1.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Rodrigo Vivi Blog: http://blog.vivi.eng.br ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] drm/i915: Reject modes where hdisplay or vdisplay is too small 2013-10-01 20:08 ` Rodrigo Vivi @ 2013-10-01 20:55 ` Ville Syrjälä 2013-10-01 21:08 ` Rodrigo Vivi 0 siblings, 1 reply; 14+ messages in thread From: Ville Syrjälä @ 2013-10-01 20:55 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx On Tue, Oct 01, 2013 at 05:08:15PM -0300, Rodrigo Vivi wrote: > On Tue, Oct 1, 2013 at 10:13 AM, <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Check the adjusted_mode hdisplay/vdisplay against the limits from BSpec. > > > > Move the ctg+ zero front porch check to the same function, and change it > > to use the crtc_ values as well. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++++++++++++++++----- > > 1 file changed, 38 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index a695888..fa02677 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4197,11 +4197,46 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc, > > pipe_config->pipe_bpp <= 24; > > } > > > > +static int intel_check_mode(struct drm_crtc *crtc, > > + const struct drm_display_mode *mode) > > +{ > > + struct drm_device *dev = crtc->dev; > > + > > + /* > > + * Cantiga+ cannot handle modes with a hsync front porch of 0. > > + * WaPruneModeWithIncorrectHsyncOffset:ctg,elk,ilk,snb,ivb,vlv,hsw. > > + */ > > + if ((INTEL_INFO(dev)->gen > 4 || IS_G4X(dev)) && > > + mode->crtc_hsync_start == mode->crtc_hdisplay) > > + return -EINVAL; > > + > > + if (IS_HASWELL(dev)) { > > + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI) && > > + mode->crtc_hdisplay < 256) > > + return -EINVAL; > > + > > + if (mode->crtc_hdisplay < 64 || mode->crtc_vdisplay < 7) > > + return -EINVAL; > > + } else if (INTEL_INFO(dev)->gen >= 5) { > > + if (mode->crtc_hdisplay < 64 || mode->crtc_vdisplay < 7) > > + return -EINVAL; > > + } else if (INTEL_INFO(dev)->gen >= 4) { > > + if (mode->crtc_hdisplay < 3 || mode->crtc_vdisplay < 3) > > + return -EINVAL; > > + } else { > > I checked all specs and all values above are ok. > > > + if (mode->crtc_hdisplay < 2 || mode->crtc_vdisplay < 2) > > Just couldn't understand why this 2 lines as minimun here. > Even on 965_g35 they are 3. 965 is gen4. G35 (BLB) is gen3(ish). The internal bspec lists the minimum size as 2 for both gen2 and gen3. > > But maybe I'm missing something so, with this fixed or explained feel > free to use: > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > > > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > static int intel_crtc_compute_config(struct intel_crtc *crtc, > > struct intel_crtc_config *pipe_config) > > { > > struct drm_device *dev = crtc->base.dev; > > struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode; > > + int ret; > > > > /* FIXME should check pixel clock limits on all platforms */ > > if (INTEL_INFO(dev)->gen < 4) { > > @@ -4236,12 +4271,9 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, > > intel_is_dual_link_lvds(dev)) || pipe_config->double_wide) > > pipe_config->pipe_src_w &= ~1; > > > > - /* Cantiga+ cannot handle modes with a hsync front porch of 0. > > - * WaPruneModeWithIncorrectHsyncOffset:ctg,elk,ilk,snb,ivb,vlv,hsw. > > - */ > > - if ((INTEL_INFO(dev)->gen > 4 || IS_G4X(dev)) && > > - adjusted_mode->hsync_start == adjusted_mode->hdisplay) > > - return -EINVAL; > > + ret = intel_check_mode(&crtc->base, adjusted_mode); > > + if (ret) > > + return ret; > > > > if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) && pipe_config->pipe_bpp > 10*3) { > > pipe_config->pipe_bpp = 10*3; /* 12bpc is gen5+ */ > > -- > > 1.8.1.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] drm/i915: Reject modes where hdisplay or vdisplay is too small 2013-10-01 20:55 ` Ville Syrjälä @ 2013-10-01 21:08 ` Rodrigo Vivi 0 siblings, 0 replies; 14+ messages in thread From: Rodrigo Vivi @ 2013-10-01 21:08 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Tue, Oct 1, 2013 at 5:55 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Oct 01, 2013 at 05:08:15PM -0300, Rodrigo Vivi wrote: >> On Tue, Oct 1, 2013 at 10:13 AM, <ville.syrjala@linux.intel.com> wrote: >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> > Check the adjusted_mode hdisplay/vdisplay against the limits from BSpec. >> > >> > Move the ctg+ zero front porch check to the same function, and change it >> > to use the crtc_ values as well. >> > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++++++++++++++++----- >> > 1 file changed, 38 insertions(+), 6 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> > index a695888..fa02677 100644 >> > --- a/drivers/gpu/drm/i915/intel_display.c >> > +++ b/drivers/gpu/drm/i915/intel_display.c >> > @@ -4197,11 +4197,46 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc, >> > pipe_config->pipe_bpp <= 24; >> > } >> > >> > +static int intel_check_mode(struct drm_crtc *crtc, >> > + const struct drm_display_mode *mode) >> > +{ >> > + struct drm_device *dev = crtc->dev; >> > + >> > + /* >> > + * Cantiga+ cannot handle modes with a hsync front porch of 0. >> > + * WaPruneModeWithIncorrectHsyncOffset:ctg,elk,ilk,snb,ivb,vlv,hsw. >> > + */ >> > + if ((INTEL_INFO(dev)->gen > 4 || IS_G4X(dev)) && >> > + mode->crtc_hsync_start == mode->crtc_hdisplay) >> > + return -EINVAL; >> > + >> > + if (IS_HASWELL(dev)) { >> > + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI) && >> > + mode->crtc_hdisplay < 256) >> > + return -EINVAL; >> > + >> > + if (mode->crtc_hdisplay < 64 || mode->crtc_vdisplay < 7) >> > + return -EINVAL; >> > + } else if (INTEL_INFO(dev)->gen >= 5) { >> > + if (mode->crtc_hdisplay < 64 || mode->crtc_vdisplay < 7) >> > + return -EINVAL; >> > + } else if (INTEL_INFO(dev)->gen >= 4) { >> > + if (mode->crtc_hdisplay < 3 || mode->crtc_vdisplay < 3) >> > + return -EINVAL; >> > + } else { >> >> I checked all specs and all values above are ok. >> >> > + if (mode->crtc_hdisplay < 2 || mode->crtc_vdisplay < 2) >> >> Just couldn't understand why this 2 lines as minimun here. >> Even on 965_g35 they are 3. > > 965 is gen4. G35 (BLB) is gen3(ish). The internal bspec lists the minimum > size as 2 for both gen2 and gen3. I couldn't see the old ones at main bspec page anymore so I used https://01.org/linuxgraphics/sites/default/files/documentation/965_g35_vol_3_display_registers_updated_1.pdf This is why I confused, but if internal says 2 for gen2 and gen3 it is ok for me ;) just rv-b! > >> >> But maybe I'm missing something so, with this fixed or explained feel >> free to use: >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >> >> >> > + return -EINVAL; >> > + } >> > + >> > + return 0; >> > +} >> > + >> > static int intel_crtc_compute_config(struct intel_crtc *crtc, >> > struct intel_crtc_config *pipe_config) >> > { >> > struct drm_device *dev = crtc->base.dev; >> > struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode; >> > + int ret; >> > >> > /* FIXME should check pixel clock limits on all platforms */ >> > if (INTEL_INFO(dev)->gen < 4) { >> > @@ -4236,12 +4271,9 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, >> > intel_is_dual_link_lvds(dev)) || pipe_config->double_wide) >> > pipe_config->pipe_src_w &= ~1; >> > >> > - /* Cantiga+ cannot handle modes with a hsync front porch of 0. >> > - * WaPruneModeWithIncorrectHsyncOffset:ctg,elk,ilk,snb,ivb,vlv,hsw. >> > - */ >> > - if ((INTEL_INFO(dev)->gen > 4 || IS_G4X(dev)) && >> > - adjusted_mode->hsync_start == adjusted_mode->hdisplay) >> > - return -EINVAL; >> > + ret = intel_check_mode(&crtc->base, adjusted_mode); >> > + if (ret) >> > + return ret; >> > >> > if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) && pipe_config->pipe_bpp > 10*3) { >> > pipe_config->pipe_bpp = 10*3; /* 12bpc is gen5+ */ >> > -- >> > 1.8.1.5 >> > >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> >> -- >> Rodrigo Vivi >> Blog: http://blog.vivi.eng.br > > -- > Ville Syrjälä > Intel OTC -- Rodrigo Vivi Blog: http://blog.vivi.eng.br ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] drm/915: Make sure pipe source size isn't zero 2013-10-01 13:13 [PATCH 0/4] drm/i915: Some sanity checks for modes ville.syrjala 2013-10-01 13:13 ` [PATCH 1/4] drm/i915: Don't populate pipe_src_{w, h} multiple times ville.syrjala 2013-10-01 13:13 ` [PATCH 2/4] drm/i915: Reject modes where hdisplay or vdisplay is too small ville.syrjala @ 2013-10-01 13:13 ` ville.syrjala 2013-10-01 13:13 ` [PATCH 4/4] drm/i915: Do some basic sanity adjustments on the user provided mode ville.syrjala 3 siblings, 0 replies; 14+ messages in thread From: ville.syrjala @ 2013-10-01 13:13 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> Check that pipe_src_{w,h} aren't zero. We can currently cause a div by zero due to the missing check, and the hardware wouldn't like it either. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fa02677..7f61cfb 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4271,6 +4271,9 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, intel_is_dual_link_lvds(dev)) || pipe_config->double_wide) pipe_config->pipe_src_w &= ~1; + if (pipe_config->pipe_src_w == 0 || pipe_config->pipe_src_h == 0) + return -EINVAL; + ret = intel_check_mode(&crtc->base, adjusted_mode); if (ret) return ret; -- 1.8.1.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/4] drm/i915: Do some basic sanity adjustments on the user provided mode 2013-10-01 13:13 [PATCH 0/4] drm/i915: Some sanity checks for modes ville.syrjala ` (2 preceding siblings ...) 2013-10-01 13:13 ` [PATCH 3/4] drm/915: Make sure pipe source size isn't zero ville.syrjala @ 2013-10-01 13:13 ` ville.syrjala 2013-10-01 19:06 ` Daniel Vetter 3 siblings, 1 reply; 14+ messages in thread From: ville.syrjala @ 2013-10-01 13:13 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> Make sure the active, blanking and sync portions of the timings are in the proper order. The contract with userspace basically is that we must not increase the active portion. For the rest we're more or less free to do as we please. A good rul IMO is that we only increase the non-active portions of the timings. We could do a lot more adjustment to make sure the timings meet all the minimum requirements of the hardware. But as the mimimums may depend on the output type and other details, we should perhaps do the adjustments at some later point. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++++++++-- 1 file changed, 58 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7f61cfb..48cad99 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8458,6 +8458,55 @@ static bool check_encoder_cloning(struct drm_crtc *crtc) return !(num_encoders > 1 && uncloneable_encoders); } +static void adjust_timings(struct drm_display_mode *mode, + int *val, int min, int clocks_per_unit) +{ + if (*val >= min) + return; + + /* maintain the same refresh rate */ + mode->clock += (min - *val) * clocks_per_unit; + + *val = min; +} + +static int intel_adjust_mode(struct drm_display_mode *mode) +{ + if (mode->clock == 0) { + DRM_DEBUG_KMS("bad pixel clock\n"); + drm_mode_debug_printmodeline(mode); + return -EINVAL; + } + + if (mode->hdisplay == 0 || mode->vdisplay == 0) { + DRM_DEBUG_KMS("bad hdisplay or vdisplay\n"); + drm_mode_debug_printmodeline(mode); + return -EINVAL; + } + + adjust_timings(mode, &mode->hsync_start, mode->hdisplay, 0); + adjust_timings(mode, &mode->hsync_end, mode->hsync_start, 0); + adjust_timings(mode, &mode->htotal, mode->hsync_end, mode->vtotal); + + adjust_timings(mode, &mode->vsync_start, mode->vdisplay, 0); + adjust_timings(mode, &mode->vsync_end, mode->vsync_start, 0); + + /* + * We must not increase the size of the active + * space area for frame packing modes. + */ + if ((mode->flags & DRM_MODE_FLAG_3D_MASK) == DRM_MODE_FLAG_3D_FRAME_PACKING) { + if (mode->vtotal < mode->vsync_end) { + DRM_DEBUG_KMS("refusing to adjust vtotal for frame packing mode\n"); + drm_mode_debug_printmodeline(mode); + return -EINVAL; + } + } else + adjust_timings(mode, &mode->vtotal, mode->vsync_end, mode->htotal); + + return 0; +} + static struct intel_crtc_config * intel_modeset_pipe_config(struct drm_crtc *crtc, struct drm_framebuffer *fb, @@ -8466,7 +8515,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct intel_encoder *encoder; struct intel_crtc_config *pipe_config; - int plane_bpp, ret = -EINVAL; + int plane_bpp, ret; bool retry = true; if (!check_encoder_cloning(crtc)) { @@ -8498,14 +8547,20 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, (DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC))) pipe_config->adjusted_mode.flags |= DRM_MODE_FLAG_NVSYNC; + ret = intel_adjust_mode(&pipe_config->adjusted_mode); + if (ret) + goto fail; + /* Compute a starting value for pipe_config->pipe_bpp taking the source * plane pixel format and any sink constraints into account. Returns the * source plane bpp so that dithering can be selected on mismatches * after encoders and crtc also have had their say. */ plane_bpp = compute_baseline_pipe_bpp(to_intel_crtc(crtc), fb, pipe_config); - if (plane_bpp < 0) + if (plane_bpp < 0) { + ret = -EINVAL; goto fail; + } /* Determine the real pipe dimensions */ drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE); @@ -8532,6 +8587,7 @@ encoder_retry: if (!(encoder->compute_config(encoder, pipe_config))) { DRM_DEBUG_KMS("Encoder config failure\n"); + ret = -EINVAL; goto fail; } } -- 1.8.1.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] drm/i915: Do some basic sanity adjustments on the user provided mode 2013-10-01 13:13 ` [PATCH 4/4] drm/i915: Do some basic sanity adjustments on the user provided mode ville.syrjala @ 2013-10-01 19:06 ` Daniel Vetter 2013-10-01 19:12 ` Ville Syrjälä 0 siblings, 1 reply; 14+ messages in thread From: Daniel Vetter @ 2013-10-01 19:06 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx On Tue, Oct 01, 2013 at 04:13:31PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Make sure the active, blanking and sync portions of the timings are in > the proper order. The contract with userspace basically is that we must > not increase the active portion. For the rest we're more or less free to > do as we please. A good rul IMO is that we only increase the non-active > portions of the timings. > > We could do a lot more adjustment to make sure the timings meet all > the minimum requirements of the hardware. But as the mimimums may depend > on the output type and other details, we should perhaps do the adjustments > at some later point. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Do we actually get such modes? I'd vote to just filter them from our ->get_modes callbacks and reject them here if at all possible ... -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 58 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 7f61cfb..48cad99 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8458,6 +8458,55 @@ static bool check_encoder_cloning(struct drm_crtc *crtc) > return !(num_encoders > 1 && uncloneable_encoders); > } > > +static void adjust_timings(struct drm_display_mode *mode, > + int *val, int min, int clocks_per_unit) > +{ > + if (*val >= min) > + return; > + > + /* maintain the same refresh rate */ > + mode->clock += (min - *val) * clocks_per_unit; > + > + *val = min; > +} > + > +static int intel_adjust_mode(struct drm_display_mode *mode) > +{ > + if (mode->clock == 0) { > + DRM_DEBUG_KMS("bad pixel clock\n"); > + drm_mode_debug_printmodeline(mode); > + return -EINVAL; > + } > + > + if (mode->hdisplay == 0 || mode->vdisplay == 0) { > + DRM_DEBUG_KMS("bad hdisplay or vdisplay\n"); > + drm_mode_debug_printmodeline(mode); > + return -EINVAL; > + } > + > + adjust_timings(mode, &mode->hsync_start, mode->hdisplay, 0); > + adjust_timings(mode, &mode->hsync_end, mode->hsync_start, 0); > + adjust_timings(mode, &mode->htotal, mode->hsync_end, mode->vtotal); > + > + adjust_timings(mode, &mode->vsync_start, mode->vdisplay, 0); > + adjust_timings(mode, &mode->vsync_end, mode->vsync_start, 0); > + > + /* > + * We must not increase the size of the active > + * space area for frame packing modes. > + */ > + if ((mode->flags & DRM_MODE_FLAG_3D_MASK) == DRM_MODE_FLAG_3D_FRAME_PACKING) { > + if (mode->vtotal < mode->vsync_end) { > + DRM_DEBUG_KMS("refusing to adjust vtotal for frame packing mode\n"); > + drm_mode_debug_printmodeline(mode); > + return -EINVAL; > + } > + } else > + adjust_timings(mode, &mode->vtotal, mode->vsync_end, mode->htotal); > + > + return 0; > +} > + > static struct intel_crtc_config * > intel_modeset_pipe_config(struct drm_crtc *crtc, > struct drm_framebuffer *fb, > @@ -8466,7 +8515,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > struct drm_device *dev = crtc->dev; > struct intel_encoder *encoder; > struct intel_crtc_config *pipe_config; > - int plane_bpp, ret = -EINVAL; > + int plane_bpp, ret; > bool retry = true; > > if (!check_encoder_cloning(crtc)) { > @@ -8498,14 +8547,20 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > (DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC))) > pipe_config->adjusted_mode.flags |= DRM_MODE_FLAG_NVSYNC; > > + ret = intel_adjust_mode(&pipe_config->adjusted_mode); > + if (ret) > + goto fail; > + > /* Compute a starting value for pipe_config->pipe_bpp taking the source > * plane pixel format and any sink constraints into account. Returns the > * source plane bpp so that dithering can be selected on mismatches > * after encoders and crtc also have had their say. */ > plane_bpp = compute_baseline_pipe_bpp(to_intel_crtc(crtc), > fb, pipe_config); > - if (plane_bpp < 0) > + if (plane_bpp < 0) { > + ret = -EINVAL; > goto fail; > + } > > /* Determine the real pipe dimensions */ > drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE); > @@ -8532,6 +8587,7 @@ encoder_retry: > > if (!(encoder->compute_config(encoder, pipe_config))) { > DRM_DEBUG_KMS("Encoder config failure\n"); > + ret = -EINVAL; > goto fail; > } > } > -- > 1.8.1.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] drm/i915: Do some basic sanity adjustments on the user provided mode 2013-10-01 19:06 ` Daniel Vetter @ 2013-10-01 19:12 ` Ville Syrjälä 0 siblings, 0 replies; 14+ messages in thread From: Ville Syrjälä @ 2013-10-01 19:12 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Tue, Oct 01, 2013 at 09:06:10PM +0200, Daniel Vetter wrote: > On Tue, Oct 01, 2013 at 04:13:31PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Make sure the active, blanking and sync portions of the timings are in > > the proper order. The contract with userspace basically is that we must > > not increase the active portion. For the rest we're more or less free to > > do as we please. A good rul IMO is that we only increase the non-active > > portions of the timings. > > > > We could do a lot more adjustment to make sure the timings meet all > > the minimum requirements of the hardware. But as the mimimums may depend > > on the output type and other details, we should perhaps do the adjustments > > at some later point. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Do we actually get such modes? > > I'd vote to just filter them from our ->get_modes callbacks and reject > them here if at all possible ... The user is free to feed us any kind of garbage. Doesn't matter which modes we list in the connector's mode list. > -Daniel > > > --- > > drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++++++++-- > > 1 file changed, 58 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 7f61cfb..48cad99 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -8458,6 +8458,55 @@ static bool check_encoder_cloning(struct drm_crtc *crtc) > > return !(num_encoders > 1 && uncloneable_encoders); > > } > > > > +static void adjust_timings(struct drm_display_mode *mode, > > + int *val, int min, int clocks_per_unit) > > +{ > > + if (*val >= min) > > + return; > > + > > + /* maintain the same refresh rate */ > > + mode->clock += (min - *val) * clocks_per_unit; > > + > > + *val = min; > > +} > > + > > +static int intel_adjust_mode(struct drm_display_mode *mode) > > +{ > > + if (mode->clock == 0) { > > + DRM_DEBUG_KMS("bad pixel clock\n"); > > + drm_mode_debug_printmodeline(mode); > > + return -EINVAL; > > + } > > + > > + if (mode->hdisplay == 0 || mode->vdisplay == 0) { > > + DRM_DEBUG_KMS("bad hdisplay or vdisplay\n"); > > + drm_mode_debug_printmodeline(mode); > > + return -EINVAL; > > + } > > + > > + adjust_timings(mode, &mode->hsync_start, mode->hdisplay, 0); > > + adjust_timings(mode, &mode->hsync_end, mode->hsync_start, 0); > > + adjust_timings(mode, &mode->htotal, mode->hsync_end, mode->vtotal); > > + > > + adjust_timings(mode, &mode->vsync_start, mode->vdisplay, 0); > > + adjust_timings(mode, &mode->vsync_end, mode->vsync_start, 0); > > + > > + /* > > + * We must not increase the size of the active > > + * space area for frame packing modes. > > + */ > > + if ((mode->flags & DRM_MODE_FLAG_3D_MASK) == DRM_MODE_FLAG_3D_FRAME_PACKING) { > > + if (mode->vtotal < mode->vsync_end) { > > + DRM_DEBUG_KMS("refusing to adjust vtotal for frame packing mode\n"); > > + drm_mode_debug_printmodeline(mode); > > + return -EINVAL; > > + } > > + } else > > + adjust_timings(mode, &mode->vtotal, mode->vsync_end, mode->htotal); > > + > > + return 0; > > +} > > + > > static struct intel_crtc_config * > > intel_modeset_pipe_config(struct drm_crtc *crtc, > > struct drm_framebuffer *fb, > > @@ -8466,7 +8515,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > > struct drm_device *dev = crtc->dev; > > struct intel_encoder *encoder; > > struct intel_crtc_config *pipe_config; > > - int plane_bpp, ret = -EINVAL; > > + int plane_bpp, ret; > > bool retry = true; > > > > if (!check_encoder_cloning(crtc)) { > > @@ -8498,14 +8547,20 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > > (DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC))) > > pipe_config->adjusted_mode.flags |= DRM_MODE_FLAG_NVSYNC; > > > > + ret = intel_adjust_mode(&pipe_config->adjusted_mode); > > + if (ret) > > + goto fail; > > + > > /* Compute a starting value for pipe_config->pipe_bpp taking the source > > * plane pixel format and any sink constraints into account. Returns the > > * source plane bpp so that dithering can be selected on mismatches > > * after encoders and crtc also have had their say. */ > > plane_bpp = compute_baseline_pipe_bpp(to_intel_crtc(crtc), > > fb, pipe_config); > > - if (plane_bpp < 0) > > + if (plane_bpp < 0) { > > + ret = -EINVAL; > > goto fail; > > + } > > > > /* Determine the real pipe dimensions */ > > drm_mode_set_crtcinfo(&pipe_config->adjusted_mode, CRTC_STEREO_DOUBLE); > > @@ -8532,6 +8587,7 @@ encoder_retry: > > > > if (!(encoder->compute_config(encoder, pipe_config))) { > > DRM_DEBUG_KMS("Encoder config failure\n"); > > + ret = -EINVAL; > > goto fail; > > } > > } > > -- > > 1.8.1.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-10-01 21:08 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-01 13:13 [PATCH 0/4] drm/i915: Some sanity checks for modes ville.syrjala
2013-10-01 13:13 ` [PATCH 1/4] drm/i915: Don't populate pipe_src_{w, h} multiple times ville.syrjala
2013-10-01 19:03 ` Daniel Vetter
2013-10-01 19:14 ` Ville Syrjälä
2013-10-01 19:52 ` [PATCH v2 " ville.syrjala
2013-10-01 20:03 ` Daniel Vetter
2013-10-01 13:13 ` [PATCH 2/4] drm/i915: Reject modes where hdisplay or vdisplay is too small ville.syrjala
2013-10-01 20:08 ` Rodrigo Vivi
2013-10-01 20:55 ` Ville Syrjälä
2013-10-01 21:08 ` Rodrigo Vivi
2013-10-01 13:13 ` [PATCH 3/4] drm/915: Make sure pipe source size isn't zero ville.syrjala
2013-10-01 13:13 ` [PATCH 4/4] drm/i915: Do some basic sanity adjustments on the user provided mode ville.syrjala
2013-10-01 19:06 ` Daniel Vetter
2013-10-01 19:12 ` Ville Syrjälä
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox