On 10/26/18 7:37 AM, Pekka Paalanen wrote:

On Thu, 11 Oct 2018 12:39:41 -0400 Nicholas Kazlauskas nicholas.kazlauskas@amd.com wrote:

...

These include the drm_connector 'vrr_capable' and the drm_crtc 'vrr_enabled' properties.

Signed-off-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com

Documentation/gpu/drm-kms.rst | 7 +++++++ drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index 4b1501b4835b..8da2a178cf85 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -575,6 +575,13 @@ Explicit Fencing Properties .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c :doc: explicit fencing properties

+Variable Refresh Properties +---------------------------

+.. kernel-doc:: drivers/gpu/drm/drm_connector.c

• :doc: Variable refresh properties

• Existing KMS Properties

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index f0deeb7298d0..2a12853ca917 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev) } EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);

+/**

• • DOC: Variable refresh properties
• • Variable refresh rate control is supported via properties on the
• • &drm_connector and &drm_crtc objects.
• • "vrr_capable":
• • Optional &drm_connector boolean property that drivers should attach
• • with drm_connector_attach_vrr_capable_property() on connectors that
• • could support variable refresh rates. Drivers should update the
• • property value by calling drm_connector_set_vrr_capable_property().
• • Absence of the property should indicate absence of support.
• • "vrr_enabled":
• • Default &drm_crtc boolean property that notifies the driver that the
• • variable refresh rate adjustment should be enabled for the CRTC.

Hi,

where is the documentation that explains how drivers must implement "variable refresh rate adjustment"?

What should and could userspace expect to get if it flicks this switch?

I also think the kernel documentation should include a description of what VRR actually is and how it conceptually works as far as userspace is concerned.

That is, the kernel documentation should describe what this thing does, so that we avoid every driver implementing a different thing. For example, one driver could prevent the luminance flickering itself by tuning the timings while another driver might not do that, which means that an application tested on the former driver will look just fine while it is unbearable to watch on the latter.

Thanks, pq

I felt it was best to leave this more on the vague side to not impose restrictions yet on what a driver must do.

If you think it's worth defining what the "baseline" expectation is for userspace, I would probably describe it as "utilizing the monitor's variable refresh rate to reduce stuttering or tearing that can occur during flipping". This is currently what the amdgpu driver has enabled for support. The implementation also isn't much more complex than just passing the variable refresh range to the hardware.

I wouldn't want every driver to be forced to implement some sort of luminance flickering by default. It's not noticeable on many panels and any tuning would inherently add latency to the output. It would probably be better left as a new property or a driver specific feature.

In general I would imagine that most future VRR features would end up as new properties. Anything that's purely software could be implemented as a drm helper that every driver can use. I think the target presentation timestamp feature is a good example for that.

...

• • Support for variable refresh rate will depend on the "vrr_capable"
• • property exposed on the &drm_connector object.
• */
• /**
  • drm_connector_attach_vrr_capable_property - creates the
  • vrr_capable property

Nicholas Kazlauskas

On 10/26/18 10:53 AM, Ville Syrjälä wrote:

On Fri, Oct 26, 2018 at 02:49:31PM +0000, Kazlauskas, Nicholas wrote:

...

On 10/26/18 7:37 AM, Pekka Paalanen wrote:

...

On Thu, 11 Oct 2018 12:39:41 -0400 Nicholas Kazlauskas nicholas.kazlauskas@amd.com wrote:

...

These include the drm_connector 'vrr_capable' and the drm_crtc 'vrr_enabled' properties.

Signed-off-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com

Documentation/gpu/drm-kms.rst | 7 +++++++ drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index 4b1501b4835b..8da2a178cf85 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -575,6 +575,13 @@ Explicit Fencing Properties .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c :doc: explicit fencing properties

+Variable Refresh Properties +---------------------------

+.. kernel-doc:: drivers/gpu/drm/drm_connector.c

• :doc: Variable refresh properties

• Existing KMS Properties

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index f0deeb7298d0..2a12853ca917 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev) } EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);

+/**

• • DOC: Variable refresh properties
• • Variable refresh rate control is supported via properties on the
• • &drm_connector and &drm_crtc objects.
• • "vrr_capable":
• • Optional &drm_connector boolean property that drivers should attach
• • with drm_connector_attach_vrr_capable_property() on connectors that
• • could support variable refresh rates. Drivers should update the
• • property value by calling drm_connector_set_vrr_capable_property().
• • Absence of the property should indicate absence of support.
• • "vrr_enabled":
• • Default &drm_crtc boolean property that notifies the driver that the
• • variable refresh rate adjustment should be enabled for the CRTC.

Hi,

where is the documentation that explains how drivers must implement "variable refresh rate adjustment"?

What should and could userspace expect to get if it flicks this switch?

I also think the kernel documentation should include a description of what VRR actually is and how it conceptually works as far as userspace is concerned.

That is, the kernel documentation should describe what this thing does, so that we avoid every driver implementing a different thing. For example, one driver could prevent the luminance flickering itself by tuning the timings while another driver might not do that, which means that an application tested on the former driver will look just fine while it is unbearable to watch on the latter.

Thanks, pq

I felt it was best to leave this more on the vague side to not impose restrictions yet on what a driver must do.

If you think it's worth defining what the "baseline" expectation is for userspace, I would probably describe it as "utilizing the monitor's variable refresh rate to reduce stuttering or tearing that can occur during flipping". This is currently what the amdgpu driver has enabled for support. The implementation also isn't much more complex than just passing the variable refresh range to the hardware.

I wouldn't want every driver to be forced to implement some sort of luminance flickering by default. It's not noticeable on many panels and any tuning would inherently add latency to the output. It would probably be better left as a new property or a driver specific feature.

In general I would imagine that most future VRR features would end up as new properties. Anything that's purely software could be implemented as a drm helper that every driver can use. I think the target presentation timestamp feature is a good example for that.

Speaking of timestamps. What is the expected behaviour of vblank timestamps when vrr is enabled?

When vrr is enabled the duration of the vertical front porch will be extended until flip or timeout occurs. The vblank timestamp will vary based on duration of the vertical front porch. The min/max duration for the front porch can be specified by the driver via the min/max range.

No changes to vblank timestamping handling should be necessary to accommodate variable refresh rate.

I think it's probably best to update the documentation for vrr_enable with some of the specifics I described above. That should help clarify userspace expectations as well.

Nicholas Kazlauskas

On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:

On 10/26/18 10:53 AM, Ville Syrjälä wrote:

...

On Fri, Oct 26, 2018 at 02:49:31PM +0000, Kazlauskas, Nicholas wrote:

...

On 10/26/18 7:37 AM, Pekka Paalanen wrote:

...

On Thu, 11 Oct 2018 12:39:41 -0400 Nicholas Kazlauskas nicholas.kazlauskas@amd.com wrote:

...

These include the drm_connector 'vrr_capable' and the drm_crtc 'vrr_enabled' properties.

Signed-off-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com

Documentation/gpu/drm-kms.rst | 7 +++++++ drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index 4b1501b4835b..8da2a178cf85 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -575,6 +575,13 @@ Explicit Fencing Properties .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c :doc: explicit fencing properties

+Variable Refresh Properties +---------------------------

+.. kernel-doc:: drivers/gpu/drm/drm_connector.c

• :doc: Variable refresh properties

• Existing KMS Properties

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index f0deeb7298d0..2a12853ca917 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev) } EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);

+/**

• • DOC: Variable refresh properties
• • Variable refresh rate control is supported via properties on the
• • &drm_connector and &drm_crtc objects.
• • "vrr_capable":
• • Optional &drm_connector boolean property that drivers should attach
• • with drm_connector_attach_vrr_capable_property() on connectors that
• • could support variable refresh rates. Drivers should update the
• • property value by calling drm_connector_set_vrr_capable_property().
• • Absence of the property should indicate absence of support.
• • "vrr_enabled":
• • Default &drm_crtc boolean property that notifies the driver that the
• • variable refresh rate adjustment should be enabled for the CRTC.

Hi,

where is the documentation that explains how drivers must implement "variable refresh rate adjustment"?

What should and could userspace expect to get if it flicks this switch?

I also think the kernel documentation should include a description of what VRR actually is and how it conceptually works as far as userspace is concerned.

That is, the kernel documentation should describe what this thing does, so that we avoid every driver implementing a different thing. For example, one driver could prevent the luminance flickering itself by tuning the timings while another driver might not do that, which means that an application tested on the former driver will look just fine while it is unbearable to watch on the latter.

Thanks, pq

I felt it was best to leave this more on the vague side to not impose restrictions yet on what a driver must do.

If you think it's worth defining what the "baseline" expectation is for userspace, I would probably describe it as "utilizing the monitor's variable refresh rate to reduce stuttering or tearing that can occur during flipping". This is currently what the amdgpu driver has enabled

I would also mention that without VRR, the display engine processes the flips independently of the rendering speed which might result into stuttering or tearing.

Might also be worth giving a quick example with numbers in the documentation. Something like: For Eg if the rendering speed is 40Hz, without VRR, display engine will flip at constant 60Hz, which means that same frame will be displayed twice which will be observed as stutter/repetition. With VRR enabled, the vertical front porch will be extended and the flip will be processed when its ready or at max blanking time resulting a smooth transition without repetition.

...

...

for support. The implementation also isn't much more complex than just passing the variable refresh range to the hardware.

I wouldn't want every driver to be forced to implement some sort of luminance flickering by default. It's not noticeable on many panels and any tuning would inherently add latency to the output. It would probably be better left as a new property or a driver specific feature.

In general I would imagine that most future VRR features would end up as new properties. Anything that's purely software could be implemented as a drm helper that every driver can use. I think the target presentation timestamp feature is a good example for that.

Speaking of timestamps. What is the expected behaviour of vblank timestamps when vrr is enabled?

When vrr is enabled the duration of the vertical front porch will be extended until flip or timeout occurs. The vblank timestamp will vary based on duration of the vertical front porch. The min/max duration for the front porch can be specified by the driver via the min/max range.

This min max range that is read from monitor descriptor is only used to program the HW registers right? Its not exposed to the userspace, correct?

Manasi

No changes to vblank timestamping handling should be necessary to accommodate variable refresh rate.

I think it's probably best to update the documentation for vrr_enable with some of the specifics I described above. That should help clarify userspace expectations as well.

Nicholas Kazlauskas

On Fri, Oct 26, 2018 at 08:06:11PM +0000, Kazlauskas, Nicholas wrote:

On 10/26/18 3:13 PM, Manasi Navare wrote:

...

On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:

...

On 10/26/18 10:53 AM, Ville Syrjälä wrote:

...

On Fri, Oct 26, 2018 at 02:49:31PM +0000, Kazlauskas, Nicholas wrote:

...

On 10/26/18 7:37 AM, Pekka Paalanen wrote:

...

On Thu, 11 Oct 2018 12:39:41 -0400 Nicholas Kazlauskas nicholas.kazlauskas@amd.com wrote:

> These include the drm_connector 'vrr_capable' and the drm_crtc > 'vrr_enabled' properties. > > Signed-off-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com > --- > Documentation/gpu/drm-kms.rst | 7 +++++++ > drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++++++++++ > 2 files changed, 29 insertions(+) > > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst > index 4b1501b4835b..8da2a178cf85 100644 > --- a/Documentation/gpu/drm-kms.rst > +++ b/Documentation/gpu/drm-kms.rst > @@ -575,6 +575,13 @@ Explicit Fencing Properties > .. kernel-doc:: drivers/gpu/drm/drm_atomic_uapi.c > :doc: explicit fencing properties > > + > +Variable Refresh Properties > +--------------------------- > + > +.. kernel-doc:: drivers/gpu/drm/drm_connector.c > + :doc: Variable refresh properties > + > Existing KMS Properties > ----------------------- > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index f0deeb7298d0..2a12853ca917 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1254,6 +1254,28 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev) > } > EXPORT_SYMBOL(drm_mode_create_scaling_mode_property); > > +/** > + * DOC: Variable refresh properties > + * > + * Variable refresh rate control is supported via properties on the > + * &drm_connector and &drm_crtc objects. > + * > + * "vrr_capable": > + * Optional &drm_connector boolean property that drivers should attach > + * with drm_connector_attach_vrr_capable_property() on connectors that > + * could support variable refresh rates. Drivers should update the > + * property value by calling drm_connector_set_vrr_capable_property(). > + * > + * Absence of the property should indicate absence of support. > + * > + * "vrr_enabled": > + * Default &drm_crtc boolean property that notifies the driver that the > + * variable refresh rate adjustment should be enabled for the CRTC.

Hi,

where is the documentation that explains how drivers must implement "variable refresh rate adjustment"?

What should and could userspace expect to get if it flicks this switch?

I also think the kernel documentation should include a description of what VRR actually is and how it conceptually works as far as userspace is concerned.

That is, the kernel documentation should describe what this thing does, so that we avoid every driver implementing a different thing. For example, one driver could prevent the luminance flickering itself by tuning the timings while another driver might not do that, which means that an application tested on the former driver will look just fine while it is unbearable to watch on the latter.

Thanks, pq

I felt it was best to leave this more on the vague side to not impose restrictions yet on what a driver must do.

If you think it's worth defining what the "baseline" expectation is for userspace, I would probably describe it as "utilizing the monitor's variable refresh rate to reduce stuttering or tearing that can occur during flipping". This is currently what the amdgpu driver has enabled

I would also mention that without VRR, the display engine processes the flips independently of the rendering speed which might result into stuttering or tearing.

Might also be worth giving a quick example with numbers in the documentation. Something like: For Eg if the rendering speed is 40Hz, without VRR, display engine will flip at constant 60Hz, which means that same frame will be displayed twice which will be observed as stutter/repetition. With VRR enabled, the vertical front porch will be extended and the flip will be processed when its ready or at max blanking time resulting a smooth transition without repetition.

Good points about mentioning the problems it solves in the documentation.

...

...

...

...

for support. The implementation also isn't much more complex than just passing the variable refresh range to the hardware.

I wouldn't want every driver to be forced to implement some sort of luminance flickering by default. It's not noticeable on many panels and any tuning would inherently add latency to the output. It would probably be better left as a new property or a driver specific feature.

In general I would imagine that most future VRR features would end up as new properties. Anything that's purely software could be implemented as a drm helper that every driver can use. I think the target presentation timestamp feature is a good example for that.

Speaking of timestamps. What is the expected behaviour of vblank timestamps when vrr is enabled?

When vrr is enabled the duration of the vertical front porch will be extended until flip or timeout occurs. The vblank timestamp will vary based on duration of the vertical front porch. The min/max duration for the front porch can be specified by the driver via the min/max range.

This min max range that is read from monitor descriptor is only used to program the HW registers right? Its not exposed to the userspace, correct?

Manasi

Currently the range isn't exposed to userspace.

I think I wouldn't mind having them for testing purposes but that can just be done via debugfs. They might make more sense as properties but I don't have any integration patches to utilize them in userspace.

...

...

No changes to vblank timestamping handling should be necessary to accommodate variable refresh rate.

I think it's probably best to update the documentation for vrr_enable with some of the specifics I described above. That should help clarify userspace expectations as well.

Nicholas Kazlauskas

Yes debugfs for min, max Vrefresh range will be good.

Manasi

Nicholas Kazlauskas

On 2018-10-11 12:39 PM, Nicholas Kazlauskas wrote:

Support for AMDGPU specific FreeSync properties and ioctls are dropped from amdgpu_dm in favor of supporting drm variable refresh rate properties.

The drm vrr_capable property is now attached to any DP/HDMI connector. Its value is updated accordingly to the connector's FreeSync capabiltiy.

The freesync_enable logic and ioctl control has has been dropped in favor of utilizing the vrr_enabled on the drm CRTC. This allows for more fine grained atomic control over which CRTCs should support variable refresh rate.

To handle state changes for vrr_enabled it was easiest to drop the forced modeset on freesync_enabled change. This patch now performs the required stream updates when planes are flipped.

This is done for a few reasons:

(1) VRR stream updates can be done in the fast update path

(2) amdgpu_dm_atomic_check would need to be hacked apart to check desired variable refresh state and capability before the CRTC disable pass.

(3) Performing VRR stream updates on-flip is needed for enabling BTR support.

VRR packets and timing adjustments are now tracked and compared to previous values sent to the hardware.

Signed-off-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 255 +++++++++--------- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 7 +- 2 files changed, 138 insertions(+), 124 deletions(-)

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 6a2342d72742..d5de4b91e144 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -1802,72 +1802,6 @@ static void dm_bandwidth_update(struct amdgpu_device *adev) /* TODO: implement later */ }

