| Message ID | 20180607230700.28359-1-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Paulo Zanoni (2018-06-08 00:07:00) > static void > skl_print_wm_changes(const struct drm_atomic_state *state) > { > @@ -5381,7 +5370,10 @@ static void skl_initial_wm(struct intel_atomic_state *state, > if (cstate->base.active_changed) > skl_atomic_update_crtc_wm(state, cstate); > > - skl_copy_ddb_for_pipe(hw_vals, results, pipe); > + memcpy(hw_vals->ddb.uv_plane[pipe], results->ddb.uv_plane[pipe], > + sizeof(hw_vals->ddb.uv_plane[pipe])); I must be seeing things. ddb.uv_plane[pipe] must be a pointer, right? Therefore sizeof(ddb.uv_plane[pipe]) must the size of that pointer and not of the struct. Do you not mean sizeof(*ddb.uv_plane[pipe]) ? Definitely time to stop reading code... -Chris
Em Sex, 2018-06-08 às 00:49 +0100, Chris Wilson escreveu: > Quoting Paulo Zanoni (2018-06-08 00:07:00) > > static void > > skl_print_wm_changes(const struct drm_atomic_state *state) > > { > > @@ -5381,7 +5370,10 @@ static void skl_initial_wm(struct > > intel_atomic_state *state, > > if (cstate->base.active_changed) > > skl_atomic_update_crtc_wm(state, cstate); > > > > - skl_copy_ddb_for_pipe(hw_vals, results, pipe); > > + memcpy(hw_vals->ddb.uv_plane[pipe], results- > > >ddb.uv_plane[pipe], > > + sizeof(hw_vals->ddb.uv_plane[pipe])); > > I must be seeing things. > > ddb.uv_plane[pipe] must be a pointer, right? No, it's an array of structs. > Therefore sizeof(ddb.uv_plane[pipe]) must the size of that pointer > and > not of the struct. It is the size of the array, which is 20 (both before and after the patch). Each member has size 4. > Do you not mean sizeof(*ddb.uv_plane[pipe]) ? What's written above means "size of the first element of ddb.uv_plane[pipe]". This sizeof operation returns 4 instead of 20. We don't want to copy just the first element of uv_plane[pipe]. > > Definitely time to stop reading code... > -Chris
Hi, Patch LGTM. Reviewed-by: Mahesh Kumar <mahesh1.kumar@intel.com> thanks, -Mahesh On 6/8/2018 4:37 AM, Paulo Zanoni wrote: > While things may have been different before, right now the function is > very simple and has a single caller. IMHO any possible benefits from > an abstraction here are gone and not worth the price of the current > indirection while reading the code. > > Cc: Mahesh Kumar <mahesh1.kumar@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 53aaaa3e6886..018aae9f5769 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5141,17 +5141,6 @@ skl_compute_ddb(struct drm_atomic_state *state) > return 0; > } > > -static void > -skl_copy_ddb_for_pipe(struct skl_ddb_values *dst, > - struct skl_ddb_values *src, > - enum pipe pipe) > -{ > - memcpy(dst->ddb.uv_plane[pipe], src->ddb.uv_plane[pipe], > - sizeof(dst->ddb.uv_plane[pipe])); > - memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe], > - sizeof(dst->ddb.plane[pipe])); > -} > - > static void > skl_print_wm_changes(const struct drm_atomic_state *state) > { > @@ -5381,7 +5370,10 @@ static void skl_initial_wm(struct intel_atomic_state *state, > if (cstate->base.active_changed) > skl_atomic_update_crtc_wm(state, cstate); > > - skl_copy_ddb_for_pipe(hw_vals, results, pipe); > + memcpy(hw_vals->ddb.uv_plane[pipe], results->ddb.uv_plane[pipe], > + sizeof(hw_vals->ddb.uv_plane[pipe])); > + memcpy(hw_vals->ddb.plane[pipe], results->ddb.plane[pipe], > + sizeof(hw_vals->ddb.plane[pipe])); > > mutex_unlock(&dev_priv->wm.wm_mutex); > }
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 53aaaa3e6886..018aae9f5769 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5141,17 +5141,6 @@ skl_compute_ddb(struct drm_atomic_state *state) return 0; } -static void -skl_copy_ddb_for_pipe(struct skl_ddb_values *dst, - struct skl_ddb_values *src, - enum pipe pipe) -{ - memcpy(dst->ddb.uv_plane[pipe], src->ddb.uv_plane[pipe], - sizeof(dst->ddb.uv_plane[pipe])); - memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe], - sizeof(dst->ddb.plane[pipe])); -} - static void skl_print_wm_changes(const struct drm_atomic_state *state) { @@ -5381,7 +5370,10 @@ static void skl_initial_wm(struct intel_atomic_state *state, if (cstate->base.active_changed) skl_atomic_update_crtc_wm(state, cstate); - skl_copy_ddb_for_pipe(hw_vals, results, pipe); + memcpy(hw_vals->ddb.uv_plane[pipe], results->ddb.uv_plane[pipe], + sizeof(hw_vals->ddb.uv_plane[pipe])); + memcpy(hw_vals->ddb.plane[pipe], results->ddb.plane[pipe], + sizeof(hw_vals->ddb.plane[pipe])); mutex_unlock(&dev_priv->wm.wm_mutex); }
While things may have been different before, right now the function is very simple and has a single caller. IMHO any possible benefits from an abstraction here are gone and not worth the price of the current indirection while reading the code. Cc: Mahesh Kumar <mahesh1.kumar@intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)