[Intel-gfx] [isg-gms] [RFC 3/6] drm/i915/vlv: Move fifo_size from intel_plane_wm_parameters to vlv_wm_state
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Wed Jun 15 05:51:25 UTC 2016
Op 14-06-16 om 23:52 schreef Matt Roper:
> On Wed, Jun 08, 2016 at 05:22:41PM +0200, Chi Ding wrote:
>> From: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>
>> This commit saves watermark for each plane in vlv_wm_state to prepare
>> for two-level watermark because we'll compute and save intermediate and
>> optimal watermark and fifo size for each plane.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Signed-off-by: Chi Ding <chix.ding at intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_drv.h | 12 +----
>> drivers/gpu/drm/i915/intel_pm.c | 111 +++++++++++++++++++++------------------
>> 2 files changed, 61 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index b973b86..31118e1 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -624,6 +624,7 @@ struct intel_crtc_state {
>> struct vlv_wm_state {
>> struct vlv_pipe_wm wm[3];
>> struct vlv_sr_wm sr[3];
>> + uint16_t fifo_size[I915_MAX_PLANES];
>> uint8_t num_active_planes;
>> uint8_t num_levels;
>> uint8_t level;
>> @@ -696,10 +697,6 @@ struct intel_crtc {
>> struct vlv_wm_state wm_state;
>> };
>>
>> -struct intel_plane_wm_parameters {
>> - uint16_t fifo_size;
>> -};
>> -
>> struct intel_plane {
>> struct drm_plane base;
>> int plane;
>> @@ -708,13 +705,6 @@ struct intel_plane {
>> int max_downscale;
>> uint32_t frontbuffer_bit;
>>
>> - /* Since we need to change the watermarks before/after
>> - * enabling/disabling the planes, we need to store the parameters here
>> - * as the other pieces of the struct may not reflect the values we want
>> - * for the watermark calculations. Currently only Haswell uses this.
>> - */
>> - struct intel_plane_wm_parameters wm;
>> -
>> /*
>> * NOTE: Do not place new plane state fields here (e.g., when adding
>> * new plane properties). New runtime state should now be placed in
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index a3942df..33f52ae 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -983,14 +983,16 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
>> return min_t(int, wm, USHRT_MAX);
>> }
>>
>> -static void vlv_compute_fifo(struct intel_crtc *crtc)
>> +static void vlv_compute_fifo(struct intel_crtc *crtc,
>> + struct vlv_wm_state *wm_state)
>> {
>> struct drm_device *dev = crtc->base.dev;
>> - struct vlv_wm_state *wm_state = &crtc->wm_state;
>> struct intel_plane *plane;
>> unsigned int total_rate = 0;
>> const int fifo_size = 512 - 1;
>> int fifo_extra, fifo_left = fifo_size;
>> + int rate[I915_MAX_PLANES] = {};
> I think this syntax might cause a warning on some versions of GCC (iirc,
> empty braces are technically illegal in the C spec, but legal in C++).
> I believe providing the value of the first element will avoid the
> warning (and still initialize all entries to 0); i.e.,
Kernel allows it, just grep for '= { }' or '= {}'.
> int rate[I915_MAX_PLANES] = { 0 };
>
>> + int i;
>>
>> for_each_intel_plane_on_crtc(dev, crtc, plane) {
>> struct intel_plane_state *state =
>> @@ -1001,58 +1003,55 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
>>
>> if (state->visible) {
>> wm_state->num_active_planes++;
>> - total_rate += drm_format_plane_cpp(state->base.fb->pixel_format, 0);
>> + rate[wm_plane_id(plane)] =
>> + drm_format_plane_cpp(state->base.fb->pixel_format, 0);
>> + total_rate += rate[wm_plane_id(plane)];
>> }
>> }
>>
>> - for_each_intel_plane_on_crtc(dev, crtc, plane) {
>> - struct intel_plane_state *state =
>> - to_intel_plane_state(plane->base.state);
>> - unsigned int rate;
>> -
>> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
>> - plane->wm.fifo_size = 63;
>> + for (i = 0; i < I915_MAX_PLANES; i++) {
> Is there a specific reason to change from iterating over planes to
> iterating over indices here? I think the end result is the same either
> way as far as I can see? (Assuming you could just set
> i = wm_plane_id(plane) like you did in the first loop if you kept the
> original loop).
Yeah, result is the same, but this way we don't have to grab the plane state multiple times.
>
>> + if (i == PLANE_CURSOR) {
>> + wm_state->fifo_size[i] = 63;
>> continue;
>> }
>>
>> - if (!state->visible) {
>> - plane->wm.fifo_size = 0;
>> + if (!rate[i]) {
>> + wm_state->fifo_size[i] = 0;
>> continue;
>> }
>>
>> - rate = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
>> - plane->wm.fifo_size = fifo_size * rate / total_rate;
>> - fifo_left -= plane->wm.fifo_size;
>> + wm_state->fifo_size[i] = fifo_size * rate[i] / total_rate;
>> + fifo_left -= wm_state->fifo_size[i];
>> }
>>
>> fifo_extra = DIV_ROUND_UP(fifo_left, wm_state->num_active_planes ?: 1);
>>
>> /* spread the remainder evenly */
>> - for_each_intel_plane_on_crtc(dev, crtc, plane) {
>> + for (i = 0; i < I915_MAX_PLANES; i++) {
>> int plane_extra;
>>
>> if (fifo_left == 0)
>> break;
>>
>> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
>> + if (i == PLANE_CURSOR)
>> continue;
>>
>> /* give it all to the first plane if none are active */
>> - if (plane->wm.fifo_size == 0 &&
>> + if (!wm_state->fifo_size[i] &&
>> wm_state->num_active_planes)
>> continue;
>>
>> plane_extra = min(fifo_extra, fifo_left);
>> - plane->wm.fifo_size += plane_extra;
>> + wm_state->fifo_size[i] += plane_extra;
>> fifo_left -= plane_extra;
>> }
>>
>> WARN_ON(fifo_left != 0);
>> }
>>
>> -static void vlv_invert_wms(struct intel_crtc *crtc)
>> +static void vlv_invert_wms(struct intel_crtc *crtc,
>> + struct vlv_wm_state *wm_state)
>> {
>> - struct vlv_wm_state *wm_state = &crtc->wm_state;
> Passing wm_state by parameter seems unrelated to the purpose of this
> patch (moving the fifo_size field). Was it supposed to go in a later
> patch?
Maybe, but it doesn't change much. The same wm_state is still used.
>> int level;
>>
>> for (level = 0; level < wm_state->num_levels; level++) {
>> @@ -1064,19 +1063,24 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
>> wm_state->sr[level].cursor = 63 - wm_state->sr[level].cursor;
>>
>> for_each_intel_plane_on_crtc(dev, crtc, plane) {
>> + int i = wm_plane_id(plane);
>> +
>> switch (plane->base.type) {
>> int sprite;
>> case DRM_PLANE_TYPE_CURSOR:
>> - wm_state->wm[level].cursor = plane->wm.fifo_size -
>> + wm_state->wm[level].cursor =
>> + wm_state->fifo_size[i] -
>> wm_state->wm[level].cursor;
>> break;
>> case DRM_PLANE_TYPE_PRIMARY:
>> - wm_state->wm[level].primary = plane->wm.fifo_size -
>> + wm_state->wm[level].primary =
>> + wm_state->fifo_size[i] -
>> wm_state->wm[level].primary;
>> break;
>> case DRM_PLANE_TYPE_OVERLAY:
>> sprite = plane->plane;
>> - wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
>> + wm_state->wm[level].sprite[sprite] =
>> + wm_state->fifo_size[i] -
>> wm_state->wm[level].sprite[sprite];
>> break;
>> }
>> @@ -1084,7 +1088,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
>> }
>> }
>>
>> -static void vlv_compute_wm(struct intel_crtc *crtc)
>> +static int vlv_compute_wm(struct intel_crtc *crtc)
>> {
>> struct drm_device *dev = crtc->base.dev;
>> struct vlv_wm_state *wm_state = &crtc->wm_state;
>> @@ -1099,7 +1103,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>>
>> wm_state->num_active_planes = 0;
>>
>> - vlv_compute_fifo(crtc);
>> + vlv_compute_fifo(crtc, wm_state);
>>
>> if (wm_state->num_active_planes != 1)
>> wm_state->cxsr = false;
>> @@ -1123,11 +1127,16 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>> int wm = vlv_compute_wm_level(plane, crtc, state, level);
>> int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
>>
>> - /* hack */
>> - if (WARN_ON(level == 0 && wm > max_wm))
>> - wm = max_wm;
>> + if (level == 0 && wm > max_wm) {
>> + DRM_DEBUG_KMS("Requested display configuration "
>> + "exceeds system watermark limitations\n");
>> + DRM_DEBUG_KMS("Plane %d.%d: blocks required = %u/%u\n",
>> + crtc->pipe,
>> + drm_plane_index(&plane->base), wm, max_wm);
>> + return -EINVAL;
>> + }
> This is an important change, but I think you meant to have this land in
> a different patch since it's unrelated to the content of this patch
> (which simply moves the fifo_size field).
Agreed.
>
>>
>> - if (wm > plane->wm.fifo_size)
>> + if (wm > wm_state->fifo_size[wm_plane_id(plane)])
>> break;
>>
>> switch (plane->base.type) {
>> @@ -1180,7 +1189,9 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>> memset(&wm_state->sr[level], 0, sizeof(wm_state->sr[level]));
>> }
>>
>> - vlv_invert_wms(crtc);
>> + vlv_invert_wms(crtc, wm_state);
>> +
>> + return 0;
>> }
>>
>> #define VLV_FIFO(plane, value) \
>> @@ -1190,24 +1201,18 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
>> {
>> struct drm_device *dev = crtc->base.dev;
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> - struct intel_plane *plane;
>> int sprite0_start = 0, sprite1_start = 0, fifo_size = 0;
>> + const struct vlv_wm_state *wm_state = &crtc->wm_state;
>>
>> - for_each_intel_plane_on_crtc(dev, crtc, plane) {
>> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
>> - WARN_ON(plane->wm.fifo_size != 63);
>> - continue;
>> - }
>>
>> - if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
>> - sprite0_start = plane->wm.fifo_size;
>> - else if (plane->plane == 0)
>> - sprite1_start = sprite0_start + plane->wm.fifo_size;
>> - else
>> - fifo_size = sprite1_start + plane->wm.fifo_size;
>> - }
>> + WARN_ON(wm_state->fifo_size[PLANE_CURSOR] != 63);
>> + sprite0_start = wm_state->fifo_size[0];
>> + sprite1_start = sprite0_start + wm_state->fifo_size[1];
>> + fifo_size = sprite1_start + wm_state->fifo_size[2];
>>
>> - WARN_ON(fifo_size != 512 - 1);
>> + WARN(fifo_size != 512 - 1, "Pipe %c FIFO split %d / %d / %d\n",
>> + pipe_name(crtc->pipe), sprite0_start,
>> + sprite1_start, fifo_size);
> The DRM_DEBUG_KMS() call below gives the same info you're adding to the
> message here; if a developer is debugging a problem here, I assume
> they'll be running with drm.debug=0xf or similar, so do we really need
> to change the WARN() line here to duplicate that info?
More information about the Intel-gfx
mailing list