| Message ID | 20181016220133.26991-9-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More watermarks improvements | expand |
On Tue, Oct 16, 2018 at 03:01:30PM -0700, Paulo Zanoni wrote: > Print a more generic "failed to compute watermark levels" whenever any > of skl_compute_wm_levels() fail, and print only the specific error > message for the specific cases. This allows us to stop passing pstate > everywhere, making the watermarks computation code a little less > dependent on random intel state structs. Nothing random about those structs. They are *the* state. Using them directly rather than duplicating informationa in the wm_params struct would probably make the code look a bit less alien. But given that the information is duplicated I guess we don't have to pass in the plane state. So the patch seems fine in that sense. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++--------------- > 1 file changed, 12 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 4053f4a68657..1290efc64869 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv, > > static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > struct intel_crtc_state *cstate, > - const struct intel_plane_state *intel_pstate, > int level, > const struct skl_wm_params *wp, > const struct skl_wm_level *result_prev, > struct skl_wm_level *result /* out */) > { > - const struct drm_plane_state *pstate = &intel_pstate->base; > uint32_t latency = dev_priv->wm.skl_latency[level]; > uint_fixed_16_16_t method1, method2; > uint_fixed_16_16_t selected_result; > @@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > if (level) { > return 0; > } else { > - struct drm_plane *plane = pstate->plane; > - > - DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n"); > - DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n", > - plane->base.id, plane->name, > + DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations: blocks required = %u/%u, lines required = %u/31\n", > res_blocks, wp->ddb_blocks, res_lines); > return -EINVAL; > } > @@ -4795,7 +4789,6 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > static int > skl_compute_wm_levels(const struct drm_i915_private *dev_priv, > struct intel_crtc_state *cstate, > - const struct intel_plane_state *intel_pstate, > const struct skl_wm_params *wm_params, > struct skl_plane_wm *wm, > int plane_id) > @@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv, > > ret = skl_compute_plane_wm(dev_priv, > cstate, > - intel_pstate, > level, > wm_params, > result_prev, > @@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate, > if (!wm_params.plane_visible) > continue; > > - ret = skl_compute_wm_levels(dev_priv, cstate, > - intel_pstate, &wm_params, wm, 0); > - if (ret) > + ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm, > + 0); > + if (ret) { > + DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n", > + plane->base.id, plane->name); > return ret; > + } > > skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0], > &wm->trans_wm); > @@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate, > return ret; > > ret = skl_compute_wm_levels(dev_priv, cstate, > - intel_pstate, &wm_params, > - wm, 1); > - if (ret) > + &wm_params, wm, 1); > + if (ret) { > + DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n", > + plane->base.id, plane->name); > return ret; > + } > } > } > > -- > 2.14.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 18, 2018 at 04:55:52PM +0300, Ville Syrjälä wrote: > On Tue, Oct 16, 2018 at 03:01:30PM -0700, Paulo Zanoni wrote: > > Print a more generic "failed to compute watermark levels" whenever any > > of skl_compute_wm_levels() fail, and print only the specific error > > message for the specific cases. This allows us to stop passing pstate > > everywhere, making the watermarks computation code a little less > > dependent on random intel state structs. > > Nothing random about those structs. They are *the* state. > Using them directly rather than duplicating informationa in the > wm_params struct would probably make the code look a bit less alien. > > But given that the information is duplicated I guess we don't have to > pass in the plane state. So the patch seems fine in that sense. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> And this will definitely conflict with Maarten's NV12 work. Either we need to duplicate even more stuff or just pass the proper states around. > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++--------------- > > 1 file changed, 12 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 4053f4a68657..1290efc64869 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv, > > > > static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > > struct intel_crtc_state *cstate, > > - const struct intel_plane_state *intel_pstate, > > int level, > > const struct skl_wm_params *wp, > > const struct skl_wm_level *result_prev, > > struct skl_wm_level *result /* out */) > > { > > - const struct drm_plane_state *pstate = &intel_pstate->base; > > uint32_t latency = dev_priv->wm.skl_latency[level]; > > uint_fixed_16_16_t method1, method2; > > uint_fixed_16_16_t selected_result; > > @@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > > if (level) { > > return 0; > > } else { > > - struct drm_plane *plane = pstate->plane; > > - > > - DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n"); > > - DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n", > > - plane->base.id, plane->name, > > + DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations: blocks required = %u/%u, lines required = %u/31\n", > > res_blocks, wp->ddb_blocks, res_lines); > > return -EINVAL; > > } > > @@ -4795,7 +4789,6 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > > static int > > skl_compute_wm_levels(const struct drm_i915_private *dev_priv, > > struct intel_crtc_state *cstate, > > - const struct intel_plane_state *intel_pstate, > > const struct skl_wm_params *wm_params, > > struct skl_plane_wm *wm, > > int plane_id) > > @@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv, > > > > ret = skl_compute_plane_wm(dev_priv, > > cstate, > > - intel_pstate, > > level, > > wm_params, > > result_prev, > > @@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate, > > if (!wm_params.plane_visible) > > continue; > > > > - ret = skl_compute_wm_levels(dev_priv, cstate, > > - intel_pstate, &wm_params, wm, 0); > > - if (ret) > > + ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm, > > + 0); > > + if (ret) { > > + DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n", > > + plane->base.id, plane->name); > > return ret; > > + } > > > > skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0], > > &wm->trans_wm); > > @@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate, > > return ret; > > > > ret = skl_compute_wm_levels(dev_priv, cstate, > > - intel_pstate, &wm_params, > > - wm, 1); > > - if (ret) > > + &wm_params, wm, 1); > > + if (ret) { > > + DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n", > > + plane->base.id, plane->name); > > return ret; > > + } > > } > > } > > > > -- > > 2.14.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
Em Qui, 2018-10-18 às 16:55 +0300, Ville Syrjälä escreveu: > On Tue, Oct 16, 2018 at 03:01:30PM -0700, Paulo Zanoni wrote: > > Print a more generic "failed to compute watermark levels" whenever > > any > > of skl_compute_wm_levels() fail, and print only the specific error > > message for the specific cases. This allows us to stop passing > > pstate > > everywhere, making the watermarks computation code a little less > > dependent on random intel state structs. > > Nothing random about those structs. They are *the* state. What I'm about to say is all probably a reflex of my own incompetence to understand the flows of our code, but here it goes. 1. There's duplication in those structs. At any given point of our source code, should you use state structs passed as parameters, or should you use object->state (e.g., intel_crtc->state)? Sometimes one thing is the new state and the other thing is the old state, sometimes it is the opposite, and checking which one is which is never trivial. I always have to go back to intel_display.c and try to find that point in the modeset where we assign crtc_config to crtc->config and then try to figure out if my code runs before or after that point. And I'm never 100% confident I'm using the correct struct. 2. There's a lot of duplication in members of those structs. There are like 5 different things that could mean "pixel rate" (for real mode, for adjusted mode, considering/not-considering scaling, rotation, etc), there are many things that could mean "source width", etc. So when you need a specific value, it's not always clear where to get it from in our driver. 3. The names inside the watermarks calculation page (or anywhere else in our spec) don't easily translate to any of those struct members mentioned in item 2. E.g., plane pixel rate. I mean, look at how many times the spec (not only for watermarks, but for other stuff too, like FBC and PSR) had wording like "source size" and we fetched the value from one place, but then we learned that we needed to fetch the value from this other place that was the same in most cases except for these X and Y corner cases. You reviewed some of those patches. So, to conclude the argument, the nice thing about struct skl_wm_params is that it allows us to have a single point where we translate "i915 terminology" to "watermarks calculation algorithm terminology", so we can fix that single place if/when needed. And then everywhere else in watermarks code we just fetch the value from this struct without having to worry "at this point in the sequence, where should this value come from?", allowing us to focus on the algorithm specifically. On top of that, there's the fact that we only compute things once instead of up to 8 times per plane (7 levels + transition watermarks). Of course, the downside is that we address the problem of "too many places to fetch information from" by introducing a new place where information is fetched from, so I definitely can't argue that the current solution is good either. I would really like to know what are your proposals here: if we decide to remove the params struct, would you suggest we simply fetch everything directly from the passed structs and recompute all the values that are computed multiple times? I'm totally willing to implement something better if you have it in your head. > Using them directly rather than duplicating informationa in the > wm_params struct would probably make the code look a bit less alien. > > But given that the information is duplicated I guess we don't have to > pass in the plane state. So the patch seems fine in that sense. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks for the reviews! > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++--------------- > > 1 file changed, 12 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c > > index 4053f4a68657..1290efc64869 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct > > drm_i915_private *dev_priv, > > > > static int skl_compute_plane_wm(const struct drm_i915_private > > *dev_priv, > > struct intel_crtc_state *cstate, > > - const struct intel_plane_state > > *intel_pstate, > > int level, > > const struct skl_wm_params *wp, > > const struct skl_wm_level > > *result_prev, > > struct skl_wm_level *result /* out > > */) > > { > > - const struct drm_plane_state *pstate = &intel_pstate- > > >base; > > uint32_t latency = dev_priv->wm.skl_latency[level]; > > uint_fixed_16_16_t method1, method2; > > uint_fixed_16_16_t selected_result; > > @@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct > > drm_i915_private *dev_priv, > > if (level) { > > return 0; > > } else { > > - struct drm_plane *plane = pstate->plane; > > - > > - DRM_DEBUG_KMS("Requested display > > configuration exceeds system watermark limitations\n"); > > - DRM_DEBUG_KMS("[PLANE:%d:%s] blocks > > required = %u/%u, lines required = %u/31\n", > > - plane->base.id, plane->name, > > + DRM_DEBUG_KMS("Requested display > > configuration exceeds system watermark limitations: blocks required > > = %u/%u, lines required = %u/31\n", > > res_blocks, wp->ddb_blocks, > > res_lines); > > return -EINVAL; > > } > > @@ -4795,7 +4789,6 @@ static int skl_compute_plane_wm(const struct > > drm_i915_private *dev_priv, > > static int > > skl_compute_wm_levels(const struct drm_i915_private *dev_priv, > > struct intel_crtc_state *cstate, > > - const struct intel_plane_state > > *intel_pstate, > > const struct skl_wm_params *wm_params, > > struct skl_plane_wm *wm, > > int plane_id) > > @@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct > > drm_i915_private *dev_priv, > > > > ret = skl_compute_plane_wm(dev_priv, > > cstate, > > - intel_pstate, > > level, > > wm_params, > > result_prev, > > @@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct > > intel_crtc_state *cstate, > > if (!wm_params.plane_visible) > > continue; > > > > - ret = skl_compute_wm_levels(dev_priv, cstate, > > - intel_pstate, > > &wm_params, wm, 0); > > - if (ret) > > + ret = skl_compute_wm_levels(dev_priv, cstate, > > &wm_params, wm, > > + 0); > > + if (ret) { > > + DRM_DEBUG_KMS("[PLANE:%d:%s] failed to > > compute watermark levels\n", > > + plane->base.id, plane- > > >name); > > return ret; > > + } > > > > skl_compute_transition_wm(cstate, &wm_params, &wm- > > >wm[0], > > &wm->trans_wm); > > @@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct > > intel_crtc_state *cstate, > > return ret; > > > > ret = skl_compute_wm_levels(dev_priv, > > cstate, > > - intel_pstate, > > &wm_params, > > - wm, 1); > > - if (ret) > > + &wm_params, > > wm, 1); > > + if (ret) { > > + DRM_DEBUG_KMS("[PLANE:%d:%s] > > failed to compute planar watermark levels\n", > > + plane->base.id, > > plane->name); > > return ret; > > + } > > } > > } > > > > -- > > 2.14.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On Mon, Oct 22, 2018 at 03:22:27PM -0700, Paulo Zanoni wrote: > Em Qui, 2018-10-18 às 16:55 +0300, Ville Syrjälä escreveu: > > On Tue, Oct 16, 2018 at 03:01:30PM -0700, Paulo Zanoni wrote: > > > Print a more generic "failed to compute watermark levels" whenever > > > any > > > of skl_compute_wm_levels() fail, and print only the specific error > > > message for the specific cases. This allows us to stop passing > > > pstate > > > everywhere, making the watermarks computation code a little less > > > dependent on random intel state structs. > > > > Nothing random about those structs. They are *the* state. > > What I'm about to say is all probably a reflex of my own incompetence > to understand the flows of our code, but here it goes. > > 1. There's duplication in those structs. At any given point of our > source code, should you use state structs passed as parameters, or > should you use object->state (e.g., intel_crtc->state)? Sometimes one > thing is the new state and the other thing is the old state, sometimes > it is the opposite, and checking which one is which is never trivial. That was solved with the mlankhorst iterators. So no more foo->state, except during readout since we don't currently allocate new states for that. > I > always have to go back to intel_display.c and try to find that point in > the modeset where we assign crtc_config to crtc->config and then try to > figure out if my code runs before or after that point. And I'm never > 100% confident I'm using the correct struct. And Maarten has been busy elimninating crtc->config as well. Not too many uses left I hope. > > 2. There's a lot of duplication in members of those structs. There are > like 5 different things that could mean "pixel rate" (for real mode, > for adjusted mode, considering/not-considering scaling, rotation, etc), > there are many things that could mean "source width", etc. So when you > need a specific value, it's not always clear where to get it from in > our driver. > > 3. The names inside the watermarks calculation page (or anywhere else > in our spec) don't easily translate to any of those struct members > mentioned in item 2. E.g., plane pixel rate. > > I mean, look at how many times the spec (not only for watermarks, but > for other stuff too, like FBC and PSR) had wording like "source size" > and we fetched the value from one place, but then we learned that we > needed to fetch the value from this other place that was the same in > most cases except for these X and Y corner cases. You reviewed some of > those patches. I don't recall many corner cases. Well, I guess you can count the cursor as one. But that's about it I think. Rotation was maybe the one that caused the most confusion but I think that was because the spec was very vague. One actually has to think a bit to figure out if it's referring to the fb orientation or the scanout orientation. > > So, to conclude the argument, the nice thing about struct skl_wm_params > is that it allows us to have a single point where we translate "i915 > terminology" to "watermarks calculation algorithm terminology", so we > can fix that single place if/when needed. That can usually be solved with functions as well. > And then everywhere else in > watermarks code we just fetch the value from this struct without having > to worry "at this point in the sequence, where should this value come > from?", allowing us to focus on the algorithm specifically. On top of > that, there's the fact that we only compute things once instead of up > to 8 times per plane (7 levels + transition watermarks). > > Of course, the downside is that we address the problem of "too many > places to fetch information from" by introducing a new place where > information is fetched from, so I definitely can't argue that the > current solution is good either. I would really like to know what are > your proposals here: if we decide to remove the params struct, would > you suggest we simply fetch everything directly from the passed structs > and recompute all the values that are computed multiple times? I'm > totally willing to implement something better if you have it in your > head. The "let's not recompute the same thing many times" argument may have some merit. Though most things are perhaps cheap enough to not matter much. And we could always stick more stuff into the plane/crtc states. But I don't have any concrete proposals at this time. I've tried to stay away from the skl wm code as much as possible so as to not get an urge to rewrite it :P
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 4053f4a68657..1290efc64869 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv, static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, struct intel_crtc_state *cstate, - const struct intel_plane_state *intel_pstate, int level, const struct skl_wm_params *wp, const struct skl_wm_level *result_prev, struct skl_wm_level *result /* out */) { - const struct drm_plane_state *pstate = &intel_pstate->base; uint32_t latency = dev_priv->wm.skl_latency[level]; uint_fixed_16_16_t method1, method2; uint_fixed_16_16_t selected_result; @@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, if (level) { return 0; } else { - struct drm_plane *plane = pstate->plane; - - DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n"); - DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n", - plane->base.id, plane->name, + DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations: blocks required = %u/%u, lines required = %u/31\n", res_blocks, wp->ddb_blocks, res_lines); return -EINVAL; } @@ -4795,7 +4789,6 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, static int skl_compute_wm_levels(const struct drm_i915_private *dev_priv, struct intel_crtc_state *cstate, - const struct intel_plane_state *intel_pstate, const struct skl_wm_params *wm_params, struct skl_plane_wm *wm, int plane_id) @@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv, ret = skl_compute_plane_wm(dev_priv, cstate, - intel_pstate, level, wm_params, result_prev, @@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate, if (!wm_params.plane_visible) continue; - ret = skl_compute_wm_levels(dev_priv, cstate, - intel_pstate, &wm_params, wm, 0); - if (ret) + ret = skl_compute_wm_levels(dev_priv, cstate, &wm_params, wm, + 0); + if (ret) { + DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute watermark levels\n", + plane->base.id, plane->name); return ret; + } skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0], &wm->trans_wm); @@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate, return ret; ret = skl_compute_wm_levels(dev_priv, cstate, - intel_pstate, &wm_params, - wm, 1); - if (ret) + &wm_params, wm, 1); + if (ret) { + DRM_DEBUG_KMS("[PLANE:%d:%s] failed to compute planar watermark levels\n", + plane->base.id, plane->name); return ret; + } } }
Print a more generic "failed to compute watermark levels" whenever any of skl_compute_wm_levels() fail, and print only the specific error message for the specific cases. This allows us to stop passing pstate everywhere, making the watermarks computation code a little less dependent on random intel state structs. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)