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@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
