| Message ID | 1476278901-15750-2-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 12, 2016 at 03:28:14PM +0200, Maarten Lankhorst wrote: > Caching is not required, drm_atomic_crtc_state_for_each_plane_state > can be used to inspect all plane_states that are assigned to the > current crtc_state, so we can just recalculate every time. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.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 6af1587e9d84..b96a899c899d 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -31,6 +31,7 @@ > #include "intel_drv.h" > #include "../../../platform/x86/intel_ips.h" > #include <linux/module.h> > +#include <drm/drm_atomic_helper.h> > > /** > * DOC: RC6 > @@ -3242,18 +3243,17 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate) > struct drm_crtc *crtc = cstate->crtc; > struct drm_device *dev = crtc->dev; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - const struct drm_plane *plane; > + struct drm_plane *plane; > const struct intel_plane *intel_plane; > - struct drm_plane_state *pstate; > + const struct drm_plane_state *pstate; > unsigned int rate, total_data_rate = 0; > int id; > - int i; > > if (WARN_ON(!state)) > return 0; > > /* Calculate and cache data rate for each plane */ > - for_each_plane_in_state(state, plane, pstate, i) { > + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) { > id = skl_wm_plane_id(to_intel_plane(plane)); > intel_plane = to_intel_plane(plane); > > @@ -3356,7 +3356,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_plane *intel_plane; > struct drm_plane *plane; > - struct drm_plane_state *pstate; > + const struct drm_plane_state *pstate; > enum pipe pipe = intel_crtc->pipe; > struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb; > uint16_t alloc_size, start, cursor_blocks; > @@ -3392,14 +3392,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, > alloc_size -= cursor_blocks; > > /* 1. Allocate the mininum required blocks for each active plane */ > - for_each_plane_in_state(state, plane, pstate, i) { > + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) { > intel_plane = to_intel_plane(plane); > id = skl_wm_plane_id(intel_plane); > > if (intel_plane->pipe != pipe) > continue; > > - if (!to_intel_plane_state(pstate)->base.visible) { > + if (!pstate->visible) { > minimum[id] = 0; > y_minimum[id] = 0; > continue; > @@ -3948,7 +3948,7 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate) > > WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc)); > > - drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) { > + drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) { Is this change necessary? Any plane that differs between the two masks should already be part of our state, so I don't think this changes the behavior at all. The original 'crtc->state->plane_mask' form is closer to the drm_atomic_add_affected_planes() that this function is modeled after so my slight preference would be to leave it alone for consistency. Aside from that, this patch is Reviewed-by: Matt Roper <matthew.d.roper@intel.com> I remember when I first wrote an early version of this code it was just doing a drm_atomic_get_plane_state() on each plane unconditionally, which was non-optimal. When I reworked it, I wasn't aware of drm_atomic_crtc_state_for_each_plane_state (or maybe it didn't exist yet), so I used caching as an alternative. But the new approach is much better so I'm glad we can get rid of the caching. Matt > id = skl_wm_plane_id(to_intel_plane(plane)); > > if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][id], > @@ -4063,14 +4063,12 @@ skl_print_wm_changes(const struct drm_atomic_state *state) > to_intel_atomic_state(state); > const struct drm_crtc *crtc; > const struct drm_crtc_state *cstate; > - const struct drm_plane *plane; > const struct intel_plane *intel_plane; > - const struct drm_plane_state *pstate; > const struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb; > const struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb; > enum pipe pipe; > int id; > - int i, j; > + int i; > > for_each_crtc_in_state(state, crtc, cstate, i) { > if (!crtc->state) > @@ -4078,10 +4076,9 @@ skl_print_wm_changes(const struct drm_atomic_state *state) > > pipe = to_intel_crtc(crtc)->pipe; > > - for_each_plane_in_state(state, plane, pstate, j) { > + for_each_intel_plane_on_crtc(dev, to_intel_crtc(crtc), intel_plane) { > const struct skl_ddb_entry *old, *new; > > - intel_plane = to_intel_plane(plane); > id = skl_wm_plane_id(intel_plane); > old = &old_ddb->plane[pipe][id]; > new = &new_ddb->plane[pipe][id]; > @@ -4094,13 +4091,13 @@ skl_print_wm_changes(const struct drm_atomic_state *state) > > if (id != PLANE_CURSOR) { > DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb (%d - %d) -> (%d - %d)\n", > - plane->base.id, id + 1, > + intel_plane->base.base.id, id + 1, > pipe_name(pipe), > old->start, old->end, > new->start, new->end); > } else { > DRM_DEBUG_ATOMIC("[PLANE:%d:cursor %c] ddb (%d - %d) -> (%d - %d)\n", > - plane->base.id, > + intel_plane->base.base.id, > pipe_name(pipe), > old->start, old->end, > new->start, new->end); > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 20-10-16 om 00:13 schreef Matt Roper: > On Wed, Oct 12, 2016 at 03:28:14PM +0200, Maarten Lankhorst wrote: >> Caching is not required, drm_atomic_crtc_state_for_each_plane_state >> can be used to inspect all plane_states that are assigned to the >> current crtc_state, so we can just recalculate every time. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.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 6af1587e9d84..b96a899c899d 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -31,6 +31,7 @@ >> #include "intel_drv.h" >> #include "../../../platform/x86/intel_ips.h" >> #include <linux/module.h> >> +#include <drm/drm_atomic_helper.h> >> >> /** >> * DOC: RC6 >> @@ -3242,18 +3243,17 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate) >> struct drm_crtc *crtc = cstate->crtc; >> struct drm_device *dev = crtc->dev; >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> - const struct drm_plane *plane; >> + struct drm_plane *plane; >> const struct intel_plane *intel_plane; >> - struct drm_plane_state *pstate; >> + const struct drm_plane_state *pstate; >> unsigned int rate, total_data_rate = 0; >> int id; >> - int i; >> >> if (WARN_ON(!state)) >> return 0; >> >> /* Calculate and cache data rate for each plane */ >> - for_each_plane_in_state(state, plane, pstate, i) { >> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) { >> id = skl_wm_plane_id(to_intel_plane(plane)); >> intel_plane = to_intel_plane(plane); >> >> @@ -3356,7 +3356,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> struct intel_plane *intel_plane; >> struct drm_plane *plane; >> - struct drm_plane_state *pstate; >> + const struct drm_plane_state *pstate; >> enum pipe pipe = intel_crtc->pipe; >> struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb; >> uint16_t alloc_size, start, cursor_blocks; >> @@ -3392,14 +3392,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, >> alloc_size -= cursor_blocks; >> >> /* 1. Allocate the mininum required blocks for each active plane */ >> - for_each_plane_in_state(state, plane, pstate, i) { >> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) { >> intel_plane = to_intel_plane(plane); >> id = skl_wm_plane_id(intel_plane); >> >> if (intel_plane->pipe != pipe) >> continue; >> >> - if (!to_intel_plane_state(pstate)->base.visible) { >> + if (!pstate->visible) { >> minimum[id] = 0; >> y_minimum[id] = 0; >> continue; >> @@ -3948,7 +3948,7 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate) >> >> WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc)); >> >> - drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) { >> + drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) { > Is this change necessary? Any plane that differs between the two masks > should already be part of our state, so I don't think this changes the > behavior at all. The original 'crtc->state->plane_mask' form is closer > to the drm_atomic_add_affected_planes() that this function is modeled > after so my slight preference would be to leave it alone for > consistency. > > Aside from that, this patch is Not completely, but I was removing it since I'm trying to get rid of all pointers to obj->state as much as I can. All accesses should be through some wrapper functions, still working out the specifics. ~Maarten
Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu: > Caching is not required, drm_atomic_crtc_state_for_each_plane_state > can be used to inspect all plane_states that are assigned to the > current crtc_state, so we can just recalculate every time. But can't the current for_each_plane_in_state() do the same thing? Why is the new macro better? What's the real point here? As someone who just downloaded the series and started looking at patch 1 without looking at the others, this commit message makes zero sense. I'd really like if you could explain how the paragraph above is connected with the patch below. What does the macro really have to do with caching? Perhaps you could elaborate more on the plans of the next patches and explain how the changes below enable the grand plan. Please do this in the commit message instead of just an email reply. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.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 6af1587e9d84..b96a899c899d 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -31,6 +31,7 @@ > #include "intel_drv.h" > #include "../../../platform/x86/intel_ips.h" > #include <linux/module.h> > +#include <drm/drm_atomic_helper.h> > > /** > * DOC: RC6 > @@ -3242,18 +3243,17 @@ skl_get_total_relative_data_rate(struct > intel_crtc_state *intel_cstate) > struct drm_crtc *crtc = cstate->crtc; > struct drm_device *dev = crtc->dev; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - const struct drm_plane *plane; > + struct drm_plane *plane; > const struct intel_plane *intel_plane; > - struct drm_plane_state *pstate; > + const struct drm_plane_state *pstate; > unsigned int rate, total_data_rate = 0; > int id; > - int i; > > if (WARN_ON(!state)) > return 0; > > /* Calculate and cache data rate for each plane */ > - for_each_plane_in_state(state, plane, pstate, i) { > + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, > cstate) { Now we can cut the "if (intel_plane->pipe != intel_crtc->pipe)" check this code has. > id = skl_wm_plane_id(to_intel_plane(plane)); > intel_plane = to_intel_plane(plane); > > @@ -3356,7 +3356,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state > *cstate, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_plane *intel_plane; > struct drm_plane *plane; > - struct drm_plane_state *pstate; > + const struct drm_plane_state *pstate; > enum pipe pipe = intel_crtc->pipe; > struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb; > uint16_t alloc_size, start, cursor_blocks; > @@ -3392,14 +3392,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state > *cstate, > alloc_size -= cursor_blocks; > > /* 1. Allocate the mininum required blocks for each active > plane */ > - for_each_plane_in_state(state, plane, pstate, i) { > + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, > &cstate->base) { > intel_plane = to_intel_plane(plane); > id = skl_wm_plane_id(intel_plane); > > if (intel_plane->pipe != pipe) > continue; Same thing here: cut the check above? > > - if (!to_intel_plane_state(pstate)->base.visible) { > + if (!pstate->visible) { I lol'd :) I'd probably have done this in a separate patch, since it doesn't seem to match the commit message. > minimum[id] = 0; > y_minimum[id] = 0; > continue; > @@ -3948,7 +3948,7 @@ skl_ddb_add_affected_planes(struct > intel_crtc_state *cstate) > > WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc)); > > - drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) > { > + drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) This should be in a separate patch with a separate commit message explaining what exactly changes and why the current code is bad. And as Matt pointed, this code is completely based on drm_atomic_add_affected_planes(), so if there's something to fix here, there's probably something to fix there. > { > id = skl_wm_plane_id(to_intel_plane(plane)); > > if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][id], > @@ -4063,14 +4063,12 @@ skl_print_wm_changes(const struct > drm_atomic_state *state) > to_intel_atomic_state(state); > const struct drm_crtc *crtc; > const struct drm_crtc_state *cstate; > - const struct drm_plane *plane; > const struct intel_plane *intel_plane; > - const struct drm_plane_state *pstate; > const struct skl_ddb_allocation *old_ddb = &dev_priv- > >wm.skl_hw.ddb; > const struct skl_ddb_allocation *new_ddb = &intel_state- > >wm_results.ddb; > enum pipe pipe; > int id; > - int i, j; > + int i; > > for_each_crtc_in_state(state, crtc, cstate, i) { > if (!crtc->state) > @@ -4078,10 +4076,9 @@ skl_print_wm_changes(const struct > drm_atomic_state *state) > > pipe = to_intel_crtc(crtc)->pipe; > > - for_each_plane_in_state(state, plane, pstate, j) { > + for_each_intel_plane_on_crtc(dev, > to_intel_crtc(crtc), intel_plane) { > const struct skl_ddb_entry *old, *new; > > - intel_plane = to_intel_plane(plane); > id = skl_wm_plane_id(intel_plane); > old = &old_ddb->plane[pipe][id]; > new = &new_ddb->plane[pipe][id]; > @@ -4094,13 +4091,13 @@ skl_print_wm_changes(const struct > drm_atomic_state *state) > > if (id != PLANE_CURSOR) { > DRM_DEBUG_ATOMIC("[PLANE:%d:plane > %d%c] ddb (%d - %d) -> (%d - %d)\n", > - plane->base.id, id > + 1, > + intel_plane- > >base.base.id, id + 1, > pipe_name(pipe), > old->start, old- > >end, > new->start, new- > >end); > } else { > DRM_DEBUG_ATOMIC("[PLANE:%d:cursor > %c] ddb (%d - %d) -> (%d - %d)\n", > - plane->base.id, > + intel_plane- > >base.base.id, > pipe_name(pipe), > old->start, old- > >end, > new->start, new- > >end); This chunk is another chunk that looks like it belongs to a separate patch. What does it have to do with the commit message above? It doesn't even look at the macro you mention. Thanks, Paulo
Op 20-10-16 om 15:11 schreef Paulo Zanoni: > Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu: >> Caching is not required, drm_atomic_crtc_state_for_each_plane_state >> can be used to inspect all plane_states that are assigned to the >> current crtc_state, so we can just recalculate every time. > But can't the current for_each_plane_in_state() do the same thing? Why > is the new macro better? What's the real point here? for_each_plane_in_state looks at all planes in the current atomic state. It doesn't enumerate planes on a crtc that are not part of it. This macro takes a crtc_state, and enumerates all plane_states assigned to it, whether they are part of the atomic state or not. This can be done because acquiring a plane state also requires acquiring crtc_state. Updating the plane state with this macro is not allowed, because it requires that the plane has to be part of the atomic state. This is why a const drm_plane_state is returned. > As someone who just downloaded the series and started looking at patch > 1 without looking at the others, this commit message makes zero sense. > I'd really like if you could explain how the paragraph above is > connected with the patch below. What does the macro really have to do > with caching? Perhaps you could elaborate more on the plans of the next > patches and explain how the changes below enable the grand plan. Please > do this in the commit message instead of just an email reply. > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.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 6af1587e9d84..b96a899c899d 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -31,6 +31,7 @@ >> #include "intel_drv.h" >> #include "../../../platform/x86/intel_ips.h" >> #include <linux/module.h> >> +#include <drm/drm_atomic_helper.h> >> >> /** >> * DOC: RC6 >> @@ -3242,18 +3243,17 @@ skl_get_total_relative_data_rate(struct >> intel_crtc_state *intel_cstate) >> struct drm_crtc *crtc = cstate->crtc; >> struct drm_device *dev = crtc->dev; >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> - const struct drm_plane *plane; >> + struct drm_plane *plane; >> const struct intel_plane *intel_plane; >> - struct drm_plane_state *pstate; >> + const struct drm_plane_state *pstate; >> unsigned int rate, total_data_rate = 0; >> int id; >> - int i; >> >> if (WARN_ON(!state)) >> return 0; >> >> /* Calculate and cache data rate for each plane */ >> - for_each_plane_in_state(state, plane, pstate, i) { >> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, >> cstate) { > > Now we can cut the "if (intel_plane->pipe != intel_crtc->pipe)" check > this code has. > >> id = skl_wm_plane_id(to_intel_plane(plane)); >> intel_plane = to_intel_plane(plane); >> >> @@ -3356,7 +3356,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state >> *cstate, >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> struct intel_plane *intel_plane; >> struct drm_plane *plane; >> - struct drm_plane_state *pstate; >> + const struct drm_plane_state *pstate; >> enum pipe pipe = intel_crtc->pipe; >> struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb; >> uint16_t alloc_size, start, cursor_blocks; >> @@ -3392,14 +3392,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state >> *cstate, >> alloc_size -= cursor_blocks; >> >> /* 1. Allocate the mininum required blocks for each active >> plane */ >> - for_each_plane_in_state(state, plane, pstate, i) { >> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, >> &cstate->base) { >> intel_plane = to_intel_plane(plane); >> id = skl_wm_plane_id(intel_plane); >> >> if (intel_plane->pipe != pipe) >> continue; > Same thing here: cut the check above? > >> >> - if (!to_intel_plane_state(pstate)->base.visible) { >> + if (!pstate->visible) { > I lol'd :) > I'd probably have done this in a separate patch, since it doesn't seem > to match the commit message. > >> minimum[id] = 0; >> y_minimum[id] = 0; >> continue; >> @@ -3948,7 +3948,7 @@ skl_ddb_add_affected_planes(struct >> intel_crtc_state *cstate) >> >> WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc)); >> >> - drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) >> { >> + drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) > > This should be in a separate patch with a separate commit message > explaining what exactly changes and why the current code is bad. And as > Matt pointed, this code is completely based > on drm_atomic_add_affected_planes(), so if there's something to fix > here, there's probably something to fix there. The subset that we care about here is crtc->state->plane_mask & cstate->base.plane_mask. Nothing to fix here, either way is correct. But using cstate instead of crtc->state is nice for removing obj->state later on. > >> { >> id = skl_wm_plane_id(to_intel_plane(plane)); >> >> if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][id], >> @@ -4063,14 +4063,12 @@ skl_print_wm_changes(const struct >> drm_atomic_state *state) >> to_intel_atomic_state(state); >> const struct drm_crtc *crtc; >> const struct drm_crtc_state *cstate; >> - const struct drm_plane *plane; >> const struct intel_plane *intel_plane; >> - const struct drm_plane_state *pstate; >> const struct skl_ddb_allocation *old_ddb = &dev_priv- >>> wm.skl_hw.ddb; >> const struct skl_ddb_allocation *new_ddb = &intel_state- >>> wm_results.ddb; >> enum pipe pipe; >> int id; >> - int i, j; >> + int i; >> >> for_each_crtc_in_state(state, crtc, cstate, i) { >> if (!crtc->state) >> @@ -4078,10 +4076,9 @@ skl_print_wm_changes(const struct >> drm_atomic_state *state) >> >> pipe = to_intel_crtc(crtc)->pipe; >> >> - for_each_plane_in_state(state, plane, pstate, j) { >> + for_each_intel_plane_on_crtc(dev, >> to_intel_crtc(crtc), intel_plane) { >> const struct skl_ddb_entry *old, *new; >> >> - intel_plane = to_intel_plane(plane); >> id = skl_wm_plane_id(intel_plane); >> old = &old_ddb->plane[pipe][id]; >> new = &new_ddb->plane[pipe][id]; >> @@ -4094,13 +4091,13 @@ skl_print_wm_changes(const struct >> drm_atomic_state *state) >> >> if (id != PLANE_CURSOR) { >> DRM_DEBUG_ATOMIC("[PLANE:%d:plane >> %d%c] ddb (%d - %d) -> (%d - %d)\n", >> - plane->base.id, id >> + 1, >> + intel_plane- >>> base.base.id, id + 1, >> pipe_name(pipe), >> old->start, old- >>> end, >> new->start, new- >>> end); >> } else { >> DRM_DEBUG_ATOMIC("[PLANE:%d:cursor >> %c] ddb (%d - %d) -> (%d - %d)\n", >> - plane->base.id, >> + intel_plane- >>> base.base.id, >> pipe_name(pipe), >> old->start, old- >>> end, >> new->start, new- >>> end); > This chunk is another chunk that looks like it belongs to a separate > patch. What does it have to do with the commit message above? It > doesn't even look at the macro you mention. Agreed, should be split out.
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 6af1587e9d84..b96a899c899d 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -31,6 +31,7 @@ #include "intel_drv.h" #include "../../../platform/x86/intel_ips.h" #include <linux/module.h> +#include <drm/drm_atomic_helper.h> /** * DOC: RC6 @@ -3242,18 +3243,17 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate) struct drm_crtc *crtc = cstate->crtc; struct drm_device *dev = crtc->dev; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - const struct drm_plane *plane; + struct drm_plane *plane; const struct intel_plane *intel_plane; - struct drm_plane_state *pstate; + const struct drm_plane_state *pstate; unsigned int rate, total_data_rate = 0; int id; - int i; if (WARN_ON(!state)) return 0; /* Calculate and cache data rate for each plane */ - for_each_plane_in_state(state, plane, pstate, i) { + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) { id = skl_wm_plane_id(to_intel_plane(plane)); intel_plane = to_intel_plane(plane); @@ -3356,7 +3356,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_plane *intel_plane; struct drm_plane *plane; - struct drm_plane_state *pstate; + const struct drm_plane_state *pstate; enum pipe pipe = intel_crtc->pipe; struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb; uint16_t alloc_size, start, cursor_blocks; @@ -3392,14 +3392,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, alloc_size -= cursor_blocks; /* 1. Allocate the mininum required blocks for each active plane */ - for_each_plane_in_state(state, plane, pstate, i) { + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) { intel_plane = to_intel_plane(plane); id = skl_wm_plane_id(intel_plane); if (intel_plane->pipe != pipe) continue; - if (!to_intel_plane_state(pstate)->base.visible) { + if (!pstate->visible) { minimum[id] = 0; y_minimum[id] = 0; continue; @@ -3948,7 +3948,7 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate) WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc)); - drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) { + drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) { id = skl_wm_plane_id(to_intel_plane(plane)); if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][id], @@ -4063,14 +4063,12 @@ skl_print_wm_changes(const struct drm_atomic_state *state) to_intel_atomic_state(state); const struct drm_crtc *crtc; const struct drm_crtc_state *cstate; - const struct drm_plane *plane; const struct intel_plane *intel_plane; - const struct drm_plane_state *pstate; const struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb; const struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb; enum pipe pipe; int id; - int i, j; + int i; for_each_crtc_in_state(state, crtc, cstate, i) { if (!crtc->state) @@ -4078,10 +4076,9 @@ skl_print_wm_changes(const struct drm_atomic_state *state) pipe = to_intel_crtc(crtc)->pipe; - for_each_plane_in_state(state, plane, pstate, j) { + for_each_intel_plane_on_crtc(dev, to_intel_crtc(crtc), intel_plane) { const struct skl_ddb_entry *old, *new; - intel_plane = to_intel_plane(plane); id = skl_wm_plane_id(intel_plane); old = &old_ddb->plane[pipe][id]; new = &new_ddb->plane[pipe][id]; @@ -4094,13 +4091,13 @@ skl_print_wm_changes(const struct drm_atomic_state *state) if (id != PLANE_CURSOR) { DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb (%d - %d) -> (%d - %d)\n", - plane->base.id, id + 1, + intel_plane->base.base.id, id + 1, pipe_name(pipe), old->start, old->end, new->start, new->end); } else { DRM_DEBUG_ATOMIC("[PLANE:%d:cursor %c] ddb (%d - %d) -> (%d - %d)\n", - plane->base.id, + intel_plane->base.base.id, pipe_name(pipe), old->start, old->end, new->start, new->end);
Caching is not required, drm_atomic_crtc_state_for_each_plane_state can be used to inspect all plane_states that are assigned to the current crtc_state, so we can just recalculate every time. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)