[Intel-gfx] [PATCH 00/21] skylake display scalers

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, March 17, 2015 7:33 AM
> To: Konduru, Chandra
> Cc: intel-gfx at lists.freedesktop.org; Conselvan De Oliveira, Ander; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 00/21] skylake display scalers
> 
> 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.

Sure.

> 
> - 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.

I have written kms tests to cover, crtc pfit, primary plane scaling and
sprite scaling and some combinations. Will be sending out once I take 
care of the review feedback and test them one more time.

> 
> - 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.

I'm covering test cases but not using atomic. Agree that once atomic
is landed, scalers can be tested in atomic fashion.

> 
> 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch