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 <matthew.d.roper@xxxxxxxxx>
> ---
> 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
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
