| Message ID | 1364341502-1184-4-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 27 Mar 2013 00:44:52 +0100 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > 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.vetter@ffwll.ch> > --- > 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 56ff8a5..3e22305 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3970,7 +3970,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 > @@ -7532,6 +7532,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 4cc6625..054032a 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 *); > @@ -203,6 +201,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 6ff145f..a2c516c 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, > }; > > @@ -1105,6 +1103,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; Changelog and summary could be better and mention the new compute_config function and how it replaces the mode_fixup one. Also, TIMINGS_SET probably wasn't a very good name in the first place, since it really deals with letter/pillar box configs. But that's not really a problem with your patch and could be changed in a follow-on. Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On Wed, Mar 27, 2013 at 09:49:52AM -0700, Jesse Barnes wrote: > Changelog and summary could be better and mention the new > compute_config function and how it replaces the mode_fixup one. > > Also, TIMINGS_SET probably wasn't a very good name in the first place, > since it really deals with letter/pillar box configs. But that's not > really a problem with your patch and could be changed in a follow-on. > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> Well, I've missed to add a commit addition discussed with Ville: > > -- > Jesse Barnes, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 27 Mar 2013 00:44:52 +0100 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > + 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 *); > @@ -203,6 +201,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; The compute_config function could actually use some kdoc instead of putting it over the timings_set function. It'll need to be expanded to cover all the pipe_config bits eventually, what they mean and when they should be set.
On Wed, Mar 27, 2013 at 5:49 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > Changelog and summary could be better and mention the new > compute_config function and how it replaces the mode_fixup one. > > Also, TIMINGS_SET probably wasn't a very good name in the first place, > since it really deals with letter/pillar box configs. But that's not > really a problem with your patch and could be changed in a follow-on. > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> I've forgotten to add a commit addition discussed with Ville: "Note that since the lvds code unconditionally sets the crtc timings, we can also unconditionally set the respective flag and not just when we set special timings like the old code did." I'll smash another paragraph for you on top (which also address an issue raised by Paulo): "This requires that we pass the pipe config around to encoders, so that they can set special attributes and set constraints. To do so introduce a new ->compute_config encoder callback, which is called in stead of the drm crtc helper's ->mode_fixup. "To avoid massive churn all over the codebase we don't want to convert all existing ->mode_fixup functions. Instead I've opted to convert them on an as-needed basis (mostly to cut down on rebase conflicts and to have more freedom to experiment around while developing the patches)." Cheers, Daniel
On Wed, Mar 27, 2013 at 5:59 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> @@ -203,6 +201,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; > > The compute_config function could actually use some kdoc instead of > putting it over the timings_set function. It'll need to be expanded to > cover all the pipe_config bits eventually, what they mean and when they > should be set. Now I very much like to claim the opposite, but this isn't designed but very much organically grown code. So imo documentation doesn't make too much sense before things settle a bit more (the auto fdi link dither at the end will introduce quite a bit of fun still ...). I've promised though in my pipe_config intro a few weeks ago that I'll create a nice blog post and doc patch once the basic stuff is settled. I still intend to deliver on that. Is that good enough? -Daniel
On Wed, 27 Mar 2013 18:06:44 +0100 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Wed, Mar 27, 2013 at 5:59 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > >> @@ -203,6 +201,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; > > > > The compute_config function could actually use some kdoc instead of > > putting it over the timings_set function. It'll need to be expanded to > > cover all the pipe_config bits eventually, what they mean and when they > > should be set. > > Now I very much like to claim the opposite, but this isn't designed > but very much organically grown code. So imo documentation doesn't > make too much sense before things settle a bit more (the auto fdi link > dither at the end will introduce quite a bit of fun still ...). > > I've promised though in my pipe_config intro a few weeks ago that I'll > create a nice blog post and doc patch once the basic stuff is settled. > I still intend to deliver on that. Is that good enough? I guess so... incrementally adding to the compute_config kdoc with the new pipe_config bits as added is too much rebase pain?
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 56ff8a5..3e22305 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3970,7 +3970,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 @@ -7532,6 +7532,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 4cc6625..054032a 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 *); @@ -203,6 +201,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 6ff145f..a2c516c 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, }; @@ -1105,6 +1103,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;
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.vetter@ffwll.ch> --- 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(-)