| Message ID | 1352120854-16533-7-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi 2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>: > Both the dp and fdi code use the exact same computations (ignore minor > differences in conversion between bits and bytes). > > This makes it even more apparent that we have a _massive_ mess between > cpu transcoder/fdi link/pch transcoder and pch link settings. And also > that we have hilarious amounts of confusion between edp and dp > (despite that they're identical at a link level). > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> This is certainly a first improvement to the m_n code and doing the exact same thing as you did was even on my TODO list. I think there's a lot more to do, but this patch is certainly the first step. There is just one minor bikeshed below: > --- > drivers/gpu/drm/i915/i915_drv.h | 13 ++++++++++++ > drivers/gpu/drm/i915/intel_display.c | 31 +++++++++------------------- > drivers/gpu/drm/i915/intel_dp.c | 39 +++--------------------------------- > 3 files changed, 26 insertions(+), 57 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c4339c2..23cea8f 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -101,6 +101,19 @@ struct intel_pch_pll { > }; > #define I915_NUM_PLLS 2 > > +/* Used by dp and fdi links */ > +struct intel_link_m_n { > + uint32_t tu; > + uint32_t gmch_m; > + uint32_t gmch_n; > + uint32_t link_m; > + uint32_t link_n; > +}; > + > +void intel_link_compute_m_n(int bpp, int nlanes, > + int pixel_clock, int link_clock, > + struct intel_link_m_n *m_n); > + > struct intel_ddi_plls { > int spll_refcount; > int wrpll1_refcount; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 0c923a6..5432e9c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3956,16 +3956,8 @@ static int i830_get_display_clock_speed(struct drm_device *dev) > return 133000; > } > > -struct fdi_m_n { > - u32 tu; > - u32 gmch_m; > - u32 gmch_n; > - u32 link_m; > - u32 link_n; > -}; > - > static void > -fdi_reduce_ratio(u32 *num, u32 *den) > +intel_reduce_ratio(uint32_t *num, uint32_t *den) > { > while (*num > 0xffffff || *den > 0xffffff) { > *num >>= 1; > @@ -3973,20 +3965,18 @@ fdi_reduce_ratio(u32 *num, u32 *den) > } > } > > -static void > -ironlake_compute_m_n(int bits_per_pixel, int nlanes, int pixel_clock, > - int link_clock, struct fdi_m_n *m_n) > +void > +intel_link_compute_m_n(int bits_per_pixel, int nlanes, > + int pixel_clock, int link_clock, > + struct intel_link_m_n *m_n) > { > - m_n->tu = 64; /* default size */ > - > - /* BUG_ON(pixel_clock > INT_MAX / 36); */ > + m_n->tu = 64; > m_n->gmch_m = bits_per_pixel * pixel_clock; > m_n->gmch_n = link_clock * nlanes * 8; I think here we should keep the gmch_m and gmch_n calculations used in the DP code instead of this one, since it matches the spec a little better... The spec says that gmch_m should be "dot clock * bytes per pixel" and gmch_n should be "ls_clk * number of lanes". So the lines that are currently inside intel_dp_compute_m_n look better: m_n->gmch_m = (pixel_clock * bpp) >> 3; m_n->gmch_n = link_clock * nlanes; (Also, in this case we'll need even higher numbers before we overflow) I applied a version with the DP values locally and tested. So my version of this patch is Reviewed-by and Tested-by me. > - fdi_reduce_ratio(&m_n->gmch_m, &m_n->gmch_n); > - > + intel_reduce_ratio(&m_n->gmch_m, &m_n->gmch_n); > m_n->link_m = pixel_clock; > m_n->link_n = link_clock; > - fdi_reduce_ratio(&m_n->link_m, &m_n->link_n); > + intel_reduce_ratio(&m_n->link_m, &m_n->link_n); > } > > static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv) > @@ -5100,7 +5090,7 @@ static void ironlake_set_m_n(struct drm_crtc *crtc, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder; > struct intel_encoder *intel_encoder, *edp_encoder = NULL; > - struct fdi_m_n m_n = {0}; > + struct intel_link_m_n m_n = {0}; > int target_clock, pixel_multiplier, lane, link_bw; > bool is_dp = false, is_cpu_edp = false; > > @@ -5158,8 +5148,7 @@ static void ironlake_set_m_n(struct drm_crtc *crtc, > > if (pixel_multiplier > 1) > link_bw *= pixel_multiplier; > - ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw, > - &m_n); > + intel_link_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw, &m_n); > > I915_WRITE(PIPE_DATA_M1(cpu_transcoder), TU_SIZE(m_n.tu) | m_n.gmch_m); > I915_WRITE(PIPE_DATA_N1(cpu_transcoder), m_n.gmch_n); > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 8ec4d1c..5d8ae65 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -742,39 +742,6 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, > return false; > } > > -struct intel_dp_m_n { > - uint32_t tu; > - uint32_t gmch_m; > - uint32_t gmch_n; > - uint32_t link_m; > - uint32_t link_n; > -}; > - > -static void > -intel_reduce_ratio(uint32_t *num, uint32_t *den) > -{ > - while (*num > 0xffffff || *den > 0xffffff) { > - *num >>= 1; > - *den >>= 1; > - } > -} > - > -static void > -intel_dp_compute_m_n(int bpp, > - int nlanes, > - int pixel_clock, > - int link_clock, > - struct intel_dp_m_n *m_n) > -{ > - m_n->tu = 64; > - m_n->gmch_m = (pixel_clock * bpp) >> 3; > - m_n->gmch_n = link_clock * nlanes; > - intel_reduce_ratio(&m_n->gmch_m, &m_n->gmch_n); > - m_n->link_m = pixel_clock; > - m_n->link_n = link_clock; > - intel_reduce_ratio(&m_n->link_m, &m_n->link_n); > -} > - > void > intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode) > @@ -785,7 +752,7 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode, > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > int lane_count = 4; > - struct intel_dp_m_n m_n; > + struct intel_link_m_n m_n; > int pipe = intel_crtc->pipe; > enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder; > > @@ -808,8 +775,8 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode, > * the number of bytes_per_pixel post-LUT, which we always > * set up for 8-bits of R/G/B, or 3 bytes total. > */ > - intel_dp_compute_m_n(intel_crtc->bpp, lane_count, > - mode->clock, adjusted_mode->clock, &m_n); > + intel_link_compute_m_n(intel_crtc->bpp, lane_count, > + mode->clock, adjusted_mode->clock, &m_n); > > if (IS_HASWELL(dev)) { > I915_WRITE(PIPE_DATA_M1(cpu_transcoder), > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Nov 20, 2012 at 12:24 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > I think here we should keep the gmch_m and gmch_n calculations used in > the DP code instead of this one, since it matches the spec a little > better... The spec says that gmch_m should be "dot clock * bytes per > pixel" and gmch_n should be "ls_clk * number of lanes". So the lines > that are currently inside intel_dp_compute_m_n look better: > > m_n->gmch_m = (pixel_clock * bpp) >> 3; > m_n->gmch_n = link_clock * nlanes; > > (Also, in this case we'll need even higher numbers before we overflow) > > I applied a version with the DP values locally and tested. So my > version of this patch is Reviewed-by and Tested-by me. How exactly should dot_clock * bytes_per_pixel work for e.g. 6bpc? Note that the above essentially just does part of the ratio reduction intel_reduce does and doesn't gain us any headroom for overflows, since in both cases we compute pixel_clock * bpp. The only difference is that we don't compute link_clock *nlanes * 8, but since the data can (almost) fill the link bw, we don't really gain anything. So imo computing the ration of bits that flow through the link is much more natural, and trying to compute the ration as bytes only obfuscates things. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c4339c2..23cea8f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -101,6 +101,19 @@ struct intel_pch_pll { }; #define I915_NUM_PLLS 2 +/* Used by dp and fdi links */ +struct intel_link_m_n { + uint32_t tu; + uint32_t gmch_m; + uint32_t gmch_n; + uint32_t link_m; + uint32_t link_n; +}; + +void intel_link_compute_m_n(int bpp, int nlanes, + int pixel_clock, int link_clock, + struct intel_link_m_n *m_n); + struct intel_ddi_plls { int spll_refcount; int wrpll1_refcount; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0c923a6..5432e9c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3956,16 +3956,8 @@ static int i830_get_display_clock_speed(struct drm_device *dev) return 133000; } -struct fdi_m_n { - u32 tu; - u32 gmch_m; - u32 gmch_n; - u32 link_m; - u32 link_n; -}; - static void -fdi_reduce_ratio(u32 *num, u32 *den) +intel_reduce_ratio(uint32_t *num, uint32_t *den) { while (*num > 0xffffff || *den > 0xffffff) { *num >>= 1; @@ -3973,20 +3965,18 @@ fdi_reduce_ratio(u32 *num, u32 *den) } } -static void -ironlake_compute_m_n(int bits_per_pixel, int nlanes, int pixel_clock, - int link_clock, struct fdi_m_n *m_n) +void +intel_link_compute_m_n(int bits_per_pixel, int nlanes, + int pixel_clock, int link_clock, + struct intel_link_m_n *m_n) { - m_n->tu = 64; /* default size */ - - /* BUG_ON(pixel_clock > INT_MAX / 36); */ + m_n->tu = 64; m_n->gmch_m = bits_per_pixel * pixel_clock; m_n->gmch_n = link_clock * nlanes * 8; - fdi_reduce_ratio(&m_n->gmch_m, &m_n->gmch_n); - + intel_reduce_ratio(&m_n->gmch_m, &m_n->gmch_n); m_n->link_m = pixel_clock; m_n->link_n = link_clock; - fdi_reduce_ratio(&m_n->link_m, &m_n->link_n); + intel_reduce_ratio(&m_n->link_m, &m_n->link_n); } static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv) @@ -5100,7 +5090,7 @@ static void ironlake_set_m_n(struct drm_crtc *crtc, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder; struct intel_encoder *intel_encoder, *edp_encoder = NULL; - struct fdi_m_n m_n = {0}; + struct intel_link_m_n m_n = {0}; int target_clock, pixel_multiplier, lane, link_bw; bool is_dp = false, is_cpu_edp = false; @@ -5158,8 +5148,7 @@ static void ironlake_set_m_n(struct drm_crtc *crtc, if (pixel_multiplier > 1) link_bw *= pixel_multiplier; - ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw, - &m_n); + intel_link_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw, &m_n); I915_WRITE(PIPE_DATA_M1(cpu_transcoder), TU_SIZE(m_n.tu) | m_n.gmch_m); I915_WRITE(PIPE_DATA_N1(cpu_transcoder), m_n.gmch_n); diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 8ec4d1c..5d8ae65 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -742,39 +742,6 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, return false; } -struct intel_dp_m_n { - uint32_t tu; - uint32_t gmch_m; - uint32_t gmch_n; - uint32_t link_m; - uint32_t link_n; -}; - -static void -intel_reduce_ratio(uint32_t *num, uint32_t *den) -{ - while (*num > 0xffffff || *den > 0xffffff) { - *num >>= 1; - *den >>= 1; - } -} - -static void -intel_dp_compute_m_n(int bpp, - int nlanes, - int pixel_clock, - int link_clock, - struct intel_dp_m_n *m_n) -{ - m_n->tu = 64; - m_n->gmch_m = (pixel_clock * bpp) >> 3; - m_n->gmch_n = link_clock * nlanes; - intel_reduce_ratio(&m_n->gmch_m, &m_n->gmch_n); - m_n->link_m = pixel_clock; - m_n->link_n = link_clock; - intel_reduce_ratio(&m_n->link_m, &m_n->link_n); -} - void intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode) @@ -785,7 +752,7 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); int lane_count = 4; - struct intel_dp_m_n m_n; + struct intel_link_m_n m_n; int pipe = intel_crtc->pipe; enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder; @@ -808,8 +775,8 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode, * the number of bytes_per_pixel post-LUT, which we always * set up for 8-bits of R/G/B, or 3 bytes total. */ - intel_dp_compute_m_n(intel_crtc->bpp, lane_count, - mode->clock, adjusted_mode->clock, &m_n); + intel_link_compute_m_n(intel_crtc->bpp, lane_count, + mode->clock, adjusted_mode->clock, &m_n); if (IS_HASWELL(dev)) { I915_WRITE(PIPE_DATA_M1(cpu_transcoder),
Both the dp and fdi code use the exact same computations (ignore minor differences in conversion between bits and bytes). This makes it even more apparent that we have a _massive_ mess between cpu transcoder/fdi link/pch transcoder and pch link settings. And also that we have hilarious amounts of confusion between edp and dp (despite that they're identical at a link level). Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_drv.h | 13 ++++++++++++ drivers/gpu/drm/i915/intel_display.c | 31 +++++++++------------------- drivers/gpu/drm/i915/intel_dp.c | 39 +++--------------------------------- 3 files changed, 26 insertions(+), 57 deletions(-)