Hey, Op 20-04-16 om 04:26 schreef Matt Roper: > We eventually want to calculate watermark values at atomic 'check' time > instead of atomic 'commit' time so that any requested configurations > that result in impossible watermark requirements are properly rejected. > The first step along this path is to allocate the DDB at atomic 'check' > time. As we perform this transition, allow the main allocation function > to operate successfully on either an in-flight state or an > already-commited state. Once we complete the transition in a future > patch, we'll come back and remove the unnecessary logic for the > already-committed case. > > v2: Rebase/refactor; we should no longer need to grab extra plane states > while allocating the DDB since we can pull cached data rates and > minimum block counts from the CRTC state for any planes that aren't > being modified by this transaction. > > Signed-off-by: Matt Roper <[email protected]> > --- > drivers/gpu/drm/i915/i915_drv.h | 6 ++ > drivers/gpu/drm/i915/intel_pm.c | 179 > +++++++++++++++++++++++++++++----------- > 2 files changed, 139 insertions(+), 46 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 85102ad..e91d39d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -324,6 +324,12 @@ struct i915_hotplug { > &dev->mode_config.plane_list, \ > base.head) > > +#define for_each_intel_plane_mask(dev, intel_plane, plane_mask) > \ > + list_for_each_entry(intel_plane, &dev->mode_config.plane_list, \ > + base.head) \ > + for_each_if ((plane_mask) & \ > + (1 << drm_plane_index(&intel_plane->base))) > + > #define for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) \ > list_for_each_entry(intel_plane, \ > &(dev)->mode_config.plane_list, \ > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 479a890..640305a 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2849,13 +2849,18 @@ skl_wm_plane_id(const struct intel_plane *plane) > static void > skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > const struct intel_crtc_state *cstate, > - const struct intel_wm_config *config, > - struct skl_ddb_entry *alloc /* out */) > + struct intel_wm_config *config, > + struct skl_ddb_entry *alloc, /* out */ > + int *num_active /* out */) > { > + struct drm_atomic_state *state = cstate->base.state; > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > + struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_crtc *for_crtc = cstate->base.crtc; > struct drm_crtc *crtc; > unsigned int pipe_size, ddb_size; > int nth_active_pipe; > + int pipe = to_intel_crtc(for_crtc)->pipe; > > if (!cstate->base.active) { > alloc->start = 0; > @@ -2870,25 +2875,59 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device > *dev, > > ddb_size -= 4; /* 4 blocks for bypass path allocation */ > > - nth_active_pipe = 0; > - for_each_crtc(dev, crtc) { > - if (!to_intel_crtc(crtc)->active) > - continue; > + /* > + * FIXME: At the moment we may be called on either in-flight or fully > + * committed cstate's. Once we fully move DDB allocation in the check > + * phase, we'll only be called on in-flight states and the 'else' > + * branch here will go away. > + * > + * The 'else' branch is slightly racy here, but it was racy to begin > + * with; since it's going away soon, no effort is made to address that. > + */ > + if (state) { > + /* > + * If the state doesn't change the active CRTC's, then there's > + * no need to recalculate; the existing pipe allocation limits > + * should remain unchanged. Note that we're safe from racing > + * commits since any racing commit that changes the active CRTC > + * list would need to grab _all_ crtc locks, including the one > + * we currently hold. > + */ > + if (!intel_state->active_pipe_changes) { > + *alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe]; > + *num_active = hweight32(dev_priv->active_crtcs); > + return; > + } > > - if (crtc == for_crtc) > - break; > + *num_active = hweight32(intel_state->active_crtcs); > + nth_active_pipe = hweight32(intel_state->active_crtcs & > + (drm_crtc_mask(for_crtc) - 1)); > + pipe_size = ddb_size / hweight32(intel_state->active_crtcs); > + alloc->start = nth_active_pipe * ddb_size / *num_active; > + alloc->end = alloc->start + pipe_size; > + } else { > + nth_active_pipe = 0; > + for_each_crtc(dev, crtc) { > + if (!to_intel_crtc(crtc)->active) > + continue; > > - nth_active_pipe++; > - } > + if (crtc == for_crtc) > + break; > > - pipe_size = ddb_size / config->num_pipes_active; > - alloc->start = nth_active_pipe * ddb_size / config->num_pipes_active; > - alloc->end = alloc->start + pipe_size; > + nth_active_pipe++; > + } > + > + pipe_size = ddb_size / config->num_pipes_active; > + alloc->start = nth_active_pipe * ddb_size / > + config->num_pipes_active; > + alloc->end = alloc->start + pipe_size; > + *num_active = config->num_pipes_active; > + } > } > > -static unsigned int skl_cursor_allocation(const struct intel_wm_config > *config) > +static unsigned int skl_cursor_allocation(int num_active) > { > - if (config->num_pipes_active == 1) > + if (num_active == 1) > return 32; > > return 8; > @@ -3054,33 +3093,48 @@ skl_get_total_relative_data_rate(struct > intel_crtc_state *intel_cstate) > return total_data_rate; > } > > -static void > +static int > skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, > struct skl_ddb_allocation *ddb /* out */) > { > + struct drm_atomic_state *state = cstate->base.state; > struct drm_crtc *crtc = cstate->base.crtc; > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_wm_config *config = &dev_priv->wm.config; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_plane *intel_plane; > + struct drm_plane *plane; > + struct drm_plane_state *pstate; > enum pipe pipe = intel_crtc->pipe; > struct skl_ddb_entry *alloc = &ddb->pipe[pipe]; > uint16_t alloc_size, start, cursor_blocks; > uint16_t *minimum = cstate->wm.skl.minimum_blocks; > uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks; > unsigned int total_data_rate; > + int num_active; > + int id, i; > + > + if (!cstate->base.active) { > + ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0; > + memset(ddb->plane[pipe], 0, > + I915_MAX_PLANES * sizeof(struct skl_ddb_entry)); > + memset(ddb->y_plane[pipe], 0, > + I915_MAX_PLANES * sizeof(struct skl_ddb_entry)); > + return 0; > ^Since these are pointers to arrays, memset(x, 0, sizeof(x)); ?
Also there still seems to be a bogus memset for plane[pipe][PLANE_CURSOR], after plane[pipe] was just zero'd. Should probably be fixed while you're touching this function. :-) ~Maarten _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