-static int amdgpu_notify_freesync(struct drm_device *dev, void *data,

• 		struct drm_file *filp)
  

-{

• struct drm_atomic_state *state;
• struct drm_modeset_acquire_ctx ctx;
• struct drm_crtc *crtc;
• struct drm_connector *connector;
• struct drm_connector_state *old_con_state, *new_con_state;
• int ret = 0;
• uint8_t i;
• bool enable = false;
• drm_modeset_acquire_init(&ctx, 0);
• state = drm_atomic_state_alloc(dev);
• if (!state) {

• ret = -ENOMEM;
  

• goto out;
  

• }
• state->acquire_ctx = &ctx;

-retry:

• drm_for_each_crtc(crtc, dev) {

• ret = drm_atomic_add_affected_connectors(state, crtc);
  

• if (ret)
  

• 	goto fail;
  

• /* TODO rework amdgpu_dm_commit_planes so we don't need this */
  

• ret = drm_atomic_add_affected_planes(state, crtc);
  

• if (ret)
  

• 	goto fail;
  

• }
• for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {

• struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
  

• struct drm_crtc_state *new_crtc_state;
  

• struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc);
  

• struct dm_crtc_state *dm_new_crtc_state;
  

• if (!acrtc) {
  

• 	ASSERT(0);
  

• 	continue;
  

• }
  

• new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
  

• dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
  

• dm_new_crtc_state->freesync_enabled = enable;
  

• }
• ret = drm_atomic_commit(state);

