| Message ID | 20181016220133.26991-7-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:28PM -0700, Paulo Zanoni wrote: > Its control flow is not as easy to follow as it could be. We recently > even had a double register write that went unnoticed until commit > 9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every time") > fixed it. The return statement in the middle along with the fact that > it's on a void function really doesn't help readability IMHO. > > Refactor the function so that the first level of checks is per > platform and the second level is for planar planes. IMHO that makes > the code much more readable. > > Requested-by: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++------------- > 1 file changed, 20 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 7fd344b81d66..f388bfa99a97 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5033,21 +5033,28 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc, > skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id), > &wm->trans_wm); > > - /* FIXME: add proper NV12 support for ICL. */ > - if (INTEL_GEN(dev_priv) >= 11) > - return skl_ddb_entry_write(dev_priv, > - PLANE_BUF_CFG(pipe, plane_id), > - &ddb->plane[pipe][plane_id]); > - if (wm->is_planar) { > - skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id), > - &ddb->uv_plane[pipe][plane_id]); > + if (INTEL_GEN(dev_priv) >= 11) { > + /* FIXME: add proper NV12 support for ICL. */ > + if (wm->is_planar) > + DRM_ERROR("No DDB support for planar formats yet\n"); > + > skl_ddb_entry_write(dev_priv, > - PLANE_NV12_BUF_CFG(pipe, plane_id), > - &ddb->plane[pipe][plane_id]); > + PLANE_BUF_CFG(pipe, plane_id), > + &ddb->plane[pipe][plane_id]); > } else { > - skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id), > - &ddb->plane[pipe][plane_id]); > - I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0); > + if (wm->is_planar) { > + skl_ddb_entry_write(dev_priv, > + PLANE_BUF_CFG(pipe, plane_id), > + &ddb->uv_plane[pipe][plane_id]); > + skl_ddb_entry_write(dev_priv, > + PLANE_NV12_BUF_CFG(pipe, plane_id), > + &ddb->plane[pipe][plane_id]); > + } else { > + skl_ddb_entry_write(dev_priv, > + PLANE_BUF_CFG(pipe, plane_id), > + &ddb->plane[pipe][plane_id]); > + I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0); > + } > } > } > > -- > 2.14.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 7fd344b81d66..f388bfa99a97 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5033,21 +5033,28 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc, skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id), &wm->trans_wm); - /* FIXME: add proper NV12 support for ICL. */ - if (INTEL_GEN(dev_priv) >= 11) - return skl_ddb_entry_write(dev_priv, - PLANE_BUF_CFG(pipe, plane_id), - &ddb->plane[pipe][plane_id]); - if (wm->is_planar) { - skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id), - &ddb->uv_plane[pipe][plane_id]); + if (INTEL_GEN(dev_priv) >= 11) { + /* FIXME: add proper NV12 support for ICL. */ + if (wm->is_planar) + DRM_ERROR("No DDB support for planar formats yet\n"); + skl_ddb_entry_write(dev_priv, - PLANE_NV12_BUF_CFG(pipe, plane_id), - &ddb->plane[pipe][plane_id]); + PLANE_BUF_CFG(pipe, plane_id), + &ddb->plane[pipe][plane_id]); } else { - skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id), - &ddb->plane[pipe][plane_id]); - I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0); + if (wm->is_planar) { + skl_ddb_entry_write(dev_priv, + PLANE_BUF_CFG(pipe, plane_id), + &ddb->uv_plane[pipe][plane_id]); + skl_ddb_entry_write(dev_priv, + PLANE_NV12_BUF_CFG(pipe, plane_id), + &ddb->plane[pipe][plane_id]); + } else { + skl_ddb_entry_write(dev_priv, + PLANE_BUF_CFG(pipe, plane_id), + &ddb->plane[pipe][plane_id]); + I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0); + } } }
Its control flow is not as easy to follow as it could be. We recently even had a double register write that went unnoticed until commit 9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every time") fixed it. The return statement in the middle along with the fact that it's on a void function really doesn't help readability IMHO. Refactor the function so that the first level of checks is per platform and the second level is for planar planes. IMHO that makes the code much more readable. Requested-by: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)