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@xxxxxxxxxxxxxxx>
>> ---
>> 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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