-fail:

• if (ret == -EDEADLK) {

• drm_atomic_state_clear(state);
  

• drm_modeset_backoff(&ctx);
  

• goto retry;
  

• }
• drm_atomic_state_put(state);

-out:

• drm_modeset_drop_locks(&ctx);
• drm_modeset_acquire_fini(&ctx);
• return ret;

-}

static const struct amdgpu_display_funcs dm_display_funcs = { .bandwidth_update = dm_bandwidth_update, /* called unconditionally */ @@ -1881,7 +1815,6 @@ static const struct amdgpu_display_funcs dm_display_funcs = { dm_crtc_get_scanoutpos,/* called unconditionally */ .add_encoder = NULL, /* VBIOS parsing. DAL does it. */ .add_connector = NULL, /* VBIOS parsing. DAL does it. */

• .notify_freesync = amdgpu_notify_freesync,

Please also drop from amdgpu_display_funcs definition in amdgpu_mode.h. Might want to drop the set_freesync_property from there as well.

};

@@ -2834,7 +2767,8 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)

state->adjust = cur->adjust; state->vrr_infopacket = cur->vrr_infopacket;

• state->freesync_enabled = cur->freesync_enabled;

• state->vrr_supported = cur->vrr_supported;

• state->freesync_config = cur->freesync_config;

  /* TODO Duplicate dc_stream after objects are stream object is flattened */

