On Mon, Jan 11, 2016 at 02:13:06PM -0800, Matt Roper wrote:
> On Mon, Jan 11, 2016 at 09:31:03PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 21, 2015 at 07:31:17AM -0800, Matt Roper wrote:
> > > In commit
> > >
> > > commit 024c9045221fe45482863c47c4b4c47d37f97cbf
> > > Author: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > Date: Thu Sep 24 15:53:11 2015 -0700
> > >
> > > drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4)
> > >
> > > I fumbled while converting the dimensions stored in the plane_parameters
> > > structure to the values stored in plane state and accidentally replaced
> > > the plane dimensions with the pipe dimensions in both the DDB allocation
> > > function and the WM calculation function. On the DDB side this is
> > > harmless since we effectively treat all of our non-cursor planes as
> > > full-screen which may not be optimal, but generally won't cause any
> > > problems either (and in 99% of the cases where there's no sprite plane
> > > usage or primary plane windowing, there's no effect at all). On the WM
> > > calculation side there's more potential for this fumble to cause actual
> > > problems since cursors also get miscalculated.
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxx>
> > > Cc: "Kondapally, Kalyan" <kalyan.kondapally@xxxxxxxxx>
> > > Cc: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_pm.c | 24 +++++++++++++-----------
> > > 1 file changed, 13 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 8d0d6f5..f4d4cc7 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -2845,25 +2845,22 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
> > > const struct drm_plane_state *pstate,
> > > int y)
> > > {
> > > - struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> > > + struct intel_plane_state *intel_pstate = to_intel_plane_state(pstate);
> > > struct drm_framebuffer *fb = pstate->fb;
> > > + unsigned w = drm_rect_width(&intel_pstate->dst);
> > > + unsigned h = drm_rect_height(&intel_pstate->dst);
> >
> > I think you're supposed to use the src dimensions in most places.
>
> Hmm, just went back to double check the bpsec and if I'm interpreting it
> correctly, it looks like we actually need to use the larger of the two:
> "Down scaling effectively increases the pixel rate. Up scaling does not
> reduce the pixel rate."
The pixel rate is "downscale factor * pixel clock"
>
> Thanks for pointing that out; I'll send an updated patch.
>
>
>
> Matt
>
> >
> > >
> > > /* for planar format */
> > > if (fb->pixel_format == DRM_FORMAT_NV12) {
> > > if (y) /* y-plane data rate */
> > > - return intel_crtc->config->pipe_src_w *
> > > - intel_crtc->config->pipe_src_h *
> > > - drm_format_plane_cpp(fb->pixel_format, 0);
> > > + return w * h * drm_format_plane_cpp(fb->pixel_format, 0);
> > > else /* uv-plane data rate */
> > > - return (intel_crtc->config->pipe_src_w/2) *
> > > - (intel_crtc->config->pipe_src_h/2) *
> > > + return (w/2) * (h/2) *
> > > drm_format_plane_cpp(fb->pixel_format, 1);
> > > }
> > >
> > > /* for packed formats */
> > > - return intel_crtc->config->pipe_src_w *
> > > - intel_crtc->config->pipe_src_h *
> > > - drm_format_plane_cpp(fb->pixel_format, 0);
> > > + return w * h * drm_format_plane_cpp(fb->pixel_format, 0);
> > > }
> > >
> > > /*
> > > @@ -2960,6 +2957,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> > > * FIXME: we may not allocate every single block here.
> > > */
> > > total_data_rate = skl_get_total_relative_data_rate(cstate);
> > > + if (!total_data_rate)
> > > + return;
> > >
> > > start = alloc->start;
> > > for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> > > @@ -3093,12 +3092,15 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > > {
> > > struct drm_plane *plane = &intel_plane->base;
> > > struct drm_framebuffer *fb = plane->state->fb;
> > > + struct intel_plane_state *intel_pstate =
> > > + to_intel_plane_state(plane->state);
> > > uint32_t latency = dev_priv->wm.skl_latency[level];
> > > uint32_t method1, method2;
> > > uint32_t plane_bytes_per_line, plane_blocks_per_line;
> > > uint32_t res_blocks, res_lines;
> > > uint32_t selected_result;
> > > uint8_t bytes_per_pixel;
> > > + unsigned w = drm_rect_width(&intel_pstate->dst);
> > >
> > > if (latency == 0 || !cstate->base.active || !fb)
> > > return false;
> > > @@ -3109,12 +3111,12 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > > latency);
> > > method2 = skl_wm_method2(skl_pipe_pixel_rate(cstate),
> > > cstate->base.adjusted_mode.crtc_htotal,
> > > - cstate->pipe_src_w,
> > > + w,
> > > bytes_per_pixel,
> > > fb->modifier[0],
> > > latency);
> > >
> > > - plane_bytes_per_line = cstate->pipe_src_w * bytes_per_pixel;
> > > + plane_bytes_per_line = w * bytes_per_pixel;
> > > plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> > >
> > > if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> > > --
> > > 2.1.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
