| Message ID | 20191030192431.5798-13-mikita.lipski@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DSC MST support for AMDGPU | expand |
Hey! Great start so far, some comments down below: On Wed, 2019-10-30 at 15:24 -0400, mikita.lipski@amd.com wrote: > From: Mikita Lipski <mikita.lipski@amd.com> > > Adding the following elements to add MST DSC support to DRM: > > - dsc_enable boolean flag to drm_dp_vcpi_allocation structure to signal, > which port got DSC enabled > > - function drm_dp_helper_update_vcpi_slots_for_dsc allows reallocation > of newly recalculated VCPI slots and raises dsc_enable flag on the port. > > - function drm_dp_mst_update_dsc_crtcs is called in drm_dp_mst_atomic_check, > its purpose is to iterate through all the ports in the topology and set > mode_changed flag on crtc if DSC has been enabled. > > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Lyude Paul <lyude@redhat.com> > Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 103 +++++++++++++++++++++++++- > include/drm/drm_dp_mst_helper.h | 4 + > 2 files changed, 106 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index d5df02315e14..4f2f09fe32f8 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -73,6 +73,7 @@ static bool drm_dp_validate_guid(struct > drm_dp_mst_topology_mgr *mgr, > static int drm_dp_mst_register_i2c_bus(struct drm_dp_aux *aux); > static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_aux *aux); > static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr); > +static void drm_dp_mst_update_dsc_crtcs(struct drm_dp_mst_topology_state > *mst_state); > > #define DP_STR(x) [DP_ ## x] = #x > > @@ -3293,6 +3294,65 @@ int drm_dp_atomic_find_vcpi_slots(struct > drm_atomic_state *state, > } > EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); > > +/** > + * drm_dp_helper_update_vcpi_slots_for_dsc() - Update VCPI slots with new > on the state > + * > + * @state: global atomic state > + * @port: port to find vcpi slots > + * @pbn: updated bandwidth required for the mode in PBN > + * > + * Function reallocates VCPI slots to the @port by calling > + * drm_dp_atomic_find_vcpi_slots. The assumption is that VCPI slots > + * have already been allocated and this is second call overwritting > + * initial values. After the VCPI is allocated dsc_enable flag is set to > + * true for atomic check. > + * > + * It is driver's responsibility to call this function after it decides > + * to enable DSC. > + * > + * See also: > + * drm_dp_mst_update_dsc_crtcs() > + * > + * Returns: > + * Total slots in the atomic state assigned for this port, or a negative > error > + * code if the port no longer exists or vcpi slots haven't been assigned. > + */ > +int drm_dp_helper_update_vcpi_slots_for_dsc(struct drm_atomic_state *state, > + struct drm_dp_mst_port *port, > + int pbn) > +{ > + struct drm_dp_mst_topology_state *topology_state; > + struct drm_dp_vcpi_allocation *pos; > + bool found = false; > + int vcpi = 0; > + > + topology_state = drm_atomic_get_mst_topology_state(state, port->mgr); > + > + if (IS_ERR(topology_state)) > + return PTR_ERR(topology_state); > + > + list_for_each_entry(pos, &topology_state->vcpis, next) { > + if (pos->port == port) { > + found = true; > + break; > + } > + } > + > + if (!found || !pos->vcpi) > + return -EINVAL; > + > + vcpi = drm_dp_atomic_find_vcpi_slots(state, port->mgr, > + port, pbn); > + > + if (vcpi < 0) > + return -EINVAL; > + > + pos->dsc_enable = true; > + > + return vcpi; > +} > + This helper I think we can simplify a bit by dropping it and merging it with drm_dp_mst_update_dsc_crtcs(). I've got a more in-depth explanation of what I mean down below: > +EXPORT_SYMBOL(drm_dp_helper_update_vcpi_slots_for_dsc); > /** > * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots > * @state: global atomic state > @@ -3871,6 +3931,46 @@ drm_dp_mst_atomic_check_topology_state(struct > drm_dp_mst_topology_mgr *mgr, > return 0; > } > > +/** > + * drm_dp_mst_update_dsc_crtcs - Set mode change flag on CRTCs which > + * just got DSC enabled > + * @state: Pointer to the new &struct drm_dp_mst_topology_state > + * > + * Itearate through all the ports in MST topology to check if DSC > + * has been enabled on any of them. Set mode_changed to true on > + * crtc state that just got DSC enabled. > + * > + * See also: > + * drm_dp_helper_update_vcpi_slots_for_dsc() > + */ > +static void > +drm_dp_mst_update_dsc_crtcs(struct drm_dp_mst_topology_state *mst_state) > +{ Just grab the atomic state from within this function, not really much point in making the caller pull in the mst topoloy state since we're the only ones using it > + struct drm_dp_vcpi_allocation *pos; > + struct drm_dp_mst_port *port; > + struct drm_connector_state *conn_state; > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; > + > + list_for_each_entry(pos, &mst_state->vcpis, next) { > + > + port = pos->port; > + conn_state = drm_atomic_get_connector_state(mst_state- > >base.state, > + port->connector); > + crtc = conn_state->crtc; > + if (!crtc) > + continue; > + > + crtc_state = drm_atomic_get_crtc_state(mst_state->base.state, > crtc); You're forgetting to check the return status of drm_atomic_get_crtc_state(), since it can fail here. You need something like: if (IS_ERR(crtc_state)) return PTR_ERR(crtc_state); > + if (port->vcpi.vcpi == pos->vcpi) > + continue; > + > + if (pos->dsc_enable) { > + crtc_state->mode_changed = true; > + pos->dsc_enable = false; > + } > + } > +} So: I think we can simply this a bit. Why not rename drm_dp_mst_update_dsc_crtcs() to drm_dp_mst_atomic_enable_dsc(), and rework it to look like this psuedocode: int drm_dp_mst_atomic_set_dsc(struct drm_atomic_state *state, struct drm_dp_mst_port *port, bool enabled) { struct drm_dp_mst_topology_state *mst_state = /* ... */; /* ... */ if (port->dsc_enable == enabled) return 0; port->dsc_enable = enabled; for_each_mst_port() { if (port->dsc_enable == enabled) continue; port->dsc_enable = enabled; conn_state = drm_atomic_get_connector_state(port->connector); /* error handling ... */ if (!conn_state->crtc) continue; crtc_state = drm_atomic_get_crtc_state(conn_state->crtc); /* error handling... */ crtc_state->mode_changed = true; } return 0; } IMO this works a bit better since all we really need is something to pull in crtcs affected by the state change. Also, more DRM_DEBUG_ATOMIC() statements similar to what I've got in the vcpi helpers would be nice :) Otherwise, looks great so far! > /** > * drm_dp_mst_atomic_check - Check that the new state of an MST topology in > an > * atomic update is valid > @@ -3887,9 +3987,9 @@ drm_dp_mst_atomic_check_topology_state(struct > drm_dp_mst_topology_mgr *mgr, > * See also: > * drm_dp_atomic_find_vcpi_slots() > * drm_dp_atomic_release_vcpi_slots() > - * > * Returns: > * > + * > * 0 if the new state is valid, negative error code otherwise. > */ > int drm_dp_mst_atomic_check(struct drm_atomic_state *state) > @@ -3902,6 +4002,7 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state > *state) > ret = drm_dp_mst_atomic_check_topology_state(mgr, mst_state); > if (ret) > break; > + drm_dp_mst_update_dsc_crtcs(mst_state); > } > > return ret; > diff --git a/include/drm/drm_dp_mst_helper.h > b/include/drm/drm_dp_mst_helper.h > index 4cf738545dfb..185e29895f5f 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -431,6 +431,7 @@ struct drm_dp_payload { > struct drm_dp_vcpi_allocation { > struct drm_dp_mst_port *port; > int vcpi; > + bool dsc_enable; > struct list_head next; > }; > > @@ -662,6 +663,9 @@ int __must_check > drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, > struct drm_dp_mst_topology_mgr *mgr, > struct drm_dp_mst_port *port, int pbn); > +int drm_dp_helper_update_vcpi_slots_for_dsc(struct drm_atomic_state *state, > + struct drm_dp_mst_port *port, > + int pbn); > int __must_check > drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, > struct drm_dp_mst_topology_mgr *mgr,
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index d5df02315e14..4f2f09fe32f8 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -73,6 +73,7 @@ static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, static int drm_dp_mst_register_i2c_bus(struct drm_dp_aux *aux); static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_aux *aux); static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr); +static void drm_dp_mst_update_dsc_crtcs(struct drm_dp_mst_topology_state *mst_state); #define DP_STR(x) [DP_ ## x] = #x @@ -3293,6 +3294,65 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, } EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); +/** + * drm_dp_helper_update_vcpi_slots_for_dsc() - Update VCPI slots with new on the state + * + * @state: global atomic state + * @port: port to find vcpi slots + * @pbn: updated bandwidth required for the mode in PBN + * + * Function reallocates VCPI slots to the @port by calling + * drm_dp_atomic_find_vcpi_slots. The assumption is that VCPI slots + * have already been allocated and this is second call overwritting + * initial values. After the VCPI is allocated dsc_enable flag is set to + * true for atomic check. + * + * It is driver's responsibility to call this function after it decides + * to enable DSC. + * + * See also: + * drm_dp_mst_update_dsc_crtcs() + * + * Returns: + * Total slots in the atomic state assigned for this port, or a negative error + * code if the port no longer exists or vcpi slots haven't been assigned. + */ +int drm_dp_helper_update_vcpi_slots_for_dsc(struct drm_atomic_state *state, + struct drm_dp_mst_port *port, + int pbn) +{ + struct drm_dp_mst_topology_state *topology_state; + struct drm_dp_vcpi_allocation *pos; + bool found = false; + int vcpi = 0; + + topology_state = drm_atomic_get_mst_topology_state(state, port->mgr); + + if (IS_ERR(topology_state)) + return PTR_ERR(topology_state); + + list_for_each_entry(pos, &topology_state->vcpis, next) { + if (pos->port == port) { + found = true; + break; + } + } + + if (!found || !pos->vcpi) + return -EINVAL; + + vcpi = drm_dp_atomic_find_vcpi_slots(state, port->mgr, + port, pbn); + + if (vcpi < 0) + return -EINVAL; + + pos->dsc_enable = true; + + return vcpi; +} + +EXPORT_SYMBOL(drm_dp_helper_update_vcpi_slots_for_dsc); /** * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots * @state: global atomic state @@ -3871,6 +3931,46 @@ drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr *mgr, return 0; } +/** + * drm_dp_mst_update_dsc_crtcs - Set mode change flag on CRTCs which + * just got DSC enabled + * @state: Pointer to the new &struct drm_dp_mst_topology_state + * + * Itearate through all the ports in MST topology to check if DSC + * has been enabled on any of them. Set mode_changed to true on + * crtc state that just got DSC enabled. + * + * See also: + * drm_dp_helper_update_vcpi_slots_for_dsc() + */ +static void +drm_dp_mst_update_dsc_crtcs(struct drm_dp_mst_topology_state *mst_state) +{ + struct drm_dp_vcpi_allocation *pos; + struct drm_dp_mst_port *port; + struct drm_connector_state *conn_state; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + + list_for_each_entry(pos, &mst_state->vcpis, next) { + + port = pos->port; + conn_state = drm_atomic_get_connector_state(mst_state->base.state, + port->connector); + crtc = conn_state->crtc; + if (!crtc) + continue; + + crtc_state = drm_atomic_get_crtc_state(mst_state->base.state, crtc); + if (port->vcpi.vcpi == pos->vcpi) + continue; + + if (pos->dsc_enable) { + crtc_state->mode_changed = true; + pos->dsc_enable = false; + } + } +} /** * drm_dp_mst_atomic_check - Check that the new state of an MST topology in an * atomic update is valid @@ -3887,9 +3987,9 @@ drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr *mgr, * See also: * drm_dp_atomic_find_vcpi_slots() * drm_dp_atomic_release_vcpi_slots() - * * Returns: * + * * 0 if the new state is valid, negative error code otherwise. */ int drm_dp_mst_atomic_check(struct drm_atomic_state *state) @@ -3902,6 +4002,7 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state) ret = drm_dp_mst_atomic_check_topology_state(mgr, mst_state); if (ret) break; + drm_dp_mst_update_dsc_crtcs(mst_state); } return ret; diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 4cf738545dfb..185e29895f5f 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -431,6 +431,7 @@ struct drm_dp_payload { struct drm_dp_vcpi_allocation { struct drm_dp_mst_port *port; int vcpi; + bool dsc_enable; struct list_head next; }; @@ -662,6 +663,9 @@ int __must_check drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, int pbn); +int drm_dp_helper_update_vcpi_slots_for_dsc(struct drm_atomic_state *state, + struct drm_dp_mst_port *port, + int pbn); int __must_check drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, struct drm_dp_mst_topology_mgr *mgr,