@@ -3053,8 +2987,6 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector) __drm_atomic_helper_connector_duplicate_state(connector, &new_state->base);

new_state->freesync_capable = state->freesync_capable;

• new_state->freesync_enable = state->freesync_enable;
• return &new_state->base;

}

@@ -3800,6 +3732,11 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, adev->mode_info.underscan_vborder_property, 0);

• if (connector_type == DRM_MODE_CONNECTOR_HDMIA ||

•    connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
  

• drm_connector_attach_vrr_capable_property(
  

• 	&aconnector->base);
  

• }

}

static int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap, @@ -4176,6 +4113,77 @@ static void prepare_flip_isr(struct amdgpu_crtc *acrtc) acrtc->crtc_id); }

+static void update_freesync_state_on_stream(

• struct amdgpu_display_manager *dm,
• struct dm_crtc_state *new_crtc_state,
• struct dc_stream_state *new_stream)

+{

• struct mod_vrr_params vrr = {0};
• struct dc_info_packet vrr_infopacket = {0};
• struct mod_freesync_config config = new_crtc_state->freesync_config;
• if (!new_stream)

• return;
  

• /*

• * TODO: Determine why min/max totals and vrefresh can be 0 here.
  

• * For now it's sufficient to just guard against these conditions.
  

• */
  

• if (!new_stream->timing.h_total || !new_stream->timing.v_total)

• return;
  

• if (new_crtc_state->vrr_supported &&

•    config.min_refresh_in_uhz &&
  

•    config.max_refresh_in_uhz) {
  

• config.state = new_crtc_state->base.vrr_enabled ?
  

• 	VRR_STATE_ACTIVE_VARIABLE :
  

• 	VRR_STATE_INACTIVE;
  

• } else {

• config.state = VRR_STATE_UNSUPPORTED;
  

• }
• mod_freesync_build_vrr_params(dm->freesync_module,

• 		      new_stream,
  

• 		      &config, &vrr);
  

• mod_freesync_build_vrr_infopacket(

• dm->freesync_module,
  

• new_stream,
  

• &vrr,
  

• packet_type_vrr,
  

• transfer_func_unknown,
  

• &vrr_infopacket);
  

• new_crtc_state->freesync_timing_changed =

• (memcmp(&new_crtc_state->adjust,
  

• 	&vrr.adjust,
  

• 	sizeof(vrr.adjust)) != 0);
  

• new_crtc_state->freesync_vrr_info_changed =

• (memcmp(&new_crtc_state->vrr_infopacket,
  

• 	&vrr_infopacket,
  

• 	sizeof(vrr_infopacket)) != 0);
  

• new_crtc_state->adjust = vrr.adjust;
• new_crtc_state->vrr_infopacket = vrr_infopacket;
• new_stream->adjust = new_crtc_state->adjust;
• new_stream->vrr_infopacket = vrr_infopacket;
• if (new_crtc_state->freesync_vrr_info_changed)

• DRM_DEBUG_KMS("VRR packet update: crtc=%u enabled=%d state=%d",
  

• 	      new_crtc_state->base.crtc->base.id,
  

• 	      (int)new_crtc_state->base.vrr_enabled,
  

• 	      (int)vrr.state);
  

• if (new_crtc_state->freesync_timing_changed)

• DRM_DEBUG_KMS("VRR timing update: crtc=%u min=%u max=%u\n",
  

• 	      new_crtc_state->base.crtc->base.id,
  

• 	      vrr.adjust.v_total_min,
  

• 	      vrr.adjust.v_total_max);
  

+}

/*

• Executes flip

@@ -4197,6 +4205,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc, struct dc_flip_addrs addr = { {0} }; /* TODO eliminate or rename surface_update */ struct dc_surface_update surface_updates[1] = { {0} };

• struct dc_stream_update stream_update = {0}; struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state); struct dc_stream_status *stream_status;

@@ -4269,11 +4278,26 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc, } surface_updates->flip_addr = &addr;

• if (acrtc_state->stream) {

• update_freesync_state_on_stream(
  

• 	&adev->dm,
  

• 	acrtc_state,
  

• 	acrtc_state->stream);
  

• if (acrtc_state->freesync_timing_changed)
  

• 	stream_update.adjust =
  

• 		&acrtc_state->stream->adjust;
  

• if (acrtc_state->freesync_vrr_info_changed)
  

• 	stream_update.vrr_infopacket =
  

• 		&acrtc_state->stream->vrr_infopacket;
  

• }

For consistency we might also want to do this in commit_planes_to_stream before the call to dc_commit_updates_for_stream.

We should really merge the two codepaths for dc_commit_updates_for_stream one of these days.

dc_commit_updates_for_stream(adev->dm.dc, surface_updates, 1, acrtc_state->stream,

• 			     NULL,
  

• 			     &stream_update,
  		     &surface_updates->surface,
  		     state);
  
  

@@ -4333,11 +4357,6 @@ static bool commit_planes_to_stream( stream_update->dst = dc_stream->dst; stream_update->out_transfer_func = dc_stream->out_transfer_func;

• if (dm_new_crtc_state->freesync_enabled != dm_old_crtc_state->freesync_enabled) {

• stream_update->vrr_infopacket = &dc_stream->vrr_infopacket;
  

• stream_update->adjust = &dc_stream->adjust;
  

• }
• for (i = 0; i < new_plane_count; i++) { updates[i].surface = plane_states[i]; updates[i].gamma =

@@ -4473,9 +4492,6 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state, spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags); }

• dc_stream_attach->adjust = acrtc_state->adjust;
  

