| Message ID | 1459475198-30094-8-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 01-04-16 om 03:46 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. > > Note one other minor change here...when working with the > already-committed state, we pull the active CRTC's from > hweight(dev_priv->active_crtcs) instead of > dev_priv->wm.config.num_pipes_active. The values should be equivalent, > but dev_priv->wm.config is pretty much unused at this point and it would > be nice to eventually remove it entirely. > --- > drivers/gpu/drm/i915/i915_drv.h | 6 +++ > drivers/gpu/drm/i915/intel_pm.c | 99 ++++++++++++++++++++++++++++++----------- > 2 files changed, 78 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index d3ebb2f..79f1974 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -318,6 +318,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 e92513e..e0ca2b9 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2849,15 +2849,15 @@ 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, > + const unsigned active_crtcs, > struct skl_ddb_entry *alloc /* out */) > { > - struct drm_crtc *for_crtc = cstate->base.crtc; > - struct drm_crtc *crtc; > + struct drm_crtc *crtc = cstate->base.crtc; > unsigned int pipe_size, ddb_size; > + unsigned int num_active = hweight32(active_crtcs); > int nth_active_pipe; > > - if (!cstate->base.active) { > + if (!cstate->base.active || WARN_ON(num_active == 0)) { > alloc->start = 0; > alloc->end = 0; > return; > @@ -2870,25 +2870,16 @@ 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; > + nth_active_pipe = hweight32(active_crtcs & (drm_crtc_mask(crtc) - 1)); > > - if (crtc == for_crtc) > - break; > - > - nth_active_pipe++; > - } > - > - pipe_size = ddb_size / config->num_pipes_active; > - alloc->start = nth_active_pipe * ddb_size / config->num_pipes_active; > + pipe_size = ddb_size / num_active; > + alloc->start = nth_active_pipe * ddb_size / num_active; > alloc->end = alloc->start + pipe_size; > } > > -static unsigned int skl_cursor_allocation(const struct intel_wm_config *config) > +static unsigned int skl_cursor_allocation(const unsigned active_crtcs) > { > - if (config->num_pipes_active == 1) > + if (hweight32(active_crtcs) == 1) > return 32; > > return 8; > @@ -3018,14 +3009,13 @@ skl_get_total_relative_data_rate(const struct intel_crtc_state *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_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; > enum pipe pipe = intel_crtc->pipe; > @@ -3034,17 +3024,43 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, > uint16_t minimum[I915_MAX_PLANES]; > uint16_t y_minimum[I915_MAX_PLANES]; > unsigned int total_data_rate; > + unsigned active_crtcs = 0; > > - skl_ddb_get_pipe_allocation_limits(dev, cstate, config, alloc); > + 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; > + } > + > + /* > + * TODO: At the moment we might call this on either an in-flight CRTC > + * state or an already-committed state, so look up the number of > + * active CRTC's accordingly. Eventually this will only be called > + * on in-flight states and we'll be able to drop some of this extra > + * logic. > + */ > + if (cstate->base.state) { > + struct intel_atomic_state *intel_state = > + to_intel_atomic_state(cstate->base.state); > + > + active_crtcs = intel_state->active_crtcs; > + } else { > + active_crtcs = dev_priv->active_crtcs; > + } > + > + skl_ddb_get_pipe_allocation_limits(dev, cstate, active_crtcs, alloc); > alloc_size = skl_ddb_entry_size(alloc); > if (alloc_size == 0) { > memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); > memset(&ddb->plane[pipe][PLANE_CURSOR], 0, > sizeof(ddb->plane[pipe][PLANE_CURSOR])); > - return; > + return 0; > } > > - cursor_blocks = skl_cursor_allocation(config); > + cursor_blocks = skl_cursor_allocation(active_crtcs); > ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - cursor_blocks; > ddb->plane[pipe][PLANE_CURSOR].end = alloc->end; > > @@ -3054,15 +3070,30 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, > /* 1. Allocate the mininum required blocks for each active plane */ > for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { > struct drm_plane *plane = &intel_plane->base; > + struct drm_plane_state *pstate; > struct drm_framebuffer *fb = plane->state->fb; > int id = skl_wm_plane_id(intel_plane); > > - if (!to_intel_plane_state(plane->state)->visible) > + /* > + * TODO: Remove support for already-committed state once we > + * only allocate DDB on in-flight states. > + */ > + if (cstate->base.state) { > + pstate = drm_atomic_get_plane_state(cstate->base.state, > + plane); > + if (IS_ERR(pstate)) > + return PTR_ERR(pstate); > + } else { > + pstate = plane->state; > + } > + > + if (!to_intel_plane_state(pstate)->visible) > continue; > > if (plane->type == DRM_PLANE_TYPE_CURSOR) > continue; > > + fb = pstate->fb; > minimum[id] = 8; > alloc_size -= minimum[id]; > y_minimum[id] = (fb->pixel_format == DRM_FORMAT_NV12) ? 8 : 0; > @@ -3078,13 +3109,26 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, > total_data_rate = skl_get_total_relative_data_rate(cstate); > > start = alloc->start; > - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { > + for_each_intel_plane_mask(dev, intel_plane, cstate->base.plane_mask) { > struct drm_plane *plane = &intel_plane->base; > - struct drm_plane_state *pstate = intel_plane->base.state; > + struct drm_plane_state *pstate; > unsigned int data_rate, y_data_rate; > uint16_t plane_blocks, y_plane_blocks = 0; > int id = skl_wm_plane_id(intel_plane); > > + /* > + * TODO: Remove support for already-committed state once we > + * only allocate DDB on in-flight states. > + */ > + if (cstate->base.state) { > + pstate = drm_atomic_get_plane_state(cstate->base.state, > + plane); > Yuck again? :( No way around this by storing data rates for example? Oh well, at least set cstate->base.planes_changed please.
On Tue, Apr 05, 2016 at 11:35:31AM +0200, Maarten Lankhorst wrote: > Op 01-04-16 om 03:46 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. > > > > Note one other minor change here...when working with the > > already-committed state, we pull the active CRTC's from > > hweight(dev_priv->active_crtcs) instead of > > dev_priv->wm.config.num_pipes_active. The values should be equivalent, > > but dev_priv->wm.config is pretty much unused at this point and it would > > be nice to eventually remove it entirely. > > --- ... > > @@ -3078,13 +3109,26 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, > > total_data_rate = skl_get_total_relative_data_rate(cstate); > > > > start = alloc->start; > > - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { > > + for_each_intel_plane_mask(dev, intel_plane, cstate->base.plane_mask) { > > struct drm_plane *plane = &intel_plane->base; > > - struct drm_plane_state *pstate = intel_plane->base.state; > > + struct drm_plane_state *pstate; > > unsigned int data_rate, y_data_rate; > > uint16_t plane_blocks, y_plane_blocks = 0; > > int id = skl_wm_plane_id(intel_plane); > > > > + /* > > + * TODO: Remove support for already-committed state once we > > + * only allocate DDB on in-flight states. > > + */ > > + if (cstate->base.state) { > > + pstate = drm_atomic_get_plane_state(cstate->base.state, > > + plane); > > > Yuck again? :( No way around this by storing data rates for example? Is there a reason to try to avoid grabbing the plane state here? If we get here, then we already have the CRTC lock, so we're not preventing any potential for parallel updates of this plane (at least not on Intel hardware where planes are tied to the CRTC). > Oh well, at least set cstate->base.planes_changed please. I'm not sure I follow this one. We're using pstate in a read-only manner so nothing we're doing here should necessitate programming planes as far as I can see. Matt
Op 14-04-16 om 03:58 schreef Matt Roper: > On Tue, Apr 05, 2016 at 11:35:31AM +0200, Maarten Lankhorst wrote: >> Op 01-04-16 om 03:46 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. >>> >>> Note one other minor change here...when working with the >>> already-committed state, we pull the active CRTC's from >>> hweight(dev_priv->active_crtcs) instead of >>> dev_priv->wm.config.num_pipes_active. The values should be equivalent, >>> but dev_priv->wm.config is pretty much unused at this point and it would >>> be nice to eventually remove it entirely. >>> --- > ... >>> @@ -3078,13 +3109,26 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, >>> total_data_rate = skl_get_total_relative_data_rate(cstate); >>> >>> start = alloc->start; >>> - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { >>> + for_each_intel_plane_mask(dev, intel_plane, cstate->base.plane_mask) { >>> struct drm_plane *plane = &intel_plane->base; >>> - struct drm_plane_state *pstate = intel_plane->base.state; >>> + struct drm_plane_state *pstate; >>> unsigned int data_rate, y_data_rate; >>> uint16_t plane_blocks, y_plane_blocks = 0; >>> int id = skl_wm_plane_id(intel_plane); >>> >>> + /* >>> + * TODO: Remove support for already-committed state once we >>> + * only allocate DDB on in-flight states. >>> + */ >>> + if (cstate->base.state) { >>> + pstate = drm_atomic_get_plane_state(cstate->base.state, >>> + plane); >>> >> Yuck again? :( No way around this by storing data rates for example? > Is there a reason to try to avoid grabbing the plane state here? If we > get here, then we already have the CRTC lock, so we're not preventing > any potential for parallel updates of this plane (at least not on Intel > hardware where planes are tied to the CRTC). If you don't have to, you don't want to grab the plane state. It's probably fine to do so temporarily, but a solution like for ILK would be better eventually. :) >> Oh well, at least set cstate->base.planes_changed please. > I'm not sure I follow this one. We're using pstate in a read-only > manner so nothing we're doing here should necessitate programming planes > as far as I can see. Just being part of the state is enough. intel_prepare_plane_fb will be called on those planes, so with my async unpin patch series you need to set the flag or you may not have async unpins, resulting in errors much later on when the fb is freed. ~Maarten
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d3ebb2f..79f1974 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -318,6 +318,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 e92513e..e0ca2b9 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2849,15 +2849,15 @@ 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, + const unsigned active_crtcs, struct skl_ddb_entry *alloc /* out */) { - struct drm_crtc *for_crtc = cstate->base.crtc; - struct drm_crtc *crtc; + struct drm_crtc *crtc = cstate->base.crtc; unsigned int pipe_size, ddb_size; + unsigned int num_active = hweight32(active_crtcs); int nth_active_pipe; - if (!cstate->base.active) { + if (!cstate->base.active || WARN_ON(num_active == 0)) { alloc->start = 0; alloc->end = 0; return; @@ -2870,25 +2870,16 @@ 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; + nth_active_pipe = hweight32(active_crtcs & (drm_crtc_mask(crtc) - 1)); - if (crtc == for_crtc) - break; - - nth_active_pipe++; - } - - pipe_size = ddb_size / config->num_pipes_active; - alloc->start = nth_active_pipe * ddb_size / config->num_pipes_active; + pipe_size = ddb_size / num_active; + alloc->start = nth_active_pipe * ddb_size / num_active; alloc->end = alloc->start + pipe_size; } -static unsigned int skl_cursor_allocation(const struct intel_wm_config *config) +static unsigned int skl_cursor_allocation(const unsigned active_crtcs) { - if (config->num_pipes_active == 1) + if (hweight32(active_crtcs) == 1) return 32; return 8; @@ -3018,14 +3009,13 @@ skl_get_total_relative_data_rate(const struct intel_crtc_state *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_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; enum pipe pipe = intel_crtc->pipe; @@ -3034,17 +3024,43 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, uint16_t minimum[I915_MAX_PLANES]; uint16_t y_minimum[I915_MAX_PLANES]; unsigned int total_data_rate; + unsigned active_crtcs = 0; - skl_ddb_get_pipe_allocation_limits(dev, cstate, config, alloc); + 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; + } + + /* + * TODO: At the moment we might call this on either an in-flight CRTC + * state or an already-committed state, so look up the number of + * active CRTC's accordingly. Eventually this will only be called + * on in-flight states and we'll be able to drop some of this extra + * logic. + */ + if (cstate->base.state) { + struct intel_atomic_state *intel_state = + to_intel_atomic_state(cstate->base.state); + + active_crtcs = intel_state->active_crtcs; + } else { + active_crtcs = dev_priv->active_crtcs; + } + + skl_ddb_get_pipe_allocation_limits(dev, cstate, active_crtcs, alloc); alloc_size = skl_ddb_entry_size(alloc); if (alloc_size == 0) { memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); memset(&ddb->plane[pipe][PLANE_CURSOR], 0, sizeof(ddb->plane[pipe][PLANE_CURSOR])); - return; + return 0; } - cursor_blocks = skl_cursor_allocation(config); + cursor_blocks = skl_cursor_allocation(active_crtcs); ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - cursor_blocks; ddb->plane[pipe][PLANE_CURSOR].end = alloc->end; @@ -3054,15 +3070,30 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, /* 1. Allocate the mininum required blocks for each active plane */ for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { struct drm_plane *plane = &intel_plane->base; + struct drm_plane_state *pstate; struct drm_framebuffer *fb = plane->state->fb; int id = skl_wm_plane_id(intel_plane); - if (!to_intel_plane_state(plane->state)->visible) + /* + * TODO: Remove support for already-committed state once we + * only allocate DDB on in-flight states. + */ + if (cstate->base.state) { + pstate = drm_atomic_get_plane_state(cstate->base.state, + plane); + if (IS_ERR(pstate)) + return PTR_ERR(pstate); + } else { + pstate = plane->state; + } + + if (!to_intel_plane_state(pstate)->visible) continue; if (plane->type == DRM_PLANE_TYPE_CURSOR) continue; + fb = pstate->fb; minimum[id] = 8; alloc_size -= minimum[id]; y_minimum[id] = (fb->pixel_format == DRM_FORMAT_NV12) ? 8 : 0; @@ -3078,13 +3109,26 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, total_data_rate = skl_get_total_relative_data_rate(cstate); start = alloc->start; - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { + for_each_intel_plane_mask(dev, intel_plane, cstate->base.plane_mask) { struct drm_plane *plane = &intel_plane->base; - struct drm_plane_state *pstate = intel_plane->base.state; + struct drm_plane_state *pstate; unsigned int data_rate, y_data_rate; uint16_t plane_blocks, y_plane_blocks = 0; int id = skl_wm_plane_id(intel_plane); + /* + * TODO: Remove support for already-committed state once we + * only allocate DDB on in-flight states. + */ + if (cstate->base.state) { + pstate = drm_atomic_get_plane_state(cstate->base.state, + plane); + if (WARN_ON(IS_ERR(pstate))) + return PTR_ERR(pstate); + } else { + pstate = plane->state; + } + if (!to_intel_plane_state(pstate)->visible) continue; if (plane->type == DRM_PLANE_TYPE_CURSOR) @@ -3125,6 +3169,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, } + return 0; } static uint32_t skl_pipe_pixel_rate(const struct intel_crtc_state *config) @@ -3615,7 +3660,7 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state); - skl_allocate_pipe_ddb(cstate, ddb); + WARN_ON(skl_allocate_pipe_ddb(cstate, ddb) != 0); skl_build_pipe_wm(cstate, ddb, pipe_wm); if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))