Re: [PATCH 3/3] drm/amd/display: Compensate for pre-DCE12 BTR-VRR hw limitations. (v3)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Apr 29, 2019 at 2:51 PM Kazlauskas, Nicholas
<Nicholas.Kazlauskas@xxxxxxx> wrote:
>
> On 4/26/19 5:40 PM, Mario Kleiner wrote:
> > Pre-DCE12 needs special treatment for BTR / low framerate
> > compensation for more stable behaviour:
> >
> > According to comments in the code and some testing on DCE-8
> > and DCE-11, DCE-11 and earlier only apply VTOTAL_MIN/MAX
> > programming with a lag of one frame, so the special BTR hw
> > programming for intermediate fixed duration frames must be
> > done inside the current frame at flip submission in atomic
> > commit tail, ie. one vblank earlier, and the fixed refresh
> > intermediate frame mode must be also terminated one vblank
> > earlier on pre-DCE12 display engines.
> >
> > To achieve proper termination on < DCE-12 shift the point
> > when the switch-back from fixed vblank duration to variable
> > vblank duration happens from the start of VBLANK (vblank irq,
> > as done on DCE-12+) to back-porch or end of VBLANK (handled
> > by vupdate irq handler). We must leave the switch-back code
> > inside VBLANK irq for DCE12+, as before.
> >
> > Doing this, we get much better behaviour of BTR for up-sweeps,
> > ie. going from short to long frame durations (~high to low fps)
> > and for constant framerate flips, as tested on DCE-8 and
> > DCE-11. Behaviour is still not quite as good as on DCN-1
> > though.
> >
> > On down-sweeps, going from long to short frame durations
> > (low fps to high fps) < DCE-12 is a little bit improved,
> > although by far not as much as for up-sweeps and constant
> > fps.
> >
> > v2: Fix some wrong locking, as pointed out by Nicholas.
> > v3: Simplify if-condition in vupdate-irq - nit by Nicholas.
> > Signed-off-by: Mario Kleiner <mario.kleiner.de@xxxxxxxxx>
>
> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
>

Thanks.

> I'd like to push patches 1+3 if you're okay with this.
>

Yes please! I'd love to have these in Linux 5.2, so that would be the
first well suitable kernel to target for my specific VRR use cases.

> I can see the value in the 2nd patch from the graphs and test
> application you've posted but it'll likely take some time to do thorough
> review and testing on this.
>

I understand.

> The question that needs to be examined with the second is what the
> optimal margin is, if any, for typical usecases across common monitor
> ranges. Not all monitors that support BTR have the same range, and many
> have a lower bound of ~48Hz. To me ~53Hz sounds rather early to be
> entering, but I'm not sure how it affects the user experience overall.
>

That's indeed a tricky tuning problem and i didn't spend much thought
on that. Just took the BTR_EXIT_MARGIN already defined as 2 msecs,
because it was conveniently there for the exit path and this way it
was nicely symmetric to the BTR exit path. 1 msec or less might just
be as good, or something more fancy and adaptive. But most value comes
from patch 3/3 anyway.

Atm. i don't have a Freesync capable display at all, so i don't know
how well this patch works out wrt. flicker etc, i can only look at my
plots and the output of the kms_vrr igt test. I just hacked the driver
to accept any DP or HDMI display as VRR capable, so i can run the
synthetic timing tests blindly (A G-Sync display mostly goes blank or
tells me a sad "Out of range signal" on its OSD, a HDMI->VGA connected
CRT monitor cycles between "Out of range", black, and some heavily
distorted image). The G-Sync display has a 30 - 144 Hz range and my
faking patch even set a range 25 - 144 Hz for most tests.

I also thought about sending my "fake VRR display" patch after
cleaning it up. I could add some amdgpu module parameter, e.g., bool
amdgpu.fakevrroutput,
that if set to 1 would allow the driver to accept any DP/HDMI display
as VRR capable, parsing the VRR range from EDID if present (that works
for G-Sync displays) or just faking a range like 30 - 144 Hz or
whatever is realistic? A blanked out display is boring to look at, but
at least it allows to run timing tests or the IGT kms_vrr tests
without the need for a dedicated FreeSync display.

>From reading on the web I understand that "FreeSync2 HDR" displays are
also HDR capable and go through some testing and certification at AMD
wrt. to minimal flicker? Would you have some recommendation or
pointers to a display i could buy if i needed a large VRR range,
minimal flicker and also good HDR capabilities - ideally with locally
dimmable backlight or such? I will probably have the money to buy one
FreeSync display, but that should also be useable for working on HDR
related stuff.

Thanks,
-mario