• dc_stream_attach->vrr_infopacket = acrtc_state->vrr_infopacket;
  

• if (false == commit_planes_to_stream(dm->dc, plane_states_constructed, planes_count,

@@ -4679,9 +4695,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) WARN_ON(!status); WARN_ON(!status->plane_count);

• dm_new_crtc_state->stream->adjust = dm_new_crtc_state->adjust;
  

• dm_new_crtc_state->stream->vrr_infopacket = dm_new_crtc_state->vrr_infopacket;
  

• /*TODO How it works with MPO ?*/ if (!commit_planes_to_stream( dm->dc,

@@ -4899,20 +4912,18 @@ static int do_aquire_global_lock(struct drm_device *dev, return ret < 0 ? ret : 0; }

-void set_freesync_on_stream(struct amdgpu_display_manager *dm,

• 	    struct dm_crtc_state *new_crtc_state,
  

• 	    struct dm_connector_state *new_con_state,
  

• 	    struct dc_stream_state *new_stream)
  

+static void get_freesync_config_for_crtc(

• struct dm_crtc_state *new_crtc_state,
• struct dm_connector_state *new_con_state)

{ struct mod_freesync_config config = {0};

• struct mod_vrr_params vrr = {0};

• struct dc_info_packet vrr_infopacket = {0}; struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(new_con_state->base.connector);

• if (new_con_state->freesync_capable &&

•    new_con_state->freesync_enable) {
  

• config.state = new_crtc_state->freesync_enabled ?
  

• new_crtc_state->vrr_supported = new_con_state->freesync_capable;
• if (new_con_state->freesync_capable) {

• config.state = new_crtc_state->base.vrr_enabled ?
  	VRR_STATE_ACTIVE_VARIABLE :
  	VRR_STATE_INACTIVE;
  

  config.min_refresh_in_uhz =

@@ -4922,19 +4933,18 @@ void set_freesync_on_stream(struct amdgpu_display_manager *dm, config.vsif_supported = true; }

• mod_freesync_build_vrr_params(dm->freesync_module,

• 		      new_stream,
  

• 		      &config, &vrr);
  

• new_crtc_state->freesync_config = config;

+}

• mod_freesync_build_vrr_infopacket(dm->freesync_module,

• 			  new_stream,
  

• 			  &vrr,
  

• 			  packet_type_fs1,
  

• 			  NULL,
  

• 			  &vrr_infopacket);
  

+static void reset_freesync_config_for_crtc(

• struct dm_crtc_state *new_crtc_state)

+{

• new_crtc_state->vrr_supported = false;

• new_crtc_state->adjust = vrr.adjust;
• new_crtc_state->vrr_infopacket = vrr_infopacket;

• memset(&new_crtc_state->adjust, 0,

•       sizeof(new_crtc_state->adjust));
  

• memset(&new_crtc_state->vrr_infopacket, 0,

•       sizeof(new_crtc_state->vrr_infopacket));
  

}

static int dm_update_crtcs_state(struct amdgpu_display_manager *dm, @@ -5009,9 +5019,6 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm, break; }

• 	set_freesync_on_stream(dm, dm_new_crtc_state,
  

• 			       dm_new_conn_state, new_stream);
  

• if (dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
      dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
  	new_crtc_state->mode_changed = false;
  

@@ -5020,9 +5027,6 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm, } }

• if (dm_old_crtc_state->freesync_enabled != dm_new_crtc_state->freesync_enabled)
  

• 	new_crtc_state->mode_changed = true;
  

• if (!drm_atomic_crtc_needs_modeset(new_crtc_state)) goto next_crtc;

@@ -5059,6 +5063,8 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm, dc_stream_release(dm_old_crtc_state->stream); dm_new_crtc_state->stream = NULL;

• 	reset_freesync_config_for_crtc(dm_new_crtc_state);
  

• *lock_and_validation_needed = true;
  
  

  } else {/* Add stream for any updated/enabled CRTC */

@@ -5136,7 +5142,9 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm, amdgpu_dm_set_ctm(dm_new_crtc_state); }

• /* Update Freesync settings. */
  

• get_freesync_config_for_crtc(dm_new_crtc_state,
  

• 			     dm_new_conn_state);
  

  }

  return ret;

@@ -5401,12 +5409,9 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, goto fail;

for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {

• struct dm_crtc_state *dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
  

• struct dm_crtc_state *dm_old_crtc_state  = to_dm_crtc_state(old_crtc_state);
  

• if (!drm_atomic_crtc_needs_modeset(new_crtc_state) && !new_crtc_state->color_mgmt_changed &&

•     (dm_old_crtc_state->freesync_enabled == dm_new_crtc_state->freesync_enabled))
  

•     !new_crtc_state->vrr_enabled)
  continue;
  
  

  if (!new_crtc_state->enable)

@@ -5558,14 +5563,15 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, struct detailed_data_monitor_range *range; struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector);

• struct dm_connector_state *dm_con_state;

• struct dm_connector_state *dm_con_state = NULL;

  struct drm_device *dev = connector->dev; struct amdgpu_device *adev = dev->dev_private;

• bool freesync_capable = false;

  if (!connector->state) { DRM_ERROR("%s - Connector has no state", __func__);

• return;
  

• goto update;
  

  }

  if (!edid) {

@@ -5575,9 +5581,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, amdgpu_dm_connector->max_vfreq = 0; amdgpu_dm_connector->pixel_clock_mhz = 0;

• dm_con_state->freesync_capable = false;
  

• dm_con_state->freesync_enable = false;
  

• return;
  

• goto update;
  

  }

  dm_con_state = to_dm_connector_state(connector->state);

@@ -5585,10 +5589,10 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, edid_check_required = false; if (!amdgpu_dm_connector->dc_sink) { DRM_ERROR("dc_sink NULL, could not add free_sync module.\n");

• return;
  

• goto update;
  

  } if (!adev->dm.freesync_module)

• return;
  

• goto update;
  

  /*
  • if edid non zero restrict freesync only for dp and edp
  */

@@ -5600,7 +5604,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, amdgpu_dm_connector); } }

• dm_con_state->freesync_capable = false; if (edid_check_required == true && (edid->version > 1 || (edid->version == 1 && edid->revision > 1))) { for (i = 0; i < 4; i++) {

@@ -5632,8 +5635,16 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10) {

• 	dm_con_state->freesync_capable = true;
  

• 	freesync_capable = true;
  

  } }

+update:

• if (dm_con_state)

• dm_con_state->freesync_capable = freesync_capable;
  

Interesting. Looks like we could just grab this from the drm_connector property as this isn't something that changes with the state but DRM forces us to track it on the connector state as drm_object_property_get_value() throws a WARN_ON for atomic drivers. Not sure I like it but it's what we got, I guess.

Harry

• if (connector->vrr_capable_property)

• drm_connector_set_vrr_capable_property(connector,
  

• 				       freesync_capable);
  

}

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h index 978b34a5011c..a5aaf8b08968 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -185,7 +185,11 @@ struct dm_crtc_state { int crc_skip_count; bool crc_enabled;

• bool freesync_enabled;

• bool freesync_timing_changed;
• bool freesync_vrr_info_changed;
• bool vrr_supported;
• struct mod_freesync_config freesync_config; struct dc_crtc_timing_adjust adjust; struct dc_info_packet vrr_infopacket;

}; @@ -207,7 +211,6 @@ struct dm_connector_state { uint8_t underscan_vborder; uint8_t underscan_hborder; bool underscan_enable;

• bool freesync_enable; bool freesync_capable;

};

On 2018-10-11 04:56 PM, Kazlauskas, Nicholas wrote:

On 10/11/2018 04:39 PM, Harry Wentland wrote:

...

On 2018-10-11 12:39 PM, Nicholas Kazlauskas wrote:

...

Support for AMDGPU specific FreeSync properties and ioctls are dropped from amdgpu_dm in favor of supporting drm variable refresh rate properties.

The drm vrr_capable property is now attached to any DP/HDMI connector. Its value is updated accordingly to the connector's FreeSync capabiltiy.

The freesync_enable logic and ioctl control has has been dropped in favor of utilizing the vrr_enabled on the drm CRTC. This allows for more fine grained atomic control over which CRTCs should support variable refresh rate.

To handle state changes for vrr_enabled it was easiest to drop the forced modeset on freesync_enabled change. This patch now performs the required stream updates when planes are flipped.

This is done for a few reasons:

(1) VRR stream updates can be done in the fast update path

(2) amdgpu_dm_atomic_check would need to be hacked apart to check      desired variable refresh state and capability before the CRTC      disable pass.

(3) Performing VRR stream updates on-flip is needed for enabling BTR      support.

VRR packets and timing adjustments are now tracked and compared to previous values sent to the hardware.

Signed-off-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 255 +++++++++---------   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   7 +-   2 files changed, 138 insertions(+), 124 deletions(-)

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 6a2342d72742..d5de4b91e144 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -1802,72 +1802,6 @@ static void dm_bandwidth_update(struct amdgpu_device *adev)       /* TODO: implement later */   }   -static int amdgpu_notify_freesync(struct drm_device *dev, void *data, -                struct drm_file *filp) -{ -    struct drm_atomic_state *state; -    struct drm_modeset_acquire_ctx ctx; -    struct drm_crtc *crtc; -    struct drm_connector *connector; -    struct drm_connector_state *old_con_state, *new_con_state; -    int ret = 0; -    uint8_t i; -    bool enable = false;

-    drm_modeset_acquire_init(&ctx, 0);

-    state = drm_atomic_state_alloc(dev); -    if (!state) { -        ret = -ENOMEM; -        goto out; -    } -    state->acquire_ctx = &ctx;

-retry: -    drm_for_each_crtc(crtc, dev) { -        ret = drm_atomic_add_affected_connectors(state, crtc); -        if (ret) -            goto fail;

-        /* TODO rework amdgpu_dm_commit_planes so we don't need this */ -        ret = drm_atomic_add_affected_planes(state, crtc); -        if (ret) -            goto fail; -    }

-    for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) { -        struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state); -        struct drm_crtc_state *new_crtc_state; -        struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc); -        struct dm_crtc_state *dm_new_crtc_state;

-        if (!acrtc) { -            ASSERT(0); -            continue; -        }

-        new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base); -        dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);

-        dm_new_crtc_state->freesync_enabled = enable; -    }

-    ret = drm_atomic_commit(state);

-fail: -    if (ret == -EDEADLK) { -        drm_atomic_state_clear(state); -        drm_modeset_backoff(&ctx); -        goto retry; -    }

-    drm_atomic_state_put(state);

-out: -    drm_modeset_drop_locks(&ctx); -    drm_modeset_acquire_fini(&ctx); -    return ret; -}     static const struct amdgpu_display_funcs dm_display_funcs = {       .bandwidth_update = dm_bandwidth_update, /* called unconditionally */ @@ -1881,7 +1815,6 @@ static const struct amdgpu_display_funcs dm_display_funcs = {           dm_crtc_get_scanoutpos,/* called unconditionally */       .add_encoder = NULL, /* VBIOS parsing. DAL does it. */       .add_connector = NULL, /* VBIOS parsing. DAL does it. */ -    .notify_freesync = amdgpu_notify_freesync,

Please also drop from amdgpu_display_funcs definition in amdgpu_mode.h. Might want to drop the set_freesync_property from there as well.

Oh right, I forgot about those. Will do.

...

...

};   @@ -2834,7 +2767,8 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)         state->adjust = cur->adjust;       state->vrr_infopacket = cur->vrr_infopacket; -    state->freesync_enabled = cur->freesync_enabled; +    state->vrr_supported = cur->vrr_supported; +    state->freesync_config = cur->freesync_config;         /* TODO Duplicate dc_stream after objects are stream object is flattened */   @@ -3053,8 +2987,6 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)       __drm_atomic_helper_connector_duplicate_state(connector, &new_state->base);         new_state->freesync_capable = state->freesync_capable; -    new_state->freesync_enable = state->freesync_enable;

