| Message ID | 1354201179-14975-10-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi 2012/11/29 Daniel Vetter <daniel.vetter@ffwll.ch>: > I like abstraction and vfuncs, but only if they actually abstract anything. > In this case here they just obfuscate, so let's rip this stuff out. > > Aside: We really should move all the haswell stuff into it's own file ... Well, a few Kernels ago we did a nice effort to move a lot of code from intel_display.c to intel_pm.c. Now we're bringing it back... I'd like to understand what are the current plans regarding this. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 -- > drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++--- > drivers/gpu/drm/i915/intel_drv.h | 2 -- > drivers/gpu/drm/i915/intel_pm.c | 37 ------------------------------------ > 4 files changed, 28 insertions(+), 44 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a98cff0..ac52c75 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -269,8 +269,6 @@ struct drm_i915_display_funcs { > void (*update_wm)(struct drm_device *dev); > void (*update_sprite_wm)(struct drm_device *dev, int pipe, > uint32_t sprite_width, int pixel_size); > - void (*update_linetime_wm)(struct drm_device *dev, int pipe, > - struct drm_display_mode *mode); > void (*modeset_global_resources)(struct drm_device *dev); > int (*crtc_mode_set)(struct drm_crtc *crtc, > struct drm_display_mode *mode, > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 0a45d07..0c7e88d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5380,11 +5380,36 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > > ret = intel_pipe_set_base(crtc, x, y, fb); > > - intel_update_linetime_watermarks(dev, pipe, adjusted_mode); > - > return fdi_config_ok ? ret : -EINVAL; > } > > +static void > +haswell_update_linetime_wm(struct drm_device *dev, int pipe, > + struct drm_display_mode *mode) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + u32 temp; > + > + temp = I915_READ(PIPE_WM_LINETIME(pipe)); > + temp &= ~PIPE_WM_LINETIME_MASK; > + > + /* The WM are computed with base on how long it takes to fill a single > + * row at the given clock rate, multiplied by 8. > + * */ > + temp |= PIPE_WM_LINETIME_TIME( > + ((mode->crtc_hdisplay * 1000) / mode->clock) * 8); > + > + /* IPS watermarks are only used by pipe A, and are ignored by > + * pipes B and C. They are calculated similarly to the common > + * linetime values, except that we are using CD clock frequency > + * in MHz instead of pixel rate for the division. > + * > + * This is a placeholder for the IPS watermark calculation code. > + */ > + > + I915_WRITE(PIPE_WM_LINETIME(pipe), temp); > +} > + > static int haswell_crtc_mode_set(struct drm_crtc *crtc, > struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode, > @@ -5465,7 +5490,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, > > ret = intel_pipe_set_base(crtc, x, y, fb); > > - intel_update_linetime_watermarks(dev, pipe, adjusted_mode); > + haswell_update_linetime_wm(dev, pipe, adjusted_mode); > > return ret; > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 7ca7772..824efde 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -622,8 +622,6 @@ extern void intel_update_watermarks(struct drm_device *dev); > extern void intel_update_sprite_watermarks(struct drm_device *dev, int pipe, > uint32_t sprite_width, > int pixel_size); > -extern void intel_update_linetime_watermarks(struct drm_device *dev, int pipe, > - struct drm_display_mode *mode); > > extern unsigned long intel_gen4_compute_offset_xtiled(int *x, int *y, > unsigned int bpp, > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index f595b8d..a487d15 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -1890,33 +1890,6 @@ static void sandybridge_update_wm(struct drm_device *dev) > cursor_wm); > } > > -static void > -haswell_update_linetime_wm(struct drm_device *dev, int pipe, > - struct drm_display_mode *mode) > -{ > - struct drm_i915_private *dev_priv = dev->dev_private; > - u32 temp; > - > - temp = I915_READ(PIPE_WM_LINETIME(pipe)); > - temp &= ~PIPE_WM_LINETIME_MASK; > - > - /* The WM are computed with base on how long it takes to fill a single > - * row at the given clock rate, multiplied by 8. > - * */ > - temp |= PIPE_WM_LINETIME_TIME( > - ((mode->crtc_hdisplay * 1000) / mode->clock) * 8); > - > - /* IPS watermarks are only used by pipe A, and are ignored by > - * pipes B and C. They are calculated similarly to the common > - * linetime values, except that we are using CD clock frequency > - * in MHz instead of pixel rate for the division. > - * > - * This is a placeholder for the IPS watermark calculation code. > - */ > - > - I915_WRITE(PIPE_WM_LINETIME(pipe), temp); > -} > - > static bool > sandybridge_compute_sprite_wm(struct drm_device *dev, int plane, > uint32_t sprite_width, int pixel_size, > @@ -2112,15 +2085,6 @@ void intel_update_watermarks(struct drm_device *dev) > dev_priv->display.update_wm(dev); > } > > -void intel_update_linetime_watermarks(struct drm_device *dev, > - int pipe, struct drm_display_mode *mode) > -{ > - struct drm_i915_private *dev_priv = dev->dev_private; > - > - if (dev_priv->display.update_linetime_wm) > - dev_priv->display.update_linetime_wm(dev, pipe, mode); > -} > - > void intel_update_sprite_watermarks(struct drm_device *dev, int pipe, > uint32_t sprite_width, int pixel_size) > { > @@ -4011,7 +3975,6 @@ void intel_init_pm(struct drm_device *dev) > if (SNB_READ_WM0_LATENCY()) { > dev_priv->display.update_wm = sandybridge_update_wm; > dev_priv->display.update_sprite_wm = sandybridge_update_sprite_wm; > - dev_priv->display.update_linetime_wm = haswell_update_linetime_wm; > } else { > DRM_DEBUG_KMS("Failed to read display plane latency. " > "Disable CxSR\n"); > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Dec 04, 2012 at 03:34:09PM -0200, Paulo Zanoni wrote: > Hi > > 2012/11/29 Daniel Vetter <daniel.vetter@ffwll.ch>: > > I like abstraction and vfuncs, but only if they actually abstract anything. > > In this case here they just obfuscate, so let's rip this stuff out. > > > > Aside: We really should move all the haswell stuff into it's own file ... > > Well, a few Kernels ago we did a nice effort to move a lot of code > from intel_display.c to intel_pm.c. Now we're bringing it back... I'd > like to understand what are the current plans regarding this. I guess we should move all the haswell stuff into intel_ddi.c, once it settles. But I'll drop the watermark patches here anyway, not thought through these here ... -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a98cff0..ac52c75 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -269,8 +269,6 @@ struct drm_i915_display_funcs { void (*update_wm)(struct drm_device *dev); void (*update_sprite_wm)(struct drm_device *dev, int pipe, uint32_t sprite_width, int pixel_size); - void (*update_linetime_wm)(struct drm_device *dev, int pipe, - struct drm_display_mode *mode); void (*modeset_global_resources)(struct drm_device *dev); int (*crtc_mode_set)(struct drm_crtc *crtc, struct drm_display_mode *mode, diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0a45d07..0c7e88d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5380,11 +5380,36 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_linetime_watermarks(dev, pipe, adjusted_mode); - return fdi_config_ok ? ret : -EINVAL; } +static void +haswell_update_linetime_wm(struct drm_device *dev, int pipe, + struct drm_display_mode *mode) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + u32 temp; + + temp = I915_READ(PIPE_WM_LINETIME(pipe)); + temp &= ~PIPE_WM_LINETIME_MASK; + + /* The WM are computed with base on how long it takes to fill a single + * row at the given clock rate, multiplied by 8. + * */ + temp |= PIPE_WM_LINETIME_TIME( + ((mode->crtc_hdisplay * 1000) / mode->clock) * 8); + + /* IPS watermarks are only used by pipe A, and are ignored by + * pipes B and C. They are calculated similarly to the common + * linetime values, except that we are using CD clock frequency + * in MHz instead of pixel rate for the division. + * + * This is a placeholder for the IPS watermark calculation code. + */ + + I915_WRITE(PIPE_WM_LINETIME(pipe), temp); +} + static int haswell_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode, @@ -5465,7 +5490,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_linetime_watermarks(dev, pipe, adjusted_mode); + haswell_update_linetime_wm(dev, pipe, adjusted_mode); return ret; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 7ca7772..824efde 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -622,8 +622,6 @@ extern void intel_update_watermarks(struct drm_device *dev); extern void intel_update_sprite_watermarks(struct drm_device *dev, int pipe, uint32_t sprite_width, int pixel_size); -extern void intel_update_linetime_watermarks(struct drm_device *dev, int pipe, - struct drm_display_mode *mode); extern unsigned long intel_gen4_compute_offset_xtiled(int *x, int *y, unsigned int bpp, diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index f595b8d..a487d15 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -1890,33 +1890,6 @@ static void sandybridge_update_wm(struct drm_device *dev) cursor_wm); } -static void -haswell_update_linetime_wm(struct drm_device *dev, int pipe, - struct drm_display_mode *mode) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - u32 temp; - - temp = I915_READ(PIPE_WM_LINETIME(pipe)); - temp &= ~PIPE_WM_LINETIME_MASK; - - /* The WM are computed with base on how long it takes to fill a single - * row at the given clock rate, multiplied by 8. - * */ - temp |= PIPE_WM_LINETIME_TIME( - ((mode->crtc_hdisplay * 1000) / mode->clock) * 8); - - /* IPS watermarks are only used by pipe A, and are ignored by - * pipes B and C. They are calculated similarly to the common - * linetime values, except that we are using CD clock frequency - * in MHz instead of pixel rate for the division. - * - * This is a placeholder for the IPS watermark calculation code. - */ - - I915_WRITE(PIPE_WM_LINETIME(pipe), temp); -} - static bool sandybridge_compute_sprite_wm(struct drm_device *dev, int plane, uint32_t sprite_width, int pixel_size, @@ -2112,15 +2085,6 @@ void intel_update_watermarks(struct drm_device *dev) dev_priv->display.update_wm(dev); } -void intel_update_linetime_watermarks(struct drm_device *dev, - int pipe, struct drm_display_mode *mode) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - - if (dev_priv->display.update_linetime_wm) - dev_priv->display.update_linetime_wm(dev, pipe, mode); -} - void intel_update_sprite_watermarks(struct drm_device *dev, int pipe, uint32_t sprite_width, int pixel_size) { @@ -4011,7 +3975,6 @@ void intel_init_pm(struct drm_device *dev) if (SNB_READ_WM0_LATENCY()) { dev_priv->display.update_wm = sandybridge_update_wm; dev_priv->display.update_sprite_wm = sandybridge_update_sprite_wm; - dev_priv->display.update_linetime_wm = haswell_update_linetime_wm; } else { DRM_DEBUG_KMS("Failed to read display plane latency. " "Disable CxSR\n");
I like abstraction and vfuncs, but only if they actually abstract anything. In this case here they just obfuscate, so let's rip this stuff out. Aside: We really should move all the haswell stuff into it's own file ... Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_drv.h | 2 -- drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_drv.h | 2 -- drivers/gpu/drm/i915/intel_pm.c | 37 ------------------------------------ 4 files changed, 28 insertions(+), 44 deletions(-)