> Nicholas Kazlauskas
>
> > ---
> >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 48 +++++++++++++++++--
> >   1 file changed, 44 insertions(+), 4 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 76b6e621793f..92b3c2cec7dd 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -364,6 +364,7 @@ static void dm_vupdate_high_irq(void *interrupt_params)
> >       struct amdgpu_device *adev = irq_params->adev;
> >       struct amdgpu_crtc *acrtc;
> >       struct dm_crtc_state *acrtc_state;
> > +     unsigned long flags;
> >
> >       acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
> >
> > @@ -379,8 +380,25 @@ static void dm_vupdate_high_irq(void *interrupt_params)
> >                * page-flip completion events that have been queued to us
> >                * if a pageflip happened inside front-porch.
> >                */
> > -             if (amdgpu_dm_vrr_active(acrtc_state))
> > +             if (amdgpu_dm_vrr_active(acrtc_state)) {
> >                       drm_crtc_handle_vblank(&acrtc->base);
> > +
> > +                     /* BTR processing for pre-DCE12 ASICs */
> > +                     if (acrtc_state->stream &&
> > +                         adev->family < AMDGPU_FAMILY_AI) {
> > +                             spin_lock_irqsave(&adev->ddev->event_lock, flags);
> > +                             mod_freesync_handle_v_update(
> > +                                 adev->dm.freesync_module,
> > +                                 acrtc_state->stream,
> > +                                 &acrtc_state->vrr_params);
> > +
> > +                             dc_stream_adjust_vmin_vmax(
> > +                                 adev->dm.dc,
> > +                                 acrtc_state->stream,
> > +                                 &acrtc_state->vrr_params.adjust);
> > +                             spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> > +                     }
> > +             }
> >       }
> >   }
> >
> > @@ -390,6 +408,7 @@ static void dm_crtc_high_irq(void *interrupt_params)
> >       struct amdgpu_device *adev = irq_params->adev;
> >       struct amdgpu_crtc *acrtc;
> >       struct dm_crtc_state *acrtc_state;
> > +     unsigned long flags;
> >
> >       acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
> >
> > @@ -412,9 +431,10 @@ static void dm_crtc_high_irq(void *interrupt_params)
> >                */
> >               amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> >
> > -             if (acrtc_state->stream &&
> > +             if (acrtc_state->stream && adev->family >= AMDGPU_FAMILY_AI &&
> >                   acrtc_state->vrr_params.supported &&
> >                   acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> > +                     spin_lock_irqsave(&adev->ddev->event_lock, flags);
> >                       mod_freesync_handle_v_update(
> >                               adev->dm.freesync_module,
> >                               acrtc_state->stream,
> > @@ -424,6 +444,7 @@ static void dm_crtc_high_irq(void *interrupt_params)
> >                               adev->dm.dc,
> >                               acrtc_state->stream,
> >                               &acrtc_state->vrr_params.adjust);
> > +                     spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> >               }
> >       }
> >   }
> > @@ -4878,8 +4899,10 @@ static void update_freesync_state_on_stream(
> >       struct dc_plane_state *surface,
> >       u32 flip_timestamp_in_us)
> >   {
> > -     struct mod_vrr_params vrr_params = new_crtc_state->vrr_params;
> > +     struct mod_vrr_params vrr_params;
> >       struct dc_info_packet vrr_infopacket = {0};
> > +     struct amdgpu_device *adev = dm->adev;
> > +     unsigned long flags;
> >
> >       if (!new_stream)
> >               return;
> > @@ -4892,6 +4915,9 @@ static void update_freesync_state_on_stream(
> >       if (!new_stream->timing.h_total || !new_stream->timing.v_total)
> >               return;
> >
> > +     spin_lock_irqsave(&adev->ddev->event_lock, flags);
> > +     vrr_params = new_crtc_state->vrr_params;
> > +
> >       if (surface) {
> >               mod_freesync_handle_preflip(
> >                       dm->freesync_module,
> > @@ -4899,6 +4925,12 @@ static void update_freesync_state_on_stream(
> >                       new_stream,
> >                       flip_timestamp_in_us,
> >                       &vrr_params);
> > +
> > +             if (adev->family < AMDGPU_FAMILY_AI &&
> > +                 amdgpu_dm_vrr_active(new_crtc_state)) {
> > +                     mod_freesync_handle_v_update(dm->freesync_module,
> > +                                                  new_stream, &vrr_params);
> > +             }
> >       }
> >
> >       mod_freesync_build_vrr_infopacket(
> > @@ -4930,6 +4962,8 @@ static void update_freesync_state_on_stream(
> >                             new_crtc_state->base.crtc->base.id,
> >                             (int)new_crtc_state->base.vrr_enabled,
> >                             (int)vrr_params.state);
> > +
> > +     spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> >   }
> >
> >   static void pre_update_freesync_state_on_stream(
> > @@ -4937,8 +4971,10 @@ static void pre_update_freesync_state_on_stream(
> >       struct dm_crtc_state *new_crtc_state)
> >   {
> >       struct dc_stream_state *new_stream = new_crtc_state->stream;
> > -     struct mod_vrr_params vrr_params = new_crtc_state->vrr_params;
> > +     struct mod_vrr_params vrr_params;
> >       struct mod_freesync_config config = new_crtc_state->freesync_config;
> > +     struct amdgpu_device *adev = dm->adev;
> > +     unsigned long flags;
> >
> >       if (!new_stream)
> >               return;
> > @@ -4950,6 +4986,9 @@ static void pre_update_freesync_state_on_stream(
> >       if (!new_stream->timing.h_total || !new_stream->timing.v_total)
> >               return;
> >
> > +     spin_lock_irqsave(&adev->ddev->event_lock, flags);
> > +     vrr_params = new_crtc_state->vrr_params;
> > +
> >       if (new_crtc_state->vrr_supported &&
> >           config.min_refresh_in_uhz &&
> >           config.max_refresh_in_uhz) {
> > @@ -4970,6 +5009,7 @@ static void pre_update_freesync_state_on_stream(
> >                       sizeof(vrr_params.adjust)) != 0);
> >
> >       new_crtc_state->vrr_params = vrr_params;
> > +     spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> >   }
> >
> >   static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
> >
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux