Re: [PATCH 3/5] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v2.

Maarten Lankhorst
2017-08-30T13:34:34Z

Op 30-08-17 om 15:09 schreef Laurent Pinchart:
> Hi Maarten,
>
> Thank you for the patch.
>
> On Wednesday, 30 August 2017 15:17:50 EEST Maarten Lankhorst wrote:
>> Most code only cares about the current commit or previous commit.
>> Fortuantely we already have a place to track those. Move it to
>> drm_crtc_state where it belongs. :)
>>
>> The per-crtc commit_list is kept for places where we have to look
>> deeper than the current or previous commit for checking whether to stall
>> on unpin. This is used in drm_atomic_helper_setup_commit and
>> intel_has_pending_fb_unpin.
>>
>> Changes since v1:
>> - Update kerneldoc for drm_crtc.commit_list. (danvet)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>> drivers/gpu/drm/drm_atomic.c | 7 ---
>> drivers/gpu/drm/drm_atomic_helper.c | 92 +++++++++-------------------------
>> include/drm/drm_atomic.h | 1 -
>> include/drm/drm_crtc.h | 23 ++++++++--
>> 4 files changed, 42 insertions(+), 81 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 2fd383d7253a..2cce48f203e0 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct
>> drm_atomic_state *state) crtc->funcs->atomic_destroy_state(crtc,
>> state->crtcs[i].state);
>>
>> - if (state->crtcs[i].commit) {
>> - kfree(state->crtcs[i].commit->event);
>> - state->crtcs[i].commit->event = NULL;
>> - drm_crtc_commit_put(state->crtcs[i].commit);
>> - }
>> -
>> - state->crtcs[i].commit = NULL;
>> state->crtcs[i].ptr = NULL;
>> state->crtcs[i].state = NULL;
>> }
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c index 11d0e94a2181..8ccb8b6536c0
>> 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>> void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
>> struct drm_atomic_state *old_state)
>> {
>> - struct drm_crtc_state *unused;
>> + struct drm_crtc_state *new_crtc_state;
>> struct drm_crtc *crtc;
>> int i;
>>
>> - for_each_new_crtc_in_state(old_state, crtc, unused, i) {
>> - struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
>> + for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
>> + struct drm_crtc_commit *commit = new_crtc_state->commit;
>> int ret;
>>
>> if (!commit)
>> @@ -1388,11 +1388,10 @@ int drm_atomic_helper_async_check(struct drm_device
>> *dev, {
>> struct drm_crtc *crtc;
>> struct drm_crtc_state *crtc_state;
>> - struct drm_crtc_commit *commit;
>> struct drm_plane *__plane, *plane = NULL;
>> struct drm_plane_state *__plane_state, *plane_state = NULL;
>> const struct drm_plane_helper_funcs *funcs;
>> - int i, j, n_planes = 0;
>> + int i, n_planes = 0;
>>
>> for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>> if (drm_atomic_crtc_needs_modeset(crtc_state))
>> @@ -1420,33 +1419,10 @@ int drm_atomic_helper_async_check(struct drm_device
>> *dev, return -EINVAL;
>>
>> /*
>> - * Don't do an async update if there is an outstanding commit modifying
>> + * TODO: Don't do an async update if there is an outstanding commit
>> modifying
>> * the plane. This prevents our async update's changes from getting
>> * overridden by a previous synchronous update's state.
>> */
> As mentioned in a comment to your previous patch, this is unrelated to
> $SUBJECT. You should mention and explain this change in the commit message
> (possibly splitting it out in a separate commit if you think that would make
> more sense, up to you).
I'm all for removing this, it doesn't work.. But I g uess it can be kept in for now and I'll drop this hunk.