| Message ID | 1361491014-13888-4-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 22, 2013 at 12:56:47AM +0100, Daniel Vetter 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(-) > <snip> > @@ -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; This changes the behaviour a bit. Previously the flag was only set from the centering funcs, but now it's set always. Is that intentional? > > 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
Hi 2013/2/21 Daniel Vetter <daniel.vetter@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.vetter@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
On Tue, Feb 26, 2013 at 02:23:23PM -0300, Paulo Zanoni wrote: > Hi > > 2013/2/21 Daniel Vetter <daniel.vetter@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.vetter@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. The patch set is pretty old (well, a few months at least), and when I've started it I kinda expected it to not get merged right away. So I eshewed doing tree-wide refactorings if not required. The second reason is that it's almost exactly the same callback as before, the only difference is that the mode/adjusted_mode argumentes are grouped together into pipe_config. With a few things tacked on top. So documentation is "it's like mode_fixup, but better". Last but not least the current pipe configuration computation code is really hapzardous and there's a lot of interdependent stuff going on. So right now I think nothing short of just reading the entire modeset code is documentation enough. So I don't think adding documentation for this in the middle of the core reorg makes much sense. I've tried to add useful documentation for most of the new pipe_config attributes though. Can I bribe you with the promise that I'll supply a nice blog post and documenatation patches once the basic infrastructure has settled instead? Cheers, Daniel > > --- > > 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
On Fri, Feb 22, 2013 at 03:51:15PM +0200, Ville Syrjälä wrote: > On Fri, Feb 22, 2013 at 12:56:47AM +0100, Daniel Vetter 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(-) > > > <snip> > > @@ -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; > > This changes the behaviour a bit. Previously the flag was only set from > the centering funcs, but now it's set always. Is that intentional? The crtc code also calls drm_mode_set_crtcinfo(adjusted_mode, 0) if ->timings_set is false, so doesn't result in any behaviour change. Hence I've figure that fewer lines of code should be better. Want me to change it back or just add a bit of text to the commit message? E.g. "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." Cheers, Daniel > > > > > 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 > > -- > Ville Syrjälä > Intel OTC
On Sun, Mar 03, 2013 at 07:01:12PM +0100, Daniel Vetter wrote: > On Fri, Feb 22, 2013 at 03:51:15PM +0200, Ville Syrjälä wrote: > > On Fri, Feb 22, 2013 at 12:56:47AM +0100, Daniel Vetter 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(-) > > > > > <snip> > > > @@ -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; > > > > This changes the behaviour a bit. Previously the flag was only set from > > the centering funcs, but now it's set always. Is that intentional? > > The crtc code also calls drm_mode_set_crtcinfo(adjusted_mode, 0) if > ->timings_set is false, so doesn't result in any behaviour change. Hence > I've figure that fewer lines of code should be better. Want me to change > it back or just add a bit of text to the commit message? E.g. > > "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." Right, I should have actually read the code with some thought instead of blindly looking at the changes. I think amending the commit message with the proposed text would be enough.
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;
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(-)