Re: [PATCH v8 16/17] drm/dp_mst: Add helper to trigger modeset on affected DSC MST CRTCs
On Tue, 2019-12-03 at 09:35 -0500, mikita.lip...@amd.com wrote:
> From: Mikita Lipski
>
> [why]
> Whenever a connector on an MST network is changed or
> undergoes a modeset, the DSC configs for each stream on that
> topology will be recalculated. This can change their required
> bandwidth, requiring a full reprogramming, as though a modeset
> was performed, even if that stream did not change timing.
>
> [how]
> Adding helper to trigger modesets on MST DSC connectors
> by setting mode_changed flag on CRTCs in the same topology
> as affected connector
>
> v2: use drm_dp_mst_dsc_aux_for_port function to verify
> if the port is DSC capable
>
> Cc: Manasi Navare
> Cc: Lyude Paul
> Signed-off-by: Mikita Lipski
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 62 +++
> include/drm/drm_dp_mst_helper.h | 2 +
> 2 files changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 76bcbb4cd8b4..fb3710b727cc 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4802,6 +4802,68 @@ drm_dp_mst_atomic_check_topology_state(struct
> drm_dp_mst_topology_mgr *mgr,
> return 0;
> }
>
> +/**
> + * drm_dp_mst_add_affected_dsc_crtcs
> + * @state: Pointer to the new drm_dp_mst_topology_state
> + * @port: Pointer tothe port of connector with new state
> + *
> + * Whenever there is a change in mst topology
> + * DSC configuration would have to be recalculated
> + * therefore we need to trigger modeset on all affected
> + * CRTCs in that topology
> + *
> + * See also:
> + * drm_dp_mst_atomic_enable_dsc()
> + */
> +int drm_dp_mst_add_affected_dsc_crtcs(struct drm_atomic_state *state,
> struct drm_dp_mst_topology_mgr *mgr)
> +{
> + struct drm_dp_mst_topology_state *mst_state;
> + struct drm_dp_vcpi_allocation *pos;
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> +
> + if (!mgr) {
> + DRM_DEBUG_ATOMIC("[MST MGR:%p] Passed Topology Manager pointer
> is pointing to NULL\n", mgr);
> + return -EINVAL;
> + }
I'd get rid of this check, we'll notice pretty easily if it's NULL :P
> +
> + mst_state = drm_atomic_get_mst_topology_state(state, mgr);
Forgot to check IS_ERR(mst_state) here
> +
> + list_for_each_entry(pos, _state->vcpis, next) {
> +
> + connector = pos->port->connector;
> +
> + if (!connector)
> + return -EINVAL;
> +
> + conn_state = drm_atomic_get_connector_state(state, connector);
> +
> + if (IS_ERR(conn_state))
> + return PTR_ERR(conn_state);
> +
> + crtc = conn_state->crtc;
> +
> + if (!crtc)
> + return -EINVAL;
This seems like something that would be an error from a driver using the API
incorrectly, maybe this should be something like
if (WARN_ON(!crtc))
return -EINVAL;
> +
> + if (!drm_dp_mst_dsc_aux_for_port(pos->port))
> + continue;
> +
> + crtc_state = drm_atomic_get_crtc_state(mst_state->base.state,
> crtc);
> +
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
> +
> + DRM_DEBUG_ATOMIC("[MST MGR:%p] Setting mode_changed flag on
> CRTC %p\n", mgr, crtc);
This can be wrapped a bit more to fit in 80 chars
> +
> + crtc_state->mode_changed = true;
Nitpick here: I usually try to group assignments and conditional checks on
those assignments unless it makes it more difficult to read, like:
ret = cool_function();
if (ret)
return ret;
But not too big of a deal either way. Won't hold back review
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_mst_add_affected_dsc_crtcs);
> +
> /**
> * drm_dp_mst_atomic_enable_dsc - Set DSC Enable Flag to On/Off
> * @state: Pointer to the new drm_atomic_state
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index 2919d9776af3..10e9c7049061 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -779,6 +779,8 @@ int drm_dp_mst_atomic_enable_dsc(struct drm_atomic_state
> *state,
>struct drm_dp_mst_port *port,
>int pbn, int pbn_div,
>bool enable);
> +int drm_dp_mst_add_affected_dsc_crtcs(struct drm_atomic_state *state,
> + struct drm_dp_mst_topology_mgr *mgr);
I'd add a __must_check attribute here. With the more important changes
addressed here:
Reviewed-by: Lyude Paul
> int __must_check
> drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
>struct drm_dp_mst_topology_mgr *mgr,
--
Cheers,
Lyude Paul
Re: [PATCH v8 16/17] drm/dp_mst: Add helper to trigger modeset on affected DSC MST CRTCs
Hi, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on next-20191203] [cannot apply to drm-intel/for-linux-next linus/master v5.4-rc8 v5.4] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/mikita-lipski-amd-com/DSC-MST-support-for-DRM-and-AMDGPU/20191204-020604 base:1ab75b2e415a29dba9aec94f203c6f88dbfc0ba0 reproduce: make htmldocs If you fix the issue, kindly add following tag Reported-by: kbuild test robot All warnings (new ones prefixed by >>): fs/posix_acl.c:647: warning: Function parameter or member 'mode_p' not described in 'posix_acl_update_mode' fs/posix_acl.c:647: warning: Function parameter or member 'acl' not described in 'posix_acl_update_mode' include/linux/regulator/machine.h:196: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints' include/linux/regulator/driver.h:223: warning: Function parameter or member 'resume' not described in 'regulator_ops' sound/soc/soc-core.c:2509: warning: Function parameter or member 'legacy_dai_naming' not described in 'snd_soc_register_dai' include/linux/skbuff.h:888: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'list' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'head_frag' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'encapsulation' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'csum_valid' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member '__pkt_vlan_present_offset' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'vlan_present' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'csum_level' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff' include/net/sock.h:232: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common' include/net/sock.h:232: warning: Function parameter or member 'skc_portpair' not described in 'sock_common' include/net/sock.h:232: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common' include/net/sock.h:232: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common' include/net/sock.h:232: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common' include/net/sock.h:232: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common' include/net/sock.h:232: warning: Function parameter or member 'skc_cookie' not described in 'sock_common' include/net/sock.h:232: warning: Function parameter or member 'skc_listener' not described in 'sock_common' include/net/sock.h:232: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common' include/net/sock.h:232: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common' include/net/sock.h:232: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common' include/net/sock.h:514: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock' include/net/sock.h:514: warning: Function parameter or member 'sk_wq_raw' not described in 'sock' include/net/sock.h:514: warning: Function
[PATCH v8 16/17] drm/dp_mst: Add helper to trigger modeset on affected DSC MST CRTCs
From: Mikita Lipski
[why]
Whenever a connector on an MST network is changed or
undergoes a modeset, the DSC configs for each stream on that
topology will be recalculated. This can change their required
bandwidth, requiring a full reprogramming, as though a modeset
was performed, even if that stream did not change timing.
[how]
Adding helper to trigger modesets on MST DSC connectors
by setting mode_changed flag on CRTCs in the same topology
as affected connector
v2: use drm_dp_mst_dsc_aux_for_port function to verify
if the port is DSC capable
Cc: Manasi Navare
Cc: Lyude Paul
Signed-off-by: Mikita Lipski
---
drivers/gpu/drm/drm_dp_mst_topology.c | 62 +++
include/drm/drm_dp_mst_helper.h | 2 +
2 files changed, 64 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 76bcbb4cd8b4..fb3710b727cc 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -4802,6 +4802,68 @@ drm_dp_mst_atomic_check_topology_state(struct
drm_dp_mst_topology_mgr *mgr,
return 0;
}
+/**
+ * drm_dp_mst_add_affected_dsc_crtcs
+ * @state: Pointer to the new drm_dp_mst_topology_state
+ * @port: Pointer tothe port of connector with new state
+ *
+ * Whenever there is a change in mst topology
+ * DSC configuration would have to be recalculated
+ * therefore we need to trigger modeset on all affected
+ * CRTCs in that topology
+ *
+ * See also:
+ * drm_dp_mst_atomic_enable_dsc()
+ */
+int drm_dp_mst_add_affected_dsc_crtcs(struct drm_atomic_state *state, struct
drm_dp_mst_topology_mgr *mgr)
+{
+ struct drm_dp_mst_topology_state *mst_state;
+ struct drm_dp_vcpi_allocation *pos;
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *crtc_state;
+
+ if (!mgr) {
+ DRM_DEBUG_ATOMIC("[MST MGR:%p] Passed Topology Manager pointer
is pointing to NULL\n", mgr);
+ return -EINVAL;
+ }
+
+ mst_state = drm_atomic_get_mst_topology_state(state, mgr);
+
+ list_for_each_entry(pos, _state->vcpis, next) {
+
+ connector = pos->port->connector;
+
+ if (!connector)
+ return -EINVAL;
+
+ conn_state = drm_atomic_get_connector_state(state, connector);
+
+ if (IS_ERR(conn_state))
+ return PTR_ERR(conn_state);
+
+ crtc = conn_state->crtc;
+
+ if (!crtc)
+ return -EINVAL;
+
+ if (!drm_dp_mst_dsc_aux_for_port(pos->port))
+ continue;
+
+ crtc_state = drm_atomic_get_crtc_state(mst_state->base.state,
crtc);
+
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
+
+ DRM_DEBUG_ATOMIC("[MST MGR:%p] Setting mode_changed flag on
CRTC %p\n", mgr, crtc);
+
+ crtc_state->mode_changed = true;
+ }
+ return 0;
+}
+EXPORT_SYMBOL(drm_dp_mst_add_affected_dsc_crtcs);
+
/**
* drm_dp_mst_atomic_enable_dsc - Set DSC Enable Flag to On/Off
* @state: Pointer to the new drm_atomic_state
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 2919d9776af3..10e9c7049061 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -779,6 +779,8 @@ int drm_dp_mst_atomic_enable_dsc(struct drm_atomic_state
*state,
struct drm_dp_mst_port *port,
int pbn, int pbn_div,
bool enable);
+int drm_dp_mst_add_affected_dsc_crtcs(struct drm_atomic_state *state,
+ struct drm_dp_mst_topology_mgr *mgr);
int __must_check
drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
struct drm_dp_mst_topology_mgr *mgr,
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
