Hi 2013/2/21 Daniel Vetter <daniel.vet...@ffwll.ch>: > Only used by the lvds encoder. Note that we shouldn't do the same > simple conversion with the FORCE_6BPC flag, since that's much better > handled by moving all the pipe_bpc computation around. > > Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>
In addition to Ville's question (which makes sense): This patch does 2 things: it adds pipe_config->timings_set, but it also adds the encoder->compute_config function. Even though I can see intel_encoder->compute_config is just like drm_encoder->mode_fixup but with different arguments, I don't think this is a trivial thing to notice (imagine a future code reader, not someone looking at this specific patch). It would be really nice if you could add the compute_config callback on a separate patch and give a detailed documentation of the expected behavior of the function for our future readers. Bonus points if you could also add documentation for all the other intel_encoder callbacks you've created and document their relationship with the drm callbacks. Also, if we agree to create a separate patch for the compute_config callbacks, why don't we just convert all encoders to use compute_config instead of mode_fixup in the very first patch? Looks simpler to me. Also, I notice that this patch series doesn't apply to drm-intel-next-queued, only to drm-intel-nightly. I'm not sure what's your plan regarding this, just noticing :) > --- > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > drivers/gpu/drm/i915/intel_drv.h | 10 ++++++---- > drivers/gpu/drm/i915/intel_lvds.c | 19 +++++++++---------- > 3 files changed, 26 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 0f61008..ad03b7f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3930,7 +3930,7 @@ static bool intel_crtc_compute_config(struct drm_crtc > *crtc, > /* All interlaced capable intel hw wants timings in frames. Note > though > * that intel_lvds_mode_fixup does some funny tricks with the crtc > * timings, so we need to be careful not to clobber these.*/ > - if (!(adjusted_mode->private_flags & INTEL_MODE_CRTC_TIMINGS_SET)) > + if (!pipe_config->timings_set) > drm_mode_set_crtcinfo(adjusted_mode, 0); > > /* WaPruneModeWithIncorrectHsyncOffset: Cantiga+ cannot handle modes > @@ -7509,6 +7509,16 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > > if (&encoder->new_crtc->base != crtc) > continue; > + > + if (encoder->compute_config) { > + if (!(encoder->compute_config(encoder, pipe_config))) > { > + DRM_DEBUG_KMS("Encoder config failure\n"); > + goto fail; > + } > + > + continue; > + } > + > encoder_funcs = encoder->base.helper_private; > if (!(encoder_funcs->mode_fixup(&encoder->base, > &pipe_config->requested_mode, > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index eca75b6..edafbef 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -105,10 +105,6 @@ > #define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0) > #define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << > INTEL_MODE_PIXEL_MULTIPLIER_SHIFT) > #define INTEL_MODE_DP_FORCE_6BPC (0x10) > -/* This flag must be set by the encoder's mode_fixup if it changes the crtc > - * timings in the mode to prevent the crtc fixup from overwriting them. > - * Currently only lvds needs that. */ > -#define INTEL_MODE_CRTC_TIMINGS_SET (0x20) > /* > * Set when limited 16-235 (as opposed to full 0-255) RGB color range is > * to be used. > @@ -158,6 +154,8 @@ struct intel_encoder { > bool cloneable; > bool connectors_active; > void (*hot_plug)(struct intel_encoder *); > + bool (*compute_config)(struct intel_encoder *, > + struct intel_crtc_config *); > void (*pre_pll_enable)(struct intel_encoder *); > void (*pre_enable)(struct intel_encoder *); > void (*enable)(struct intel_encoder *); > @@ -202,6 +200,10 @@ struct intel_connector { > struct intel_crtc_config { > struct drm_display_mode requested_mode; > struct drm_display_mode adjusted_mode; > + /* This flag must be set by the encoder's compute_config callback if > it > + * changes the crtc timings in the mode to prevent the crtc fixup from > + * overwriting them. Currently only lvds needs that. */ > + bool timings_set; > }; > > struct intel_crtc { > diff --git a/drivers/gpu/drm/i915/intel_lvds.c > b/drivers/gpu/drm/i915/intel_lvds.c > index 3d1d974..1616f53 100644 > --- a/drivers/gpu/drm/i915/intel_lvds.c > +++ b/drivers/gpu/drm/i915/intel_lvds.c > @@ -261,8 +261,6 @@ centre_horizontally(struct drm_display_mode *mode, > > mode->crtc_hsync_start = mode->crtc_hblank_start + sync_pos; > mode->crtc_hsync_end = mode->crtc_hsync_start + sync_width; > - > - mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET; > } > > static void > @@ -284,8 +282,6 @@ centre_vertically(struct drm_display_mode *mode, > > mode->crtc_vsync_start = mode->crtc_vblank_start + sync_pos; > mode->crtc_vsync_end = mode->crtc_vsync_start + sync_width; > - > - mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET; > } > > static inline u32 panel_fitter_scaling(u32 source, u32 target) > @@ -301,15 +297,17 @@ static inline u32 panel_fitter_scaling(u32 source, u32 > target) > return (FACTOR * ratio + FACTOR/2) / FACTOR; > } > > -static bool intel_lvds_mode_fixup(struct drm_encoder *encoder, > - const struct drm_display_mode *mode, > - struct drm_display_mode *adjusted_mode) > +static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder, > + struct intel_crtc_config *pipe_config) > { > - struct drm_device *dev = encoder->dev; > + struct drm_device *dev = intel_encoder->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(encoder); > + struct intel_lvds_encoder *lvds_encoder = > + to_lvds_encoder(&intel_encoder->base); > struct intel_connector *intel_connector = > &lvds_encoder->attached_connector->base; > + struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode; > + struct drm_display_mode *mode = &pipe_config->requested_mode; > struct intel_crtc *intel_crtc = lvds_encoder->base.new_crtc; > u32 pfit_control = 0, pfit_pgm_ratios = 0, border = 0; > int pipe; > @@ -359,6 +357,7 @@ static bool intel_lvds_mode_fixup(struct drm_encoder > *encoder, > I915_WRITE(BCLRPAT(pipe), 0); > > drm_mode_set_crtcinfo(adjusted_mode, 0); > + pipe_config->timings_set = true; > > switch (intel_connector->panel.fitting_mode) { > case DRM_MODE_SCALE_CENTER: > @@ -661,7 +660,6 @@ static int intel_lvds_set_property(struct drm_connector > *connector, > } > > static const struct drm_encoder_helper_funcs intel_lvds_helper_funcs = { > - .mode_fixup = intel_lvds_mode_fixup, > .mode_set = intel_lvds_mode_set, > }; > > @@ -1102,6 +1100,7 @@ bool intel_lvds_init(struct drm_device *dev) > intel_encoder->enable = intel_enable_lvds; > intel_encoder->pre_enable = intel_pre_enable_lvds; > intel_encoder->pre_pll_enable = intel_pre_pll_enable_lvds; > + intel_encoder->compute_config = intel_lvds_compute_config; > intel_encoder->disable = intel_disable_lvds; > intel_encoder->get_hw_state = intel_lvds_get_hw_state; > intel_connector->get_hw_state = intel_connector_get_hw_state; > -- > 1.7.11.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx