Op 17-01-17 om 02:01 schreef Laurent Pinchart:
> Hi Maarten,
>
> (CC'ing Daniel)
>
> Thank you for the patch.
>
> On Monday 16 Jan 2017 10:37:41 Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++++---------------
>> 1 file changed, 152 insertions(+), 141 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c index d153e8a72921..b26cf786ce12
>> 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> [snip]
>
>> @@ -245,22 +246,22 @@ steal_encoder(struct drm_atomic_state *state,
>> {
>> struct drm_crtc_state *crtc_state;
>> struct drm_connector *connector;
>> - struct drm_connector_state *connector_state;
>> + struct drm_connector_state *old_connector_state, *new_connector_state;
> The kernel favours one variable declaration per line, and I think this file
> does as well, at least mostly (this comment holds for the rest of this patch
> and the other patches in the series).
This is the first I've heard of it..
~/nfs/linux$ git grep 'int i,' | wc -l
8490
~/nfs/linux$ git grep 'int i;' | wc -l
23636
Based on this, I don't think it's uncommon to have multiple declarations per line.
>> int i;
>>
>> - for_each_connector_in_state(state, connector, connector_state, i) {
>> + for_each_oldnew_connector_in_state(state, connector,
>> old_connector_state, new_connector_state, i) {
> This is getting quite long, you could wrap the line.
Sounds good.
>> struct drm_crtc *encoder_crtc;
>>
>> - if (connector_state->best_encoder != encoder)
>> + if (new_connector_state->best_encoder != encoder)
>> continue;
>>
>> - encoder_crtc = connector->state->crtc;
>> + encoder_crtc = old_connector_state->crtc;
>>
>> DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s],
> stealing it\n",
>> encoder->base.id, encoder->name,
>> encoder_crtc->base.id, encoder_crtc->name);
>>
>> - set_best_encoder(state, connector_state, NULL);
>> + set_best_encoder(state, new_connector_state, NULL);
>>
>> crtc_state = drm_atomic_get_existing_crtc_state(state,
> encoder_crtc);
>> crtc_state->connectors_changed = true;
> [snip]
>
>> @@ -516,14 +518,15 @@ drm_atomic_helper_check_modeset(struct drm_device
>> *dev, if (ret)
>> return ret;
>>
>> - for_each_connector_in_state(state, connector, connector_state, i) {
>> + for_each_oldnew_connector_in_state(state, connector,
>> old_connector_state, new_connector_state, i) {
> Line wrap here too ?
>
>> /*
>> * This only sets crtc->connectors_changed for routing
> changes,
>> * drivers must set crtc->connectors_changed themselves when
>> * connector properties need to be updated.
>> */
>> ret = update_connector_routing(state, connector,
>> - connector_state);
>> + old_connector_state,
>> + new_connector_state);
>> if (ret)
>> return ret;
>> }
> [snip]
>
>> @@ -599,15 +602,15 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>> struct drm_crtc *crtc;
>> struct drm_crtc_state *crtc_state;
>> struct drm_plane *plane;
>> - struct drm_plane_state *plane_state;
>> + struct drm_plane_state *plane_state, *old_plane_state;
> In some location you use new_*_state and in others *_state. I believe this
> should this be standardized to avoid confusion (the code is hard enough to
> read as it is :-)).
>
> Similarly, you sometimes use *_conn_state and sometimes *_connector_state.
> That should be standardized as well.
Indeed, at least for those that use both.
>> int i, ret = 0;
>>
>> - for_each_plane_in_state(state, plane, plane_state, i) {
>> + for_each_oldnew_plane_in_state(state, plane, old_plane_state,
>> plane_state, i) {
>> const struct drm_plane_helper_funcs *funcs;
>>
>> funcs = plane->helper_private;
>>
>> - drm_atomic_helper_plane_changed(state, plane_state, plane);
>> + drm_atomic_helper_plane_changed(state, old_plane_state,
>> plane_state, plane);
>>
>> if (!funcs || !funcs->atomic_check)
>> continue;
> [snip]
>
>> @@ -974,18 +977,18 @@ void drm_atomic_helper_commit_modeset_enables(struct
>> drm_device *dev, }
>> }
>>
>> - for_each_connector_in_state(old_state, connector, old_conn_state, i) {
>> + for_each_new_connector_in_state(old_state, connector, conn_state, i) {
> Not strictly related to this patch, but I've been wondering how this works,
> given that we need to loop over connectors in the new state, not the old
> state. After some investigation I've realized that both the old and new states
> contain the same list of objects, as we don't keep a copy of the old global
> atomic state. old_state was the new state before the state swap, and the swap
> operation sets state->/objects/[*].state to the old state for all objects, but
> doesn't modify the object pointers. This is possibly more of a question for
> Daniel, should this be captured in the documentation somewhere (and is my
> understanding correct) ?
Yes. But that will go away soon after this patch + all drivers converted.
This is why get_state should not be called during atomic commit, and get_existing_state will be removed.
>> const struct drm_encoder_helper_funcs *funcs;
>> struct drm_encoder *encoder;
>>
>> - if (!connector->state->best_encoder)
>> + if (!conn_state->best_encoder)
>> continue;
>>
>> - if (!connector->state->crtc->state->active ||
>> - !drm_atomic_crtc_needs_modeset(connector->state->crtc-
>> state))
>> + if (!conn_state->crtc->state->active ||
>> + !drm_atomic_crtc_needs_modeset(conn_state->crtc->state))
>> continue;
>>
>> - encoder = connector->state->best_encoder;
>> + encoder = conn_state->best_encoder;
>> funcs = encoder->helper_private;
>>
>> DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
> [snip]
>
>> @@ -1921,12 +1919,19 @@ void drm_atomic_helper_cleanup_planes(struct
>> drm_device *dev, struct drm_atomic_state *old_state)
>> {
>> struct drm_plane *plane;
>> - struct drm_plane_state *plane_state;
>> + struct drm_plane_state *plane_state, *new_plane_state;
>> int i;
>>
>> - for_each_plane_in_state(old_state, plane, plane_state, i) {
>> + for_each_oldnew_plane_in_state(old_state, plane, plane_state,
>> new_plane_state, i) {
>> const struct drm_plane_helper_funcs *funcs;
>>
>> + /*
>> + * This might be called before swapping when commit is
> aborted,
>> + * in which case we have to free the new state.
> Should this be "cleanup the new state" instead of "free the new state" ?
Indeed.
>> + */
>> + if (plane_state == plane->state)
>> + plane_state = new_plane_state;
>> +
>> funcs = plane->helper_private;
>>
>> if (funcs->cleanup_fb)
> [snip]
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
