> -----Original Message-----
> From: Roper, Matthew D
> Sent: Thursday, April 09, 2015 2:51 PM
> To: Konduru, Chandra
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vetter, Daniel; Conselvan De Oliveira, Ander
> Subject: Re: [PATCH 09/14] drm/i915: setup scalers for crtc_compute_config
>
> On Tue, Apr 07, 2015 at 03:28:42PM -0700, Chandra Konduru wrote:
> > Added intel_atomic_setup_scalers to setup scalers based on staged
> > scaling requests from a crtc and its planes. If staged requests are
> > supportable, this function assigns scalers to requested planes and
> > crtc. Note that the scaler assignement itself is staged into
> > crtc_state and respective plane_states for later commit after all
> > checks have been done.
> >
> > overall high level flow:
> > - scaler requests are staged into crtc_state by planes/crtc
> > - check whether staged scaling requests can be supported
> > - add planes using scalers that aren't in current transaction
> > - assign scalers to requested users
> > - as part of plane commit, scalers will be committed
> > (i.e., either attached or detached) to respective planes in hw
> > - as part of crtc_commit, scaler will be either attached or detached
> > to crtc in hw
> >
> > crtc_compute_config calls intel_atomic_setup_scalers() to start scaler
> > assignments as per scaler state in crtc config. This call should be
> > moved to atomic crtc once it is available.
> >
> > v2:
> > -removed a log message (me)
> > -changed input parameter to crtc_state (me)
> >
> > v3:
> > -remove assigning plane_state returned by drm_atomic_get_plane_state
> > (Matt) -fail if there is an error from drm_atomic_get_plane_state
> > (Matt)
> >
> > v4:
> > -changes to align with updated scaler structure (Matt, me)
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_atomic.c | 137
> ++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_display.c | 10 ++-
> > drivers/gpu/drm/i915/intel_drv.h | 3 +
> > 3 files changed, 149 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > b/drivers/gpu/drm/i915/intel_atomic.c
> > index 3903b90..ac317b4 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -241,3 +241,140 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> > {
> > drm_atomic_helper_crtc_destroy_state(crtc, state); }
> > +
> > +/**
> > + * intel_atomic_setup_scalers() - setup scalers for crtc per staged
> > +requests
> > + * @dev: DRM device
> > + * @crtc: intel crtc
> > + * @crtc_state: incoming crtc_state to validate and setup scalers
> > + *
> > + * This function sets up scalers based on staged scaling requests for
> > + * a @crtc and its planes. It is called from crtc level check path.
> > +If request
> > + * is a supportable request, it attaches scalers to requested planes and crtc.
> > + *
> > + * This function takes into account the current scaler(s) in use by
> > +any planes
> > + * not being part of this atomic state
> > + *
> > + * Returns:
> > + * 0 - scalers were setup succesfully
> > + * error code - otherwise
> > + */
> > +int intel_atomic_setup_scalers(struct drm_device *dev,
> > + struct intel_crtc *intel_crtc,
> > + struct intel_crtc_state *crtc_state) {
> > + struct drm_plane *plane = NULL;
> > + struct intel_plane *intel_plane;
> > + struct intel_plane_state *plane_state = NULL;
> > + struct intel_crtc_scaler_state *scaler_state;
> > + struct drm_atomic_state *drm_state;
> > + int num_scalers_need;
> > + int i, j;
> > +
> > + if (INTEL_INFO(dev)->gen < 9 || !intel_crtc || !crtc_state)
> > + return 0;
> > +
> > + scaler_state = &crtc_state->scaler_state;
> > + drm_state = crtc_state->base.state;
> > +
> > + num_scalers_need = hweight32(scaler_state->scaler_users);
> > + DRM_DEBUG_KMS("crtc_state = %p need = %d avail = %d scaler_users =
> 0x%x\n",
> > + crtc_state, num_scalers_need, intel_crtc->num_scalers,
> > + scaler_state->scaler_users);
> > +
> > + /*
> > + * High level flow:
> > + * - staged scaler requests are already in scaler_state->scaler_users
> > + * - check whether staged scaling requests can be supported
> > + * - add planes using scalers that aren't in current transaction
> > + * - assign scalers to requested users
> > + * - as part of plane commit, scalers will be committed
> > + * (i.e., either attached or detached) to respective planes in hw
> > + * - as part of crtc_commit, scaler will be either attached or detached
> > + * to crtc in hw
> > + */
> > +
> > + /* fail if required scalers > available scalers */
> > + if (num_scalers_need > intel_crtc->num_scalers){
> > + DRM_DEBUG_KMS("Too many scaling requests %d > %d\n",
> > + num_scalers_need, intel_crtc->num_scalers);
> > + return -EINVAL;
> > + }
> > +
> > + /* walkthrough scaler_users bits and start assigning scalers */
> > + for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) {
> > + int *scaler_id;
> > +
> > + /* skip if scaler not required */
> > + if (!(scaler_state->scaler_users & (1 << i)))
> > + continue;
> > +
> > + if (i == SKL_CRTC_INDEX) {
> > + /* panel fitter case: assign as a crtc scaler */
> > + scaler_id = &scaler_state->scaler_id;
> > + } else {
> > + if (!drm_state)
> > + continue;
> > +
> > + /* plane scaler case: assign as a plane scaler */
> > + /* find the plane that set the bit as scaler_user */
> > + plane = drm_state->planes[i];
> > +
> > + /*
> > + * to enable/disable hq mode, add planes that are using
> scaler
> > + * into this transaction
> > + */
>
> The code here looks fine, but the reference to "hq mode" kind of comes out of
> the blue. I realize you're setting HQ or DYN down at the bottom of this function,
> but there's never really any indication of what those are or what they mean (or
> how having the other plane states in the top-level transaction relates to HQ
> mode). I'm assuming what your comment here is trying to say is that the mode
> for an already setup and unchanged plane+scaler might have to change due to
> changes to the total number of scalers overall, right?
>
> So maybe just add a paragraph to the commit message explaining the HQ stuff a
> little bit? I think the code itself should be fine so you can consider it
>
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>
> after updating the commit message.
OK
>
>
> Matt
>
> > + if (!plane) {
> > + struct drm_plane_state *state;
> > + plane = drm_plane_from_index(dev, i);
> > + state =
> drm_atomic_get_plane_state(drm_state, plane);
> > + if (IS_ERR(state)) {
> > + DRM_DEBUG_KMS("Failed to add
> [PLANE:%d] to drm_state\n",
> > + plane->base.id);
> > + return PTR_ERR(state);
> > + }
> > + }
> > +
> > + intel_plane = to_intel_plane(plane);
> > +
> > + /* plane on different crtc cannot be a scaler user of this
> crtc */
> > + if (WARN_ON(intel_plane->pipe != intel_crtc->pipe)) {
> > + continue;
> > + }
> > +
> > + plane_state = to_intel_plane_state(drm_state-
> >plane_states[i]);
> > + scaler_id = &plane_state->scaler_id;
> > + }
> > +
> > + if (*scaler_id < 0) {
> > + /* find a free scaler */
> > + for (j = 0; j < intel_crtc->num_scalers; j++) {
> > + if (!scaler_state->scalers[j].in_use) {
> > + scaler_state->scalers[j].in_use = 1;
> > + *scaler_id = scaler_state->scalers[j].id;
> > + DRM_DEBUG_KMS("Attached scaler id
> %u.%u to %s:%d\n",
> > + intel_crtc->pipe,
> > + i == SKL_CRTC_INDEX ?
> scaler_state->scaler_id :
> > + plane_state-
> >scaler_id,
> > + i == SKL_CRTC_INDEX ? "CRTC"
> : "PLANE",
> > + i == SKL_CRTC_INDEX ?
> intel_crtc->base.base.id :
> > + plane->base.id);
> > + break;
> > + }
> > + }
> > + }
> > +
> > + if (WARN_ON(*scaler_id < 0)) {
> > + DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n",
> > + i == SKL_CRTC_INDEX ? "CRTC" : "PLANE",
> > + i == SKL_CRTC_INDEX ? intel_crtc-
> >base.base.id:plane->base.id);
> > + continue;
> > + }
> > +
> > + /* set scaler mode */
> > + scaler_state->scalers[*scaler_id].mode = (num_scalers_need ==
> 1) ?
> > + PS_SCALER_MODE_HQ : PS_SCALER_MODE_DYN;
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index f1051f0..9691768 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5809,6 +5809,7 @@ static int intel_crtc_compute_config(struct
> intel_crtc *crtc,
> > struct drm_device *dev = crtc->base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_display_mode *adjusted_mode =
> > &pipe_config->base.adjusted_mode;
> > + int ret;
> >
> > /* FIXME should check pixel clock limits on all platforms */
> > if (INTEL_INFO(dev)->gen < 4) {
> > @@ -5863,7 +5864,14 @@ static int intel_crtc_compute_config(struct
> intel_crtc *crtc,
> > if (pipe_config->has_pch_encoder)
> > return ironlake_fdi_compute_config(crtc, pipe_config);
> >
> > - return 0;
> > + /* FIXME: remove below call once atomic mode set is place and all crtc
> > + * related checks called from atomic_crtc_check function */
> > + ret = 0;
> > + DRM_DEBUG_KMS("intel_crtc = %p drm_state (pipe_config-
> >base.state) = %p\n",
> > + crtc, pipe_config->base.state);
> > + ret = intel_atomic_setup_scalers(dev, crtc, pipe_config);
> > +
> > + return ret;
> > }
> >
> > static int skylake_get_display_clock_speed(struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index adca692..f9614fb 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1382,6 +1382,9 @@ intel_atomic_get_crtc_state(struct
> > drm_atomic_state *state,
> >
> > return to_intel_crtc_state(crtc_state); }
> > +int intel_atomic_setup_scalers(struct drm_device *dev,
> > + struct intel_crtc *intel_crtc,
> > + struct intel_crtc_state *crtc_state);
> >
> > /* intel_atomic_plane.c */
> > struct intel_plane_state *intel_create_plane_state(struct drm_plane
> > *plane);
> > --
> > 1.7.9.5
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
