Re: [PATCH 00/21] skylake display scalers

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Mar 14, 2015 at 10:55:25PM -0700, Chandra Konduru wrote:
> This patch enables skylake display scalers in atomic framework.
> Skylake scalers are sharable within a pipe and can be used as a panel
> fitter or plane scaler. Two scalers cannot be ganged to a single plane
> to get higher scale factor but simultaneous use of one as plane scaler
> and one scaler as panel fitter is allowed. Reformatted previous patch
> series into smaller patches and addressed previous feedback inputs.
> Performed some initial testing and more testing is in works.
> Testing is done applying these patches on top of Ander's v2
> atomic crtc patches.
> As several atomic crtc is in flight, will revisit scalers and perform
> any additional testing after atomic crtc is in place.

Ok sprinkled a few smaller comments over a few patches, looks good overall
I think. And we discussed a bunch of the more tricky bits already in a 1:1
code walkthrough.

A few high-level bits:
- When splitting up patches pls dont make patches which just add
  functions/structs without using them. That makes it a lot harder to
  understand the big picture. The only exception is register #defines
  since reviewing those against Bspec can be done mostly independently of
  the code. So for functions make sure you have at least 1 use-site so
  that it's clear what it does. For structures just start out with the
  basic structure, then add members as you start using them.

  Since this amounts to a full rewrite of the patch series and this one
  here isn't too tricky I think it's ok to not do that here. But please
  follow this bkm for the next one.

- Testcase: We definitely don't have testcases for primary plane scaling
  (and panning fwiw), and iirc we also don't have any function testcases
  for sprite scaling.

- Testcases 2: I think we're also missing tests for the crtc pfit stuff.
  I think it'd be good to exercise the various resolutions and different
  boxing modes supported. I don't think we can do a full functional
  testcase here with crc.

- Unfortunately we can't yet test the sharing logic fully due to lack of
  atomic. But that's an open that we need to address once atomic has
  landed. Especially to make sure that extreme cases (e.g. all scalers
  used by planes <-> pfit enabled) work correctly.

Overall I think this is ready for detailed review. Since we lack igts I
recommend that the reviewer also does the igt coverage, that's a good way
to make sure all issues are caught.

Thanks, Daniel


> 
> Thanks,
> Chandra
> 
> Chandra Konduru (21):
>   drm/i915: Adding drm helper function drm_plane_from_index().
>   drm/i915: Register definitions for skylake scalers
>   drm/i915: Enable get_colorkey functions for primary plane.
>   drm/i915: skylake scaler structure definitions
>   drm/i915: Initialize skylake scalers
>   drm/i915: Dump scaler_state too as part of dumping crtc_state
>   drm/i915: Helper function to update skylake scaling ratio.
>   drm/i915: Add helper function to update scaler_users in crtc_state
>   drm/i915: Add atomic function to setup scalers scalers for a crtc.
>   drm/i915: Helper function to detach a scaler from a plane or crtc
>   drm/i915: Ensure planes begin with no scaler.
>   drm/i915: Ensure colorkey and scaling aren't enabled at same time
>   drm/i915: Preserve scaler state when clearing crtc_state
>   drm/i915: use current scaler state during readout_hw_state.
>   drm/i915: Update scaling ratio as part of crtc_compute_config
>   drm/i915: Ensure setting up scalers into staged crtc_state
>   drm/i915: copy staged scaler state from drm state to crtc->config.
>   drm/i915: stage panel fitting scaler request for fixed mode panel
>   drm/i915: Enable skylake panel fitting using skylake shared scalers
>   drm/i915: Enable skylake primary plane scaling using shared scalers
>   drm/i915: Enable skylake sprite plane scaling using shared scalers
> 
>  drivers/gpu/drm/drm_crtc.c           |   20 ++
>  drivers/gpu/drm/i915/i915_reg.h      |  114 +++++++++
>  drivers/gpu/drm/i915/intel_atomic.c  |  157 ++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  442 +++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_dp.c      |    7 +
>  drivers/gpu/drm/i915/intel_drv.h     |  109 +++++++++
>  drivers/gpu/drm/i915/intel_sprite.c  |   95 ++++++--
>  include/drm/drm_crtc.h               |    1 +
>  8 files changed, 895 insertions(+), 50 deletions(-)
> 
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx