On Wed, 2019-08-21 at 20:02 +0000, Francis, David wrote:
> I know this looks like it could be a DRM helper, but
> - That would require a DRM-generic way of knowing if a
> connector supports DSC, and current precedent is that
> DSC functionality is stored on a driver-specific basis
Don't mistake a lack of foresight for precedence. As well we already have a
bunch of dsc helpers in drm_dp_dsc.c, so it would seem reasonable to add more.
> - This function, by necessity, locks global state. Other
> hardware may be able to solve this problem with more fine-
> grained locking, for example by recalculating DSC configs
> and then adding to the state all CRTCs for which the config
> has changed
This function doesn't nessecarily need to be copy-pasted into DRM as is,
rather I'd like to see us come up with an actual design that can be used with
multiple drivers like we said. If AMD were moved to use the newer atomic
helpers, drm_dp_mst_atomic_check(). Note that when adding this I didn't move
over amd because at the time I looked at amdgpu's MST code for DM it didn't
look like any of it was really prepared for basic atomic state checking at
all, I don't know if this has changed or if maybe I missed something in the
confusing DM/DC layer.
A lot of the information you're looking at in this function could be done
entirely from just looking at the atomic topology state and adding properties
to it as needed, along with using that topology state to infer precisely which
connectors (and thus, which CRTCs) need locking. All that needs to be done is
adding whatever information you need to that atomic state.
Note as well I've already changed a lot of the MST helpers already, so if we
end up having to modify these helpers down the line so that other drivers can
use them it's really not a problem.
> - AMD is currently the only user of MST DSC, so if this needs
> to be in DRM, it would be easier to do so once it is
> clear what functionality is common between drivers
Currently yes, but Intel is planning on adding support for this as well very
soon (I just discussed this with so collaborating with them and figuring out a
design that works for everyone shouldn't be too difficult. Talking with Dave
Airlie he pointed out, and tbh I agree with him, that we really should land
this as helpers first as there's less motivation to do so after it's been
added to a driver. As well, we also can move things out of intel's driver and
into common helpers as need be.
>
> ________________________________________
> From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> on behalf of David
> Francis <David.Francis@xxxxxxx>
> Sent: August 21, 2019 4:01 PM
> To: dri-devel@xxxxxxxxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Li, Sun peng (Leo); Francis, David; Kazlauskas, Nicholas
> Subject: [PATCH v3 16/16] drm/amd/display: Trigger modesets on MST DSC
> connectors
>
> Whenever a connector on an MST network is attached, detached, 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.
>
> Therefore, whenever a crtc has drm_atomic_crtc_needs_modeset,
> for each crtc that shares a MST topology with that stream and
> supports DSC, add that crtc (and all affected connectors and
> planes) to the atomic state and set mode_changed on its state
>
> v2: Do this check only on Navi and before adding connectors
> and planes on modesetting crtcs
>
> Cc: Leo Li <sunpeng.li@xxxxxxx>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
> Cc: Lyude Paul <lyude@xxxxxxxxxx>
> Signed-off-by: David Francis <David.Francis@xxxxxxx>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 77 +++++++++++++++++++
> 1 file changed, 77 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 145fd73025dc..702fb0e29053 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6475,7 +6475,73 @@ static int do_aquire_global_lock(struct drm_device
> *dev,
>
> return ret < 0 ? ret : 0;
> }
> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
> +/*
> + * TODO: This logic should at some point be moved into DRM
> + */
> +static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state,
> struct drm_crtc *crtc)
> +{
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> + struct drm_connector_list_iter conn_iter;
> + struct drm_crtc_state *new_crtc_state;
> + struct amdgpu_dm_connector *aconnector = NULL, *aconnector_to_add;
> + int i, j;
> + struct drm_crtc *crtcs_affected[AMDGPU_MAX_CRTCS] = { 0 };
> +
> + for_each_new_connector_in_state(state, connector, conn_state, i) {
> + if (conn_state->crtc != crtc)
> + continue;
> +
> + aconnector = to_amdgpu_dm_connector(connector);
> + if (!aconnector->port)
> + aconnector = NULL;
> + else
> + break;
> + }
> +
> + if (!aconnector)
> + return 0;
> +
> + i = 0;
> + drm_connector_list_iter_begin(state->dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> + if (!connector->state || !connector->state->crtc)
> + continue;
> +
> + aconnector_to_add = to_amdgpu_dm_connector(connector);
> + if (!aconnector_to_add->port)
> + continue;
> +
> + if (aconnector_to_add->port->mgr != aconnector->port->mgr)
> + continue;
> +
> + if (!aconnector_to_add->dc_sink)
> + continue;
> +
> + if (!aconnector_to_add->dc_sink-
> >sink_dsc_caps.dsc_dec_caps.is_dsc_supported)
> + continue;
> +
> + if (i >= AMDGPU_MAX_CRTCS)
> + continue;
> +
> + crtcs_affected[i] = connector->state->crtc;
> + i++;
> + }
> + drm_connector_list_iter_end(&conn_iter);
> +
> + for (j = 0; j < i; j++) {
> + new_crtc_state = drm_atomic_get_crtc_state(state,
> crtcs_affected[j]);
> + if (IS_ERR(new_crtc_state))
> + return PTR_ERR(new_crtc_state);
>
> + new_crtc_state->mode_changed = true;
> + }
> +
> + return 0;
> +
> +}
> +#endif
> static void get_freesync_config_for_crtc(
> struct dm_crtc_state *new_crtc_state,
> struct dm_connector_state *new_con_state)
> @@ -7160,6 +7226,17 @@ static int amdgpu_dm_atomic_check(struct drm_device
> *dev,
> if (ret)
> goto fail;
>
> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
> + if (adev->asic_type >= CHIP_NAVI10) {
> + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> + if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
> + ret = add_affected_mst_dsc_crtcs(state,
> crtc);
> + if (ret)
> + goto fail;
> + }
> + }
> + }
> +#endif
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> if (!drm_atomic_crtc_needs_modeset(new_crtc_state) &&
> !new_crtc_state->color_mgmt_changed &&
> --
> 2.17.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Cheers,
Lyude Paul
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
