| Message ID | 20200228203552.30273-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915: Don't check uv_wm in skl_plane_wm_equals() | expand |
On Fri, 2020-02-28 at 22:35 +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Currently we're comparing the watermarks between the old and new > states > before we've fully computed the new watermarks. In particular > skl_build_pipe_wm() will not account for the amount of ddb space > we'll > have. That information is only available during skl_compute_ddb() > which will proceed to zero out any watermark level exceeding the > ddb allocation. If we're short on ddb space this will end up > adding the plane to the state due erronously determining that the > watermarks have changed. Fix the problem by deferring > skl_wm_add_affected_planes() until we have the final watermarks > computed. > > Noticed this when trying enable transition watermarks on glk. > We now computed the trans_wm as 28, but we only had 14 blocks > of ddb, and thus skl_compute_ddb() ended up disabling the cursor > trans_wm every time. Thus we ended up adding the cursor to every > commit that didn't actually affect the cursor at all. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c > index 39299811b650..a3d76e69caae 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5762,16 +5762,24 @@ skl_compute_wm(struct intel_atomic_state > *state) > ret = skl_build_pipe_wm(new_crtc_state); > if (ret) > return ret; > - > - ret = skl_wm_add_affected_planes(state, crtc); > - if (ret) > - return ret; > } > > ret = skl_compute_ddb(state); > if (ret) > return ret; > > + /* > + * skl_compute_ddb() will have adjusted the final watermarks > + * based on how much ddb is available. Now we can actually > + * check if the final watermarks changed. > + */ > + for_each_oldnew_intel_crtc_in_state(state, crtc, > old_crtc_state, > + new_crtc_state, i) { > + ret = skl_wm_add_affected_planes(state, crtc); > + if (ret) > + return ret; > + } skl_compute_ddb() is already calling skl_wm_add_affected_planes() after do the ddb allocation for each pipe, so we could remove this chunk, with that: Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > + > skl_print_wm_changes(state); > > return 0;
On Wed, Mar 04, 2020 at 12:21:01AM +0000, Souza, Jose wrote: > On Fri, 2020-02-28 at 22:35 +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Currently we're comparing the watermarks between the old and new > > states > > before we've fully computed the new watermarks. In particular > > skl_build_pipe_wm() will not account for the amount of ddb space > > we'll > > have. That information is only available during skl_compute_ddb() > > which will proceed to zero out any watermark level exceeding the > > ddb allocation. If we're short on ddb space this will end up > > adding the plane to the state due erronously determining that the > > watermarks have changed. Fix the problem by deferring > > skl_wm_add_affected_planes() until we have the final watermarks > > computed. > > > > Noticed this when trying enable transition watermarks on glk. > > We now computed the trans_wm as 28, but we only had 14 blocks > > of ddb, and thus skl_compute_ddb() ended up disabling the cursor > > trans_wm every time. Thus we ended up adding the cursor to every > > commit that didn't actually affect the cursor at all. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c > > index 39299811b650..a3d76e69caae 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5762,16 +5762,24 @@ skl_compute_wm(struct intel_atomic_state > > *state) > > ret = skl_build_pipe_wm(new_crtc_state); > > if (ret) > > return ret; > > - > > - ret = skl_wm_add_affected_planes(state, crtc); > > - if (ret) > > - return ret; > > } > > > > ret = skl_compute_ddb(state); > > if (ret) > > return ret; > > > > + /* > > + * skl_compute_ddb() will have adjusted the final watermarks > > + * based on how much ddb is available. Now we can actually > > + * check if the final watermarks changed. > > + */ > > + for_each_oldnew_intel_crtc_in_state(state, crtc, > > old_crtc_state, > > + new_crtc_state, i) { > > + ret = skl_wm_add_affected_planes(state, crtc); > > + if (ret) > > + return ret; > > + } > > skl_compute_ddb() is already calling skl_wm_add_affected_planes() after > do the ddb allocation for each pipe, so we could remove this chunk, skl_compute_ddb() calls skl_*ddb*_add_affected_planes(), which is a different thing. > with that: > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > + > > skl_print_wm_changes(state); > > > > return 0;
On Wed, 2020-03-04 at 13:46 +0200, Ville Syrjälä wrote: > On Wed, Mar 04, 2020 at 12:21:01AM +0000, Souza, Jose wrote: > > On Fri, 2020-02-28 at 22:35 +0200, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Currently we're comparing the watermarks between the old and new > > > states > > > before we've fully computed the new watermarks. In particular > > > skl_build_pipe_wm() will not account for the amount of ddb space > > > we'll > > > have. That information is only available during skl_compute_ddb() > > > which will proceed to zero out any watermark level exceeding the > > > ddb allocation. If we're short on ddb space this will end up > > > adding the plane to the state due erronously determining that the > > > watermarks have changed. Fix the problem by deferring > > > skl_wm_add_affected_planes() until we have the final watermarks > > > computed. > > > > > > Noticed this when trying enable transition watermarks on glk. > > > We now computed the trans_wm as 28, but we only had 14 blocks > > > of ddb, and thus skl_compute_ddb() ended up disabling the cursor > > > trans_wm every time. Thus we ended up adding the cursor to every > > > commit that didn't actually affect the cursor at all. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++---- > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > > b/drivers/gpu/drm/i915/intel_pm.c > > > index 39299811b650..a3d76e69caae 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -5762,16 +5762,24 @@ skl_compute_wm(struct intel_atomic_state > > > *state) > > > ret = skl_build_pipe_wm(new_crtc_state); > > > if (ret) > > > return ret; > > > - > > > - ret = skl_wm_add_affected_planes(state, crtc); > > > - if (ret) > > > - return ret; > > > } > > > > > > ret = skl_compute_ddb(state); > > > if (ret) > > > return ret; > > > > > > + /* > > > + * skl_compute_ddb() will have adjusted the final watermarks > > > + * based on how much ddb is available. Now we can actually > > > + * check if the final watermarks changed. > > > + */ > > > + for_each_oldnew_intel_crtc_in_state(state, crtc, > > > old_crtc_state, > > > + new_crtc_state, i) { > > > + ret = skl_wm_add_affected_planes(state, crtc); > > > + if (ret) > > > + return ret; > > > + } > > > > skl_compute_ddb() is already calling skl_wm_add_affected_planes() > > after > > do the ddb allocation for each pipe, so we could remove this chunk, > > skl_compute_ddb() calls skl_*ddb*_add_affected_planes(), which is a > different thing.. Thanks Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > with that: > > > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > > > + > > > skl_print_wm_changes(state); > > > > > > return 0;
On Wed, Mar 04, 2020 at 11:25:42PM +0000, Souza, Jose wrote: > On Wed, 2020-03-04 at 13:46 +0200, Ville Syrjälä wrote: > > On Wed, Mar 04, 2020 at 12:21:01AM +0000, Souza, Jose wrote: > > > On Fri, 2020-02-28 at 22:35 +0200, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Currently we're comparing the watermarks between the old and new > > > > states > > > > before we've fully computed the new watermarks. In particular > > > > skl_build_pipe_wm() will not account for the amount of ddb space > > > > we'll > > > > have. That information is only available during skl_compute_ddb() > > > > which will proceed to zero out any watermark level exceeding the > > > > ddb allocation. If we're short on ddb space this will end up > > > > adding the plane to the state due erronously determining that the > > > > watermarks have changed. Fix the problem by deferring > > > > skl_wm_add_affected_planes() until we have the final watermarks > > > > computed. > > > > > > > > Noticed this when trying enable transition watermarks on glk. > > > > We now computed the trans_wm as 28, but we only had 14 blocks > > > > of ddb, and thus skl_compute_ddb() ended up disabling the cursor > > > > trans_wm every time. Thus we ended up adding the cursor to every > > > > commit that didn't actually affect the cursor at all. > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++---- > > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > > > b/drivers/gpu/drm/i915/intel_pm.c > > > > index 39299811b650..a3d76e69caae 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -5762,16 +5762,24 @@ skl_compute_wm(struct intel_atomic_state > > > > *state) > > > > ret = skl_build_pipe_wm(new_crtc_state); > > > > if (ret) > > > > return ret; > > > > - > > > > - ret = skl_wm_add_affected_planes(state, crtc); > > > > - if (ret) > > > > - return ret; > > > > } > > > > > > > > ret = skl_compute_ddb(state); > > > > if (ret) > > > > return ret; > > > > > > > > + /* > > > > + * skl_compute_ddb() will have adjusted the final watermarks > > > > + * based on how much ddb is available. Now we can actually > > > > + * check if the final watermarks changed. > > > > + */ > > > > + for_each_oldnew_intel_crtc_in_state(state, crtc, > > > > old_crtc_state, > > > > + new_crtc_state, i) { > > > > + ret = skl_wm_add_affected_planes(state, crtc); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > > > skl_compute_ddb() is already calling skl_wm_add_affected_planes() > > > after > > > do the ddb allocation for each pipe, so we could remove this chunk, > > > > skl_compute_ddb() calls skl_*ddb*_add_affected_planes(), which is a > > different thing.. > > Thanks No, thank you for the review. Series pushed to dinq. > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > > > > with that: > > > > > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > > > > > + > > > > skl_print_wm_changes(state); > > > > > > > > return 0;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 39299811b650..a3d76e69caae 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5762,16 +5762,24 @@ skl_compute_wm(struct intel_atomic_state *state) ret = skl_build_pipe_wm(new_crtc_state); if (ret) return ret; - - ret = skl_wm_add_affected_planes(state, crtc); - if (ret) - return ret; } ret = skl_compute_ddb(state); if (ret) return ret; + /* + * skl_compute_ddb() will have adjusted the final watermarks + * based on how much ddb is available. Now we can actually + * check if the final watermarks changed. + */ + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, + new_crtc_state, i) { + ret = skl_wm_add_affected_planes(state, crtc); + if (ret) + return ret; + } + skl_print_wm_changes(state); return 0;