Re: [PATCH v2 7/8] drm: Use mode_valid() in atomic modeset
On Fri, May 12, 2017 at 12:53:56PM +0300, Laurent Pinchart wrote: > Hi Daniel, > > On Wednesday 10 May 2017 19:55:56 Daniel Vetter wrote: > > On Wed, May 10, 2017 at 09:38:00PM +0530, Archit Taneja wrote: > > > On 5/9/2017 10:30 PM, Jose Abreu wrote: > > > > This patches makes use of the new mode_valid() callbacks introduced > > > > previously to validate the full video pipeline when modesetting. > > > > > > > > This calls the connector->mode_valid(), encoder->mode_valid(), > > > > bridge->mode_valid() and crtc->mode_valid() so that we can > > > > make sure that the mode will be accepted in every components. > > > > > > > > Signed-off-by: Jose Abreu> > > > Cc: Carlos Palminha > > > > Cc: Alexey Brodkin > > > > Cc: Ville Syrjälä > > > > Cc: Daniel Vetter > > > > Cc: Dave Airlie > > > > Cc: Andrzej Hajda > > > > Cc: Archit Taneja > > > > --- > > > > > > > > Changes v1->v2: > > > > - Removed call to connector->mode_valid (Ville, Daniel) > > > > - Change function name (Ville) > > > > - Use for_each_new_connector_in_state (Ville) > > > > - Do not validate if connector and mode didn't change (Ville) > > > > - Use new helpers to call mode_valid > > > > > > > > drivers/gpu/drm/drm_atomic_helper.c | 75 +++-- > > > > 1 file changed, 72 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > > > b/drivers/gpu/drm/drm_atomic_helper.c index 8be9719..19d0ea1 100644 > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > > @@ -452,6 +452,69 @@ static int handle_conflicting_encoders(struct > > > > drm_atomic_state *state, > > > > return 0; > > > > } > > > > > > > > +static enum drm_mode_status mode_valid_path(struct drm_connector > > > > *connector, > > > > + struct drm_encoder *encoder, > > > > + struct drm_crtc *crtc, > > > > + struct drm_display_mode > > > > *mode) > > > > +{ > > > > + enum drm_mode_status ret; > > > > + > > > > + ret = drm_encoder_mode_valid(encoder, mode); > > > > + if (ret != MODE_OK) { > > > > + DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() > > > > failed\n", > > > > + encoder->base.id, encoder->name); > > > > + return ret; > > > > + } > > > > + > > > > + ret = drm_bridge_mode_valid(encoder->bridge, mode); > > > > + if (ret != MODE_OK) { > > > > + DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n"); > > > > + return ret; > > > > + } > > > > + > > > > + ret = drm_crtc_mode_valid(crtc, mode); > > > > + if (ret != MODE_OK) { > > > > + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n", > > > > + crtc->base.id, crtc->name); > > > > + return ret; > > > > + } > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static int > > > > +mode_valid(struct drm_atomic_state *state) > > > > +{ > > > > + struct drm_connector_state *conn_state; > > > > + struct drm_connector *connector; > > > > + int i; > > > > + > > > > + for_each_new_connector_in_state(state, connector, conn_state, > > > > i) { > > > > + struct drm_encoder *encoder = conn_state->best_encoder; > > > > + struct drm_crtc *crtc = conn_state->crtc; > > > > + struct drm_crtc_state *crtc_state; > > > > + enum drm_mode_status mode_status; > > > > + struct drm_display_mode *mode; > > > > + > > > > + if (!crtc || !encoder) > > > > + continue; > > > > + > > > > + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > > > + if (!crtc_state) > > > > + continue; > > > > + if (!crtc_state->mode_changed && !crtc_state > > > > ->connectors_changed) > > > > + continue; > > > > + > > > > + mode = _state->mode; > > > > + > > > > + mode_status = mode_valid_path(connector, encoder, crtc, > > > > mode); > > > > + if (mode_status != MODE_OK) > > > > + return -EINVAL; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > /** > > > >* drm_atomic_helper_check_modeset - validate state object for modeset > > > >changes * @dev: DRM device > > > > @@ -466,13 +529,15 @@ static int handle_conflicting_encoders(struct > > > > drm_atomic_state *state, > > > > * 2. _connector_helper_funcs.atomic_check to validate the > > > > connector state.
Re: [PATCH v2 7/8] drm: Use mode_valid() in atomic modeset
Hi Daniel, On Wednesday 10 May 2017 19:55:56 Daniel Vetter wrote: > On Wed, May 10, 2017 at 09:38:00PM +0530, Archit Taneja wrote: > > On 5/9/2017 10:30 PM, Jose Abreu wrote: > > > This patches makes use of the new mode_valid() callbacks introduced > > > previously to validate the full video pipeline when modesetting. > > > > > > This calls the connector->mode_valid(), encoder->mode_valid(), > > > bridge->mode_valid() and crtc->mode_valid() so that we can > > > make sure that the mode will be accepted in every components. > > > > > > Signed-off-by: Jose Abreu> > > Cc: Carlos Palminha > > > Cc: Alexey Brodkin > > > Cc: Ville Syrjälä > > > Cc: Daniel Vetter > > > Cc: Dave Airlie > > > Cc: Andrzej Hajda > > > Cc: Archit Taneja > > > --- > > > > > > Changes v1->v2: > > > - Removed call to connector->mode_valid (Ville, Daniel) > > > - Change function name (Ville) > > > - Use for_each_new_connector_in_state (Ville) > > > - Do not validate if connector and mode didn't change (Ville) > > > - Use new helpers to call mode_valid > > > > > > drivers/gpu/drm/drm_atomic_helper.c | 75 +++-- > > > 1 file changed, 72 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > > b/drivers/gpu/drm/drm_atomic_helper.c index 8be9719..19d0ea1 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -452,6 +452,69 @@ static int handle_conflicting_encoders(struct > > > drm_atomic_state *state, > > > return 0; > > > } > > > > > > +static enum drm_mode_status mode_valid_path(struct drm_connector > > > *connector, > > > + struct drm_encoder *encoder, > > > + struct drm_crtc *crtc, > > > + struct drm_display_mode *mode) > > > +{ > > > + enum drm_mode_status ret; > > > + > > > + ret = drm_encoder_mode_valid(encoder, mode); > > > + if (ret != MODE_OK) { > > > + DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() failed\n", > > > + encoder->base.id, encoder->name); > > > + return ret; > > > + } > > > + > > > + ret = drm_bridge_mode_valid(encoder->bridge, mode); > > > + if (ret != MODE_OK) { > > > + DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n"); > > > + return ret; > > > + } > > > + > > > + ret = drm_crtc_mode_valid(crtc, mode); > > > + if (ret != MODE_OK) { > > > + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n", > > > + crtc->base.id, crtc->name); > > > + return ret; > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +static int > > > +mode_valid(struct drm_atomic_state *state) > > > +{ > > > + struct drm_connector_state *conn_state; > > > + struct drm_connector *connector; > > > + int i; > > > + > > > + for_each_new_connector_in_state(state, connector, conn_state, i) { > > > + struct drm_encoder *encoder = conn_state->best_encoder; > > > + struct drm_crtc *crtc = conn_state->crtc; > > > + struct drm_crtc_state *crtc_state; > > > + enum drm_mode_status mode_status; > > > + struct drm_display_mode *mode; > > > + > > > + if (!crtc || !encoder) > > > + continue; > > > + > > > + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > > + if (!crtc_state) > > > + continue; > > > + if (!crtc_state->mode_changed && !crtc_state > > > ->connectors_changed) > > > + continue; > > > + > > > + mode = _state->mode; > > > + > > > + mode_status = mode_valid_path(connector, encoder, crtc, mode); > > > + if (mode_status != MODE_OK) > > > + return -EINVAL; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > /** > > >* drm_atomic_helper_check_modeset - validate state object for modeset > > >changes * @dev: DRM device > > > @@ -466,13 +529,15 @@ static int handle_conflicting_encoders(struct > > > drm_atomic_state *state, > > > * 2. _connector_helper_funcs.atomic_check to validate the > > > connector state. > > > * 3. If it's determined a modeset is needed then all connectors on the > > > affected crtc > > > *crtc are added and _connector_helper_funcs.atomic_check is > > > run on them. > > > - * 4. _bridge_funcs.mode_fixup is called on all encoder bridges. > > > - * 5. _encoder_helper_funcs.atomic_check is called to validate any > > > encoder state. > > > + * 4. _encoder_helper_funcs.mode_valid, > > > _bridge_funcs.mode_valid and > > > + *_crtc_helper_funcs.mode_valid are called on the affected > > > components. > > > + * 5. _bridge_funcs.mode_fixup is called on all encoder bridges. > > > + * 6.
Re: [PATCH v2 7/8] drm: Use mode_valid() in atomic modeset
On Wed, May 10, 2017 at 09:38:00PM +0530, Archit Taneja wrote: > > > On 5/9/2017 10:30 PM, Jose Abreu wrote: > > This patches makes use of the new mode_valid() callbacks introduced > > previously to validate the full video pipeline when modesetting. > > > > This calls the connector->mode_valid(), encoder->mode_valid(), > > bridge->mode_valid() and crtc->mode_valid() so that we can > > make sure that the mode will be accepted in every components. > > > > Signed-off-by: Jose Abreu> > Cc: Carlos Palminha > > Cc: Alexey Brodkin > > Cc: Ville Syrjälä > > Cc: Daniel Vetter > > Cc: Dave Airlie > > Cc: Andrzej Hajda > > Cc: Archit Taneja > > --- > > > > Changes v1->v2: > > - Removed call to connector->mode_valid (Ville, Daniel) > > - Change function name (Ville) > > - Use for_each_new_connector_in_state (Ville) > > - Do not validate if connector and mode didn't change (Ville) > > - Use new helpers to call mode_valid > > > > drivers/gpu/drm/drm_atomic_helper.c | 75 > > +++-- > > 1 file changed, 72 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > b/drivers/gpu/drm/drm_atomic_helper.c > > index 8be9719..19d0ea1 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -452,6 +452,69 @@ static int handle_conflicting_encoders(struct > > drm_atomic_state *state, > > return 0; > > } > > +static enum drm_mode_status mode_valid_path(struct drm_connector > > *connector, > > + struct drm_encoder *encoder, > > + struct drm_crtc *crtc, > > + struct drm_display_mode *mode) > > +{ > > + enum drm_mode_status ret; > > + > > + ret = drm_encoder_mode_valid(encoder, mode); > > + if (ret != MODE_OK) { > > + DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() failed\n", > > + encoder->base.id, encoder->name); > > + return ret; > > + } > > + > > + ret = drm_bridge_mode_valid(encoder->bridge, mode); > > + if (ret != MODE_OK) { > > + DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n"); > > + return ret; > > + } > > + > > + ret = drm_crtc_mode_valid(crtc, mode); > > + if (ret != MODE_OK) { > > + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n", > > + crtc->base.id, crtc->name); > > + return ret; > > + } > > + > > + return ret; > > +} > > + > > +static int > > +mode_valid(struct drm_atomic_state *state) > > +{ > > + struct drm_connector_state *conn_state; > > + struct drm_connector *connector; > > + int i; > > + > > + for_each_new_connector_in_state(state, connector, conn_state, i) { > > + struct drm_encoder *encoder = conn_state->best_encoder; > > + struct drm_crtc *crtc = conn_state->crtc; > > + struct drm_crtc_state *crtc_state; > > + enum drm_mode_status mode_status; > > + struct drm_display_mode *mode; > > + > > + if (!crtc || !encoder) > > + continue; > > + > > + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > + if (!crtc_state) > > + continue; > > + if (!crtc_state->mode_changed && > > !crtc_state->connectors_changed) > > + continue; > > + > > + mode = _state->mode; > > + > > + mode_status = mode_valid_path(connector, encoder, crtc, mode); > > + if (mode_status != MODE_OK) > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > /** > >* drm_atomic_helper_check_modeset - validate state object for modeset > > changes > >* @dev: DRM device > > @@ -466,13 +529,15 @@ static int handle_conflicting_encoders(struct > > drm_atomic_state *state, > >* 2. _connector_helper_funcs.atomic_check to validate the connector > > state. > >* 3. If it's determined a modeset is needed then all connectors on the > > affected crtc > >*crtc are added and _connector_helper_funcs.atomic_check is run > > on them. > > - * 4. _bridge_funcs.mode_fixup is called on all encoder bridges. > > - * 5. _encoder_helper_funcs.atomic_check is called to validate any > > encoder state. > > + * 4. _encoder_helper_funcs.mode_valid, _bridge_funcs.mode_valid > > and > > + *_crtc_helper_funcs.mode_valid are called on the affected > > components. > > + * 5. _bridge_funcs.mode_fixup is called on all encoder bridges. > > + * 6. _encoder_helper_funcs.atomic_check is called to validate any > > encoder state. > >*This function is only called when the encoder will be part of a > > configured crtc, > >*it must not
Re: [PATCH v2 7/8] drm: Use mode_valid() in atomic modeset
On 5/9/2017 10:30 PM, Jose Abreu wrote: This patches makes use of the new mode_valid() callbacks introduced previously to validate the full video pipeline when modesetting. This calls the connector->mode_valid(), encoder->mode_valid(), bridge->mode_valid() and crtc->mode_valid() so that we can make sure that the mode will be accepted in every components. Signed-off-by: Jose AbreuCc: Carlos Palminha Cc: Alexey Brodkin Cc: Ville Syrjälä Cc: Daniel Vetter Cc: Dave Airlie Cc: Andrzej Hajda Cc: Archit Taneja --- Changes v1->v2: - Removed call to connector->mode_valid (Ville, Daniel) - Change function name (Ville) - Use for_each_new_connector_in_state (Ville) - Do not validate if connector and mode didn't change (Ville) - Use new helpers to call mode_valid drivers/gpu/drm/drm_atomic_helper.c | 75 +++-- 1 file changed, 72 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8be9719..19d0ea1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -452,6 +452,69 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state, return 0; } +static enum drm_mode_status mode_valid_path(struct drm_connector *connector, + struct drm_encoder *encoder, + struct drm_crtc *crtc, + struct drm_display_mode *mode) +{ + enum drm_mode_status ret; + + ret = drm_encoder_mode_valid(encoder, mode); + if (ret != MODE_OK) { + DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() failed\n", + encoder->base.id, encoder->name); + return ret; + } + + ret = drm_bridge_mode_valid(encoder->bridge, mode); + if (ret != MODE_OK) { + DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n"); + return ret; + } + + ret = drm_crtc_mode_valid(crtc, mode); + if (ret != MODE_OK) { + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n", + crtc->base.id, crtc->name); + return ret; + } + + return ret; +} + +static int +mode_valid(struct drm_atomic_state *state) +{ + struct drm_connector_state *conn_state; + struct drm_connector *connector; + int i; + + for_each_new_connector_in_state(state, connector, conn_state, i) { + struct drm_encoder *encoder = conn_state->best_encoder; + struct drm_crtc *crtc = conn_state->crtc; + struct drm_crtc_state *crtc_state; + enum drm_mode_status mode_status; + struct drm_display_mode *mode; + + if (!crtc || !encoder) + continue; + + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + if (!crtc_state) + continue; + if (!crtc_state->mode_changed && !crtc_state->connectors_changed) + continue; + + mode = _state->mode; + + mode_status = mode_valid_path(connector, encoder, crtc, mode); + if (mode_status != MODE_OK) + return -EINVAL; + } + + return 0; +} + /** * drm_atomic_helper_check_modeset - validate state object for modeset changes * @dev: DRM device @@ -466,13 +529,15 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state, * 2. _connector_helper_funcs.atomic_check to validate the connector state. * 3. If it's determined a modeset is needed then all connectors on the affected crtc *crtc are added and _connector_helper_funcs.atomic_check is run on them. - * 4. _bridge_funcs.mode_fixup is called on all encoder bridges. - * 5. _encoder_helper_funcs.atomic_check is called to validate any encoder state. + * 4. _encoder_helper_funcs.mode_valid, _bridge_funcs.mode_valid and + *_crtc_helper_funcs.mode_valid are called on the affected components. + * 5. _bridge_funcs.mode_fixup is called on all encoder bridges. + * 6. _encoder_helper_funcs.atomic_check is called to validate any encoder state. *This function is only called when the encoder will be part of a configured crtc, *it must not be used for implementing connector property validation. *If this function is NULL, _atomic_encoder_helper_funcs.mode_fixup is called *instead. - * 6. _crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints. + * 7. _crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints. * *
[PATCH v2 7/8] drm: Use mode_valid() in atomic modeset
This patches makes use of the new mode_valid() callbacks introduced previously to validate the full video pipeline when modesetting. This calls the connector->mode_valid(), encoder->mode_valid(), bridge->mode_valid() and crtc->mode_valid() so that we can make sure that the mode will be accepted in every components. Signed-off-by: Jose AbreuCc: Carlos Palminha Cc: Alexey Brodkin Cc: Ville Syrjälä Cc: Daniel Vetter Cc: Dave Airlie Cc: Andrzej Hajda Cc: Archit Taneja --- Changes v1->v2: - Removed call to connector->mode_valid (Ville, Daniel) - Change function name (Ville) - Use for_each_new_connector_in_state (Ville) - Do not validate if connector and mode didn't change (Ville) - Use new helpers to call mode_valid drivers/gpu/drm/drm_atomic_helper.c | 75 +++-- 1 file changed, 72 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8be9719..19d0ea1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -452,6 +452,69 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state, return 0; } +static enum drm_mode_status mode_valid_path(struct drm_connector *connector, + struct drm_encoder *encoder, + struct drm_crtc *crtc, + struct drm_display_mode *mode) +{ + enum drm_mode_status ret; + + ret = drm_encoder_mode_valid(encoder, mode); + if (ret != MODE_OK) { + DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() failed\n", + encoder->base.id, encoder->name); + return ret; + } + + ret = drm_bridge_mode_valid(encoder->bridge, mode); + if (ret != MODE_OK) { + DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n"); + return ret; + } + + ret = drm_crtc_mode_valid(crtc, mode); + if (ret != MODE_OK) { + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n", + crtc->base.id, crtc->name); + return ret; + } + + return ret; +} + +static int +mode_valid(struct drm_atomic_state *state) +{ + struct drm_connector_state *conn_state; + struct drm_connector *connector; + int i; + + for_each_new_connector_in_state(state, connector, conn_state, i) { + struct drm_encoder *encoder = conn_state->best_encoder; + struct drm_crtc *crtc = conn_state->crtc; + struct drm_crtc_state *crtc_state; + enum drm_mode_status mode_status; + struct drm_display_mode *mode; + + if (!crtc || !encoder) + continue; + + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + if (!crtc_state) + continue; + if (!crtc_state->mode_changed && !crtc_state->connectors_changed) + continue; + + mode = _state->mode; + + mode_status = mode_valid_path(connector, encoder, crtc, mode); + if (mode_status != MODE_OK) + return -EINVAL; + } + + return 0; +} + /** * drm_atomic_helper_check_modeset - validate state object for modeset changes * @dev: DRM device @@ -466,13 +529,15 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state, * 2. _connector_helper_funcs.atomic_check to validate the connector state. * 3. If it's determined a modeset is needed then all connectors on the affected crtc *crtc are added and _connector_helper_funcs.atomic_check is run on them. - * 4. _bridge_funcs.mode_fixup is called on all encoder bridges. - * 5. _encoder_helper_funcs.atomic_check is called to validate any encoder state. + * 4. _encoder_helper_funcs.mode_valid, _bridge_funcs.mode_valid and + *_crtc_helper_funcs.mode_valid are called on the affected components. + * 5. _bridge_funcs.mode_fixup is called on all encoder bridges. + * 6. _encoder_helper_funcs.atomic_check is called to validate any encoder state. *This function is only called when the encoder will be part of a configured crtc, *it must not be used for implementing connector property validation. *If this function is NULL, _atomic_encoder_helper_funcs.mode_fixup is called *instead. - * 6. _crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints. + * 7. _crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints. * * _crtc_state.mode_changed is set when the input mode is changed. *