return &new_state->base;   }   @@ -3800,6 +3732,11 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,                   adev->mode_info.underscan_vborder_property,                   0);   +    if (connector_type == DRM_MODE_CONNECTOR_HDMIA || +        connector_type == DRM_MODE_CONNECTOR_DisplayPort) { +        drm_connector_attach_vrr_capable_property( +            &aconnector->base); +    }   }     static int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap, @@ -4176,6 +4113,77 @@ static void prepare_flip_isr(struct amdgpu_crtc *acrtc)                            acrtc->crtc_id);   }   +static void update_freesync_state_on_stream( +    struct amdgpu_display_manager *dm, +    struct dm_crtc_state *new_crtc_state, +    struct dc_stream_state *new_stream) +{ +    struct mod_vrr_params vrr = {0}; +    struct dc_info_packet vrr_infopacket = {0}; +    struct mod_freesync_config config = new_crtc_state->freesync_config;

+    if (!new_stream) +        return;

+    /* +     * TODO: Determine why min/max totals and vrefresh can be 0 here. +     * For now it's sufficient to just guard against these conditions. +     */

+    if (!new_stream->timing.h_total || !new_stream->timing.v_total) +        return;

+    if (new_crtc_state->vrr_supported && +        config.min_refresh_in_uhz && +        config.max_refresh_in_uhz) { +        config.state = new_crtc_state->base.vrr_enabled ? +            VRR_STATE_ACTIVE_VARIABLE : +            VRR_STATE_INACTIVE; +    } else { +        config.state = VRR_STATE_UNSUPPORTED; +    }

+    mod_freesync_build_vrr_params(dm->freesync_module, +                      new_stream, +                      &config, &vrr);

+    mod_freesync_build_vrr_infopacket( +        dm->freesync_module, +        new_stream, +        &vrr, +        packet_type_vrr, +        transfer_func_unknown, +        &vrr_infopacket);

+    new_crtc_state->freesync_timing_changed = +        (memcmp(&new_crtc_state->adjust, +            &vrr.adjust, +            sizeof(vrr.adjust)) != 0);

+    new_crtc_state->freesync_vrr_info_changed = +        (memcmp(&new_crtc_state->vrr_infopacket, +            &vrr_infopacket, +            sizeof(vrr_infopacket)) != 0);

+    new_crtc_state->adjust = vrr.adjust; +    new_crtc_state->vrr_infopacket = vrr_infopacket;

+    new_stream->adjust = new_crtc_state->adjust; +    new_stream->vrr_infopacket = vrr_infopacket;

+    if (new_crtc_state->freesync_vrr_info_changed) +        DRM_DEBUG_KMS("VRR packet update: crtc=%u enabled=%d state=%d", +                  new_crtc_state->base.crtc->base.id, +                  (int)new_crtc_state->base.vrr_enabled, +                  (int)vrr.state);

+    if (new_crtc_state->freesync_timing_changed) +        DRM_DEBUG_KMS("VRR timing update: crtc=%u min=%u max=%u\n", +                  new_crtc_state->base.crtc->base.id, +                  vrr.adjust.v_total_min, +                  vrr.adjust.v_total_max); +}

/*    * Executes flip    * @@ -4197,6 +4205,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,       struct dc_flip_addrs addr = { {0} };       /* TODO eliminate or rename surface_update */       struct dc_surface_update surface_updates[1] = { {0} }; +    struct dc_stream_update stream_update = {0};       struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);       struct dc_stream_status *stream_status;   @@ -4269,11 +4278,26 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,       }       surface_updates->flip_addr = &addr;   +    if (acrtc_state->stream) { +        update_freesync_state_on_stream( +            &adev->dm, +            acrtc_state, +            acrtc_state->stream);

+        if (acrtc_state->freesync_timing_changed) +            stream_update.adjust = +                &acrtc_state->stream->adjust;

+        if (acrtc_state->freesync_vrr_info_changed) +            stream_update.vrr_infopacket = +                &acrtc_state->stream->vrr_infopacket; +    }

For consistency we might also want to do this in commit_planes_to_stream before the call to dc_commit_updates_for_stream.

We should really merge the two codepaths for dc_commit_updates_for_stream one of these days.

In a prior revision to this patch I actually did do this on both but reduced it to just the flip - the reason being that there's no point in enabling this when we're *not* flipping since it's needed for this to function properly.

The merge is probably a good idea down the line to cut down on code duplication even further.

Sounds good.

Harry

...

...

dc_commit_updates_for_stream(adev->dm.dc,                            surface_updates,                            1,                            acrtc_state->stream, -                         NULL, +                         &stream_update,                            &surface_updates->surface,                            state);   @@ -4333,11 +4357,6 @@ static bool commit_planes_to_stream(       stream_update->dst = dc_stream->dst;       stream_update->out_transfer_func = dc_stream->out_transfer_func;   -    if (dm_new_crtc_state->freesync_enabled != dm_old_crtc_state->freesync_enabled) { -        stream_update->vrr_infopacket = &dc_stream->vrr_infopacket; -        stream_update->adjust = &dc_stream->adjust; -    }

for (i = 0; i < new_plane_count; i++) {           updates[i].surface = plane_states[i];           updates[i].gamma = @@ -4473,9 +4492,6 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,               spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);           }   -        dc_stream_attach->adjust = acrtc_state->adjust; -        dc_stream_attach->vrr_infopacket = acrtc_state->vrr_infopacket;

if (false == commit_planes_to_stream(dm->dc,                               plane_states_constructed,                               planes_count, @@ -4679,9 +4695,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)           WARN_ON(!status);           WARN_ON(!status->plane_count);   -        dm_new_crtc_state->stream->adjust = dm_new_crtc_state->adjust; -        dm_new_crtc_state->stream->vrr_infopacket = dm_new_crtc_state->vrr_infopacket;

/*TODO How it works with MPO ?*/           if (!commit_planes_to_stream(                   dm->dc, @@ -4899,20 +4912,18 @@ static int do_aquire_global_lock(struct drm_device *dev,       return ret < 0 ? ret : 0;   }   -void set_freesync_on_stream(struct amdgpu_display_manager *dm, -                struct dm_crtc_state *new_crtc_state, -                struct dm_connector_state *new_con_state, -                struct dc_stream_state *new_stream) +static void get_freesync_config_for_crtc( +    struct dm_crtc_state *new_crtc_state, +    struct dm_connector_state *new_con_state)   {       struct mod_freesync_config config = {0}; -    struct mod_vrr_params vrr = {0}; -    struct dc_info_packet vrr_infopacket = {0};       struct amdgpu_dm_connector *aconnector =               to_amdgpu_dm_connector(new_con_state->base.connector);   -    if (new_con_state->freesync_capable && -        new_con_state->freesync_enable) { -        config.state = new_crtc_state->freesync_enabled ? +    new_crtc_state->vrr_supported = new_con_state->freesync_capable;

+    if (new_con_state->freesync_capable) { +        config.state = new_crtc_state->base.vrr_enabled ?                   VRR_STATE_ACTIVE_VARIABLE :                   VRR_STATE_INACTIVE;           config.min_refresh_in_uhz = @@ -4922,19 +4933,18 @@ void set_freesync_on_stream(struct amdgpu_display_manager *dm,           config.vsif_supported = true;       }   -    mod_freesync_build_vrr_params(dm->freesync_module, -                      new_stream, -                      &config, &vrr); +    new_crtc_state->freesync_config = config; +}   -    mod_freesync_build_vrr_infopacket(dm->freesync_module, -                      new_stream, -                      &vrr, -                      packet_type_fs1, -                      NULL, -                      &vrr_infopacket); +static void reset_freesync_config_for_crtc( +    struct dm_crtc_state *new_crtc_state) +{ +    new_crtc_state->vrr_supported = false;   -    new_crtc_state->adjust = vrr.adjust; -    new_crtc_state->vrr_infopacket = vrr_infopacket; +    memset(&new_crtc_state->adjust, 0, +           sizeof(new_crtc_state->adjust)); +    memset(&new_crtc_state->vrr_infopacket, 0, +           sizeof(new_crtc_state->vrr_infopacket));   }     static int dm_update_crtcs_state(struct amdgpu_display_manager *dm, @@ -5009,9 +5019,6 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,                   break;               }   -            set_freesync_on_stream(dm, dm_new_crtc_state, -                           dm_new_conn_state, new_stream);

if (dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&                   dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {                   new_crtc_state->mode_changed = false; @@ -5020,9 +5027,6 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,               }           }   -        if (dm_old_crtc_state->freesync_enabled != dm_new_crtc_state->freesync_enabled) -            new_crtc_state->mode_changed = true;

if (!drm_atomic_crtc_needs_modeset(new_crtc_state))               goto next_crtc;   @@ -5059,6 +5063,8 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,               dc_stream_release(dm_old_crtc_state->stream);               dm_new_crtc_state->stream = NULL;   +            reset_freesync_config_for_crtc(dm_new_crtc_state);

*lock_and_validation_needed = true;             } else {/* Add stream for any updated/enabled CRTC */ @@ -5136,7 +5142,9 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,               amdgpu_dm_set_ctm(dm_new_crtc_state);           }   - +        /* Update Freesync settings. */ +        get_freesync_config_for_crtc(dm_new_crtc_state, +                         dm_new_conn_state);       }         return ret; @@ -5401,12 +5409,9 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,           goto fail;         for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { -        struct dm_crtc_state *dm_new_crtc_state = to_dm_crtc_state(new_crtc_state); -        struct dm_crtc_state *dm_old_crtc_state  = to_dm_crtc_state(old_crtc_state);

if (!drm_atomic_crtc_needs_modeset(new_crtc_state) &&               !new_crtc_state->color_mgmt_changed && -            (dm_old_crtc_state->freesync_enabled == dm_new_crtc_state->freesync_enabled)) +            !new_crtc_state->vrr_enabled)               continue;             if (!new_crtc_state->enable) @@ -5558,14 +5563,15 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,       struct detailed_data_monitor_range *range;       struct amdgpu_dm_connector *amdgpu_dm_connector =               to_amdgpu_dm_connector(connector); -    struct dm_connector_state *dm_con_state; +    struct dm_connector_state *dm_con_state = NULL;         struct drm_device *dev = connector->dev;       struct amdgpu_device *adev = dev->dev_private; +    bool freesync_capable = false;         if (!connector->state) {           DRM_ERROR("%s - Connector has no state", __func__); -        return; +        goto update;       }         if (!edid) { @@ -5575,9 +5581,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,           amdgpu_dm_connector->max_vfreq = 0;           amdgpu_dm_connector->pixel_clock_mhz = 0;   -        dm_con_state->freesync_capable = false; -        dm_con_state->freesync_enable = false; -        return; +        goto update;       }         dm_con_state = to_dm_connector_state(connector->state); @@ -5585,10 +5589,10 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,       edid_check_required = false;       if (!amdgpu_dm_connector->dc_sink) {           DRM_ERROR("dc_sink NULL, could not add free_sync module.\n"); -        return; +        goto update;       }       if (!adev->dm.freesync_module) -        return; +        goto update;       /*        * if edid non zero restrict freesync only for dp and edp        */ @@ -5600,7 +5604,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,                           amdgpu_dm_connector);           }       } -    dm_con_state->freesync_capable = false;       if (edid_check_required == true && (edid->version > 1 ||          (edid->version == 1 && edid->revision > 1))) {           for (i = 0; i < 4; i++) { @@ -5632,8 +5635,16 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,           if (amdgpu_dm_connector->max_vfreq -               amdgpu_dm_connector->min_vfreq > 10) {   -            dm_con_state->freesync_capable = true; +            freesync_capable = true;           }       }

+update: +    if (dm_con_state) +        dm_con_state->freesync_capable = freesync_capable;

Interesting. Looks like we could just grab this from the drm_connector property as this isn't something that changes with the state but DRM forces us to track it on the connector state as drm_object_property_get_value() throws a WARN_ON for atomic drivers. Not sure I like it but it's what we got, I guess.

Harry

That was my original plan, but I think I understand the reasoning behind the separation of state and property.

I don't mind this approach too much because the value really feels like an implementation detail specific to our driver in this case.

It's not really describing whether the connector is actually capable or not, but whether it's fine to perform vrr updates later when we're updating the planes.

...

...

+    if (connector->vrr_capable_property) +        drm_connector_set_vrr_capable_property(connector, +                               freesync_capable);   }   diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h index 978b34a5011c..a5aaf8b08968 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -185,7 +185,11 @@ struct dm_crtc_state {       int crc_skip_count;       bool crc_enabled;   -    bool freesync_enabled; +    bool freesync_timing_changed; +    bool freesync_vrr_info_changed;

+    bool vrr_supported; +    struct mod_freesync_config freesync_config;       struct dc_crtc_timing_adjust adjust;       struct dc_info_packet vrr_infopacket;   }; @@ -207,7 +211,6 @@ struct dm_connector_state {       uint8_t underscan_vborder;       uint8_t underscan_hborder;       bool underscan_enable; -    bool freesync_enable;       bool freesync_capable;   };

Nicholas Kazlauskas