Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Manasi Navare]

On Fri, Oct 26, 2018 at 08:06:11PM +, Kazlauskas, Nicholas wrote:
> On 10/26/18 3:13 PM, Manasi Navare wrote:
> > On Fri, Oct 26, 2018 at 05:34:15PM +, Kazlauskas, Nicholas wrote:
> >> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
> >>> On Fri, Oct 26, 2018 at 02:49:31PM +, Kazlauskas, Nicholas wrote:
>  On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> > On Thu, 11 Oct 2018 12:39:41 -0400
> > Nicholas Kazlauskas  wrote:
> >
> >> These include the drm_connector 'vrr_capable' and the drm_crtc
> >> 'vrr_enabled' properties.
> >>
> >> Signed-off-by: Nicholas Kazlauskas 
> >> ---
> >> 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
> >> + * _connector and _crtc objects.
> >> + *
> >> + * "vrr_capable":
> >> + *Optional _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 _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 

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 10/26/18 3:13 PM, Manasi Navare wrote:
> On Fri, Oct 26, 2018 at 05:34:15PM +, Kazlauskas, Nicholas wrote:
>> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
>>> On Fri, Oct 26, 2018 at 02:49:31PM +, Kazlauskas, Nicholas wrote:
 On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> On Thu, 11 Oct 2018 12:39:41 -0400
> Nicholas Kazlauskas  wrote:
>
>> These include the drm_connector 'vrr_capable' and the drm_crtc
>> 'vrr_enabled' properties.
>>
>> Signed-off-by: Nicholas Kazlauskas 
>> ---
>> 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
>> + * _connector and _crtc objects.
>> + *
>> + * "vrr_capable":
>> + *  Optional _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 _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 

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Manasi Navare]

On Fri, Oct 26, 2018 at 05:34:15PM +, Kazlauskas, Nicholas wrote:
> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
> > On Fri, Oct 26, 2018 at 02:49:31PM +, Kazlauskas, Nicholas wrote:
> >> On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> >>> On Thu, 11 Oct 2018 12:39:41 -0400
> >>> Nicholas Kazlauskas  wrote:
> >>>
>  These include the drm_connector 'vrr_capable' and the drm_crtc
>  'vrr_enabled' properties.
> 
>  Signed-off-by: Nicholas Kazlauskas 
>  ---
> 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
>  + * _connector and _crtc objects.
>  + *
>  + * "vrr_capable":
>  + *  Optional _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 _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.

Re: [PATCH] drm/v3d: Fix compilation error on kbuilt test robot.

[Christian König]

Am 26.10.18 um 16:43 schrieb Andrey Grodzovsky:

Signed-off-by: Andrey Grodzovsky 


Reviewed-by: Christian König 


---
  drivers/gpu/drm/v3d/v3d_sched.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 273d0fb..80b641f 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -222,7 +222,7 @@ v3d_sched_init(struct v3d_dev *v3d)
 _sched_ops,
 hw_jobs_limit, job_hang_limit,
 msecs_to_jiffies(hang_limit_ms),
-"v3d_render", true);
+"v3d_render");
if (ret) {
dev_err(v3d->dev, "Failed to create render scheduler: %d.",
ret);


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Ville Syrjälä]

On Fri, Oct 26, 2018 at 05:34:15PM +, Kazlauskas, Nicholas wrote:
> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
> > On Fri, Oct 26, 2018 at 02:49:31PM +, Kazlauskas, Nicholas wrote:
> >> On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> >>> On Thu, 11 Oct 2018 12:39:41 -0400
> >>> Nicholas Kazlauskas  wrote:
> >>>
>  These include the drm_connector 'vrr_capable' and the drm_crtc
>  'vrr_enabled' properties.
> 
>  Signed-off-by: Nicholas Kazlauskas 
>  ---
> 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
>  + * _connector and _crtc objects.
>  + *
>  + * "vrr_capable":
>  + *  Optional _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 _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 

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 10/26/18 10:53 AM, Ville Syrjälä wrote:
> On Fri, Oct 26, 2018 at 02:49:31PM +, Kazlauskas, Nicholas wrote:
>> On 10/26/18 7:37 AM, Pekka Paalanen wrote:
>>> On Thu, 11 Oct 2018 12:39:41 -0400
>>> Nicholas Kazlauskas  wrote:
>>>
 These include the drm_connector 'vrr_capable' and the drm_crtc
 'vrr_enabled' properties.

 Signed-off-by: Nicholas Kazlauskas 
 ---
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
 + * _connector and _crtc objects.
 + *
 + * "vrr_capable":
 + *Optional _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 _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 

Re: [PATCH] RFC: Make igts for cross-driver stuff mandatory?

[Alex Deucher]

On Fri, Oct 19, 2018 at 4:51 AM Daniel Vetter  wrote:
>
> Hi all,
>
> This is just to collect feedback on this idea, and see whether the
> overall dri-devel community stands on all this. I think the past few
> cross-vendor uapi extensions all came with igts attached, and
> personally I think there's lots of value in having them: A
> cross-vendor interface isn't useful if every driver implements it
> slightly differently.
>
> I think there's 2 questions here:
>
> - Do we want to make such testcases mandatory?
>
> - If yes, are we there yet, or is there something crucially missing
>   still?
>
> And of course there's a bunch of details to figure out. Like we
> probably want to also recommend the selftests/unit-tests in
> drivers/gpu/drm/selftest, since fairly often that's a much more
> effective approach to checking the details than from userspace.
>
> Feedback and thoughts very much appreciated.

As long as we can fix up any cross platform issues, this seems reasonable to me.

Alex

>
> Cheers, Daniel
> ---
>  Documentation/gpu/drm-uapi.rst | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 4b4bf2c5eac5..91cf6e4b6303 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -238,6 +238,13 @@ DRM specific patterns. Note that ENOTTY has the slightly 
> unintuitive meaning of
>  Testing and validation
>  ==
>
> +Testing Requirements for userspace API
> +--
> +
> +New cross-driver userspace interface extensions, like new IOCTL, new KMS
> +properties, new files in sysfs or anything else that constitutes an API 
> change
> +need to have driver-agnostic testcases in IGT for that feature.
> +
>  Validating changes with IGT
>  ---
>
> --
> 2.19.1
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Fix compute ring 1.0.0 failure after reset

[Grodzovsky, Andrey]

On 10/26/2018 04:05 AM, Christian König wrote:
> Am 25.10.18 um 22:16 schrieb Andrey Grodzovsky:
>> Problem: After GPU reset on dGPUs with gfx8 compute ring
>> 1.0.0 fails to pass the ring test. Ring registers inspection
>> shows that it's active and no hang is observed (rptr == wptr)
>> No significant diffs were observed between CP_HQD* registers
>> for the ring in good and bad shape.
>>
>> Fix: No clear reason why but reversing the order of ring tests
>> fixes the problem.
>>
>> Signed-off-by: Andrey Grodzovsky 
>
> Mhm, maybe try adding a delay before the ring test?
First thing I tried, didn't help.
>
> Could be that the rings are started in reverse order as well and for 
> some reason the first one is start tested to quickly after a reset.

No, KCQ queues mapping just before the test goes in 0..max order.

Andrey
>
> Anyway patch is Acked-by: Christian König 
>
> Thanks,
> Christian.
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 6 --
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> index b2e1376..02f8ca5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> @@ -4811,8 +4811,10 @@ static int gfx_v8_0_kcq_resume(struct 
>> amdgpu_device *adev)
>>   if (r)
>>   goto done;
>>   -    /* Test KCQs */
>> -    for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>> +    /* Test KCQs - reversing the order of rings seems to fix ring 
>> test failure
>> + * after GPU reset
>> + */
>> +    for (i = adev->gfx.num_compute_rings - 1; i >= 0; i--) {
>>   ring = >gfx.compute_ring[i];
>>   r = amdgpu_ring_test_helper(ring);
>>   }
>

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Ville Syrjälä]

On Fri, Oct 26, 2018 at 02:49:31PM +, Kazlauskas, Nicholas wrote:
> On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> > On Thu, 11 Oct 2018 12:39:41 -0400
> > Nicholas Kazlauskas  wrote:
> > 
> >> These include the drm_connector 'vrr_capable' and the drm_crtc
> >> 'vrr_enabled' properties.
> >>
> >> Signed-off-by: Nicholas Kazlauskas 
> >> ---
> >>   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
> >> + * _connector and _crtc objects.
> >> + *
> >> + * "vrr_capable":
> >> + *Optional _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 _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?

-- 
Ville Syrjälä
Intel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/v3d: Fix compilation error on kbuilt test robot.

[Alex Deucher]

On Fri, Oct 26, 2018 at 10:43 AM Andrey Grodzovsky
 wrote:
>
> Signed-off-by: Andrey Grodzovsky 
Reviewed-by: Alex Deucher 

> ---
>  drivers/gpu/drm/v3d/v3d_sched.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
> index 273d0fb..80b641f 100644
> --- a/drivers/gpu/drm/v3d/v3d_sched.c
> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> @@ -222,7 +222,7 @@ v3d_sched_init(struct v3d_dev *v3d)
>  _sched_ops,
>  hw_jobs_limit, job_hang_limit,
>  msecs_to_jiffies(hang_limit_ms),
> -"v3d_render", true);
> +"v3d_render");
> if (ret) {
> dev_err(v3d->dev, "Failed to create render scheduler: %d.",
> ret);
> --
> 2.7.4
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 10/26/18 7:37 AM, Pekka Paalanen wrote:
> On Thu, 11 Oct 2018 12:39:41 -0400
> Nicholas Kazlauskas  wrote:
> 
>> These include the drm_connector 'vrr_capable' and the drm_crtc
>> 'vrr_enabled' properties.
>>
>> Signed-off-by: Nicholas Kazlauskas 
>> ---
>>   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
>> + * _connector and _crtc objects.
>> + *
>> + * "vrr_capable":
>> + *  Optional _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 _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 _connector object.
>> + */
>> +
>>   /**
>>* drm_connector_attach_vrr_capable_property - creates the
>>* vrr_capable property
> 

Nicholas Kazlauskas
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/v3d: Fix compilation error on kbuilt test robot.

[Andrey Grodzovsky]

Signed-off-by: Andrey Grodzovsky 
---
 drivers/gpu/drm/v3d/v3d_sched.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 273d0fb..80b641f 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -222,7 +222,7 @@ v3d_sched_init(struct v3d_dev *v3d)
 _sched_ops,
 hw_jobs_limit, job_hang_limit,
 msecs_to_jiffies(hang_limit_ms),
-"v3d_render", true);
+"v3d_render");
if (ret) {
dev_err(v3d->dev, "Failed to create render scheduler: %d.",
ret);
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [igt-dev] [PATCH] RFC: Make igts for cross-driver stuff mandatory?

[Alex Deucher]

On Fri, Oct 26, 2018 at 4:32 AM Daniel Vetter  wrote:
>
> On Fri, Oct 26, 2018 at 5:50 AM Zhou, David(ChunMing)
>  wrote:
> >
> > Make igt for cross-driver, I think you should rename it first, not an intel 
> > specific. NO company wants their employee working on other company stuff.
> > You can rename it to DGT(drm graphics test), and published following  
> > libdrm, or directly merge to libdrm, then everyone  can use it and develop 
> > it in same page, which is only my personal opinion.
>
> We renamed it ot  IGT gpu tools, that was even enough for ARM folks.
> If this is seriously what AMD expects before considering, I'm not sure
> what to say.
>
> Alex, Christian, is this the official AMD stance that you can't touch
> stuff because of the letter i?

We don't have any restrictions.

Alex

> -Daniel
>
>
> > Regards,
> > David
> >
> > > -Original Message-
> > > From: dri-devel  On Behalf Of 
> > > Eric
> > > Anholt
> > > Sent: Friday, October 26, 2018 12:36 AM
> > > To: Sean Paul ; Daniel Vetter 
> > > Cc: IGT development ; Intel Graphics
> > > Development ; DRI Development  > > de...@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org
> > > Subject: Re: [igt-dev] [PATCH] RFC: Make igts for cross-driver stuff
> > > mandatory?
> > >
> > > Sean Paul  writes:
> > >
> > > > On Fri, Oct 19, 2018 at 10:50:49AM +0200, Daniel Vetter wrote:
> > > >> Hi all,
> > > >>
> > > >> This is just to collect feedback on this idea, and see whether the
> > > >> overall dri-devel community stands on all this. I think the past few
> > > >> cross-vendor uapi extensions all came with igts attached, and
> > > >> personally I think there's lots of value in having them: A
> > > >> cross-vendor interface isn't useful if every driver implements it
> > > >> slightly differently.
> > > >>
> > > >> I think there's 2 questions here:
> > > >>
> > > >> - Do we want to make such testcases mandatory?
> > > >>
> > > >
> > > > Yes, more testing == better code.
> > > >
> > > >
> > > >> - If yes, are we there yet, or is there something crucially missing
> > > >>   still?
> > > >
> > > > In my experience, no. Last week while trying to replicate an intel-gfx
> > > > CI failure, I tried compiling igt for one of my (intel) chromebooks.
> > > > It seems like cross-compilation (or, in my case, just specifying
> > > > prefix/ld_library_path/sbin_path) is broken on igt. If we want to
> > > > impose restrictions across the entire subsystem, we need to make sure
> > > > that everyone can build and deploy igt easily.
> > > >
> > > > I managed to hack around everything and get it working, but I still
> > > > haven't tried switching out the toolchain. Once we have some GitLab CI
> > > > to validate cross-compilation, then we can consider making IGT 
> > > > mandatory.
> > > >
> > > > It's possible that I'm just a meson n00b and didn't use the right
> > > > incantation, so maybe it already works, but then we need better
> > > documentation.
> > > >
> > > > I've pasted my horrible hacks below, I also didn't have libunwind, so
> > > > removed its usage.
> > >
> > > I've also had to cut out libunwind for cross-compiling on many occasions.
> > > Worst library.
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v5 2/4] drm: Add vrr_enabled property to drm CRTC

[Pekka Paalanen]

On Mon, 15 Oct 2018 12:06:52 +0200
Michel Dänzer  wrote:

> On 2018-10-15 11:47 a.m., Christian König wrote:
> > Am 15.10.2018 um 11:40 schrieb Michel Dänzer:  
> >> On 2018-10-13 7:38 p.m., Christian König wrote:  
> >>> Am 12.10.2018 um 18:44 schrieb Nicholas Kazlauskas:  
>  This patch introduces the 'vrr_enabled' CRTC property to allow
>  dynamic control over variable refresh rate support for a CRTC.
> 
>  This property should be treated like a content hint to the driver -
>  if the hardware or driver is not capable of driving variable refresh
>  timings then this is not considered an error.
> 
>  Capability for variable refresh rate support should be determined
>  by querying the vrr_capable drm connector property.
> 
>  It is worth noting that while the property is intended for atomic use
>  it isn't filtered from legacy userspace queries. This allows for Xorg
>  userspace drivers to implement support.  
> >>> I'm honestly still not convinced that a per CRTC property is actually
> >>> the right approach.
> >>>
> >>> What we should rather do instead is to implement a target timestamp for
> >>> the page flip.  
> >> You'd have to be more specific about how the latter could completely
> >> replace the former. I don't see how it could.  
> > 
> > Each flip request send by an application gets a timestamp when the flip
> > should be displayed.
> > 
> > If I'm not completely mistaken we already have something like that in
> > both DRI2 as well as DRI3.  
> 
> Certainly not DRI2, but for now we're not enabling VRR with that anyway.
> 
> While the X11 Present extension specifies PresentOptionUST for this,
> support for it isn't implemented yet in xserver (as in setting the
> option has no effect, the X server interprets the target value as an MSC
> anyway).
> 
> So this couldn't work before the next major release of xserver, which
> based on recent history could be at least about one year away.

Hi

> In contrast, the CRTC property based solution for the gaming use-case
> can work even with older xserver versions.

This is probably the heaviest reason.

Coming up with a KMS UABI for target timestamps could get complicated.
Do you need a flip queue deeper than one? Do you need to be able to
cancel flips?

> > So as far as I can see we only need to add an extra flag that those
> > information are trust worth in the context of VRR as well.
> > 
> > If we don't set this flag we always get the always working fallback
> > behavior, e.g. VRR is disabled and we have a fixed refresh rate.
> > 
> > If we set this flag and the timestamp is in the past we get the VRR
> > behavior to display the next frame as soon as possible.
> > 
> > If we set this flag and specific a specific timestamp then we get the
> > VRR behavior to display the frame as close as possible to the specified
> > timestamp.  
> 
> Apart from the above, another issue is that this would give direct
> control to the client on whether or not VRR should be used. But we want
> to allow the user to disable VRR even if a client wants to use it, via
> an RandR output property. This requires that the Xorg driver can control
> whether or not VRR can actually be used, via the CRTC property added by
> this patch.

It would not imply direct control to clients. The target timestamps go
through the X server, the X server can mangle them or remove them
before calling KMS any way it wants. The X server can invent a RandR
property to disable/enable VRR.

One would need a video-DDX update the very minimum to start passing the
timestamps to the kernel, so there is no way VRR would be enabled
unwanted.


Thanks,
pq


pgpQtr8CslWSA.pgp
Description: OpenPGP digital signature
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: set backlight level limit to 1

[Wentland, Harry]

On 2018-10-26 2:44 a.m., Guttula, Suresh wrote:
> 
> 
> On 10/26/2018 12:04 AM, Wentland, Harry wrote:
>> On 2018-10-25 2:58 a.m., Guttula, Suresh wrote:
>>> This patch will work as workaround for silicon limitation
>>> related to PWM dutycycle when the backlight level goes to 0.
>>>
>>> Actually PWM value is 16 bit value and valid range from 1-65535.
>>> when ever user requested to set this PWM value to 0 which is not
>>> fall in the range, in VBIOS taken care this by limiting to 1.
>>> This patch here will do the same. Either driver or VBIOS can not
>>> pass 0 value as it is not a valid range for PWM and it will
>>> give a high PWM pulse which is not the intented behaviour as
>>> per HW constraints.
>>>
>>
>>
>> Comments from Windows dev, Anthony, CCed here as well:
>>> In Windows, we typically never set backlight PWM value to 0.
>>> Windows brightness slider is from 0 - 100% brightness.
>>> 0% brightness maps to some non-zero value. So user never gets the chance to 
>>> set value to 0.
>>>
>>> I'm not 100% sure what kind of flashing they are seeing.
>>> I thought 0 PWM works, and just means PWM signal always low, I think it 
>>> would equate to panel backlight completely being off.
>>> I do know of a flashing issue caused by fractional PWM at low PWM values, 
>>> but this would not even be at 0. The flashing problem would be anywhere 
>>> from 1 - 300 for example. (out of 65535)
>>
>> Do you see flickering when value is at 0 or in the 1 > 0 transition? 1 would 
>> be 65535/256 = 256 and put us right in the 1-300 range that has a flashing 
>> problem.
>>
>> Harry
>>
> 
> We can not make out if flicker is at transition from 1 to 0 or exactly 
> at 0, because the flickering is very instantaneous.But, I am not able to 
> see the issue @1 , only seen the issue after going down from 1.
> 

Apparently Windows driver also checks ACPI ATIF, VBIOS, and the Windows 
registry for backlight range information. When no info is present it limits 
backlight to the 12-255 range. We might want to implement the ATIF and VBIOS 
checks on amdgpu as well, but for now I think your fix is good.

Reviewed-by: Harry Wentland 

Harry

> Thanks,
> Suresh G
> 
>>> Signed-off-by: suresh guttula 
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 
>>>   1 file changed, 8 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 492230c..38f84b2 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -1518,6 +1518,14 @@ static int amdgpu_dm_backlight_update_status(struct 
>>> backlight_device *bd)
>>>   {
>>> struct amdgpu_display_manager *dm = bl_get_data(bd);
>>>   
>>> +   /*
>>> +* When we use brightness low key to reduce the brightness,
>>> +* brightness level reaching to 0, with which we can see flash
>>> +* screen on ui beacuse of HW limitation.To avoid that  we are
>>> +* limiting level to 1
>>> +*/
>>> +   if (bd->props.brightness < 1)
>>> +   return 1;
>>> if (dc_link_set_backlight_level(dm->backlight_link,
>>> bd->props.brightness, 0, 0))
>>> return 0;
>>>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/3] Revert "drm/amd/powerplay: Enable/Disable NBPSTATE on On/OFF of UVD"

[Guttula, Suresh]

From: "S, Shirish" 

This reverts commit dbd8299c32f6f413f6cfe322fe0308f3cfc577e8.

Reason for revert:
This patch sends  msg PPSMC_MSG_DisableLowMemoryPstate(0x002e)
in wrong of sequence to SMU which is before PPSMC_MSG_UVDPowerON (0x0008).
This leads to SMU failing to service the request as it is
dependent on UVD to be powered ON, since it accesses UVD
registers.

This msg should ideally be sent only when the UVD is about to decode
a 4k video.

Signed-off-by: Shirish S 
---
 drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
index fef111d..53cf787 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
@@ -1228,17 +1228,14 @@ static int smu8_dpm_force_dpm_level(struct pp_hwmgr 
*hwmgr,
 
 static int smu8_dpm_powerdown_uvd(struct pp_hwmgr *hwmgr)
 {
-   if (PP_CAP(PHM_PlatformCaps_UVDPowerGating)) {
-   smu8_nbdpm_pstate_enable_disable(hwmgr, true, true);
+   if (PP_CAP(PHM_PlatformCaps_UVDPowerGating))
return smum_send_msg_to_smc(hwmgr, PPSMC_MSG_UVDPowerOFF);
-   }
return 0;
 }
 
 static int smu8_dpm_powerup_uvd(struct pp_hwmgr *hwmgr)
 {
if (PP_CAP(PHM_PlatformCaps_UVDPowerGating)) {
-   smu8_nbdpm_pstate_enable_disable(hwmgr, false, true);
return smum_send_msg_to_smc_with_parameter(
hwmgr,
PPSMC_MSG_UVDPowerON,
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/3] drm/amd/powerplay:add hwmgr callback to update nbpstate on Carrizo

[Guttula, Suresh]

This callback is used to access hwmgr function named as
cz_nbdpm_pstate_enable_disable.

Signed-off-by: suresh guttula 
---
 drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c | 1 +
 drivers/gpu/drm/amd/powerplay/inc/hwmgr.h| 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
index 53cf787..553a203 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
@@ -1992,6 +1992,7 @@ static const struct pp_hwmgr_func smu8_hwmgr_funcs = {
.power_state_set = smu8_set_power_state_tasks,
.dynamic_state_management_disable = smu8_disable_dpm_tasks,
.notify_cac_buffer_info = smu8_notify_cac_buffer_info,
+   .update_nbdpm_pstate = smu8_nbdpm_pstate_enable_disable,
.get_thermal_temperature_range = smu8_get_thermal_temperature_range,
 };
 
diff --git a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h 
b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
index 07d180ce..fb0f96f 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
@@ -317,6 +317,9 @@ struct pp_hwmgr_func {
uint32_t mc_addr_low,
uint32_t mc_addr_hi,
uint32_t size);
+   int (*update_nbdpm_pstate)(struct pp_hwmgr *hwmgr,
+   bool enable,
+   bool lock);
int (*get_thermal_temperature_range)(struct pp_hwmgr *hwmgr,
struct PP_TemperatureRange *range);
int (*get_power_profile_mode)(struct pp_hwmgr *hwmgr, char *buf);
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/3] drm/amd:Enable/Disable NBPSTATE on On/OFF of UVD

[Guttula, Suresh]

We observe black lines (underflow) on display when playing a
4K video with UVD. On Disabling Low memory P state this issue is
not seen.
In this patch ,disabling low memory P state only when video
size >= 4k.
Multiple runs of power measurement shows no imapct

Signed-off-by: suresh guttula 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c  | 17 +
 drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c |  4 +++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
index e5a6db6..6902719 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
@@ -38,6 +38,7 @@
 #include "amdgpu_uvd.h"
 #include "cikd.h"
 #include "uvd/uvd_4_2_d.h"
+#include "hwmgr.h"
 
 /* 1 second timeout */
 #define UVD_IDLE_TIMEOUT   msecs_to_jiffies(1000)
@@ -78,6 +79,7 @@
 #define UVD_GPCOM_VCPU_DATA1   0x03c5
 #define UVD_NO_OP  0x03ff
 #define UVD_BASE_SI0x3800
+#define WIDTH_4K3840
 
 /**
  * amdgpu_uvd_cs_ctx - Command submission parser context
@@ -528,6 +530,21 @@ static int amdgpu_uvd_cs_msg_decode(struct amdgpu_device 
*adev, uint32_t *msg,
unsigned image_size, tmp, min_dpb_size, num_dpb_buffer;
unsigned min_ctx_size = ~0;
 
+//disable Low Memory PState for UVD(4k videos)
+   if (adev->asic_type == CHIP_STONEY && width >= WIDTH_4K) {
+   struct pp_hwmgr  *hwmgr;
+   struct pp_instance *pp_handle =
+   (struct pp_instance *)adev->powerplay.pp_handle;
+   if (pp_handle) {
+   hwmgr = pp_handle->hwmgr;
+   if (hwmgr && hwmgr->hwmgr_func &&
+   hwmgr->hwmgr_func->update_nbdpm_pstate)
+   hwmgr->hwmgr_func->update_nbdpm_pstate(hwmgr,
+   false,
+   true);
+   }
+   }
+
image_size = width * height;
image_size += image_size / 2;
image_size = ALIGN(image_size, 1024);
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
index 553a203..0bf56f7 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
@@ -1228,8 +1228,10 @@ static int smu8_dpm_force_dpm_level(struct pp_hwmgr 
*hwmgr,
 
 static int smu8_dpm_powerdown_uvd(struct pp_hwmgr *hwmgr)
 {
-   if (PP_CAP(PHM_PlatformCaps_UVDPowerGating))
+   if (PP_CAP(PHM_PlatformCaps_UVDPowerGating)) {
+   smu8_nbdpm_pstate_enable_disable(hwmgr, true, true);
return smum_send_msg_to_smc(hwmgr, PPSMC_MSG_UVDPowerOFF);
+   }
return 0;
 }
 
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: add VM test to exercise max/min address space

[Deucher, Alexander]

Reviewed-by: Alex Deucher 


From: amd-gfx  on behalf of Christian 
König 
Sent: Friday, October 26, 2018 6:59:21 AM
To: amd-gfx@lists.freedesktop.org
Subject: [PATCH libdrm] amdgpu: add VM test to exercise max/min address space

Make sure the kernel doesn't crash if we map something at the minimum/maximum 
address.

Signed-off-by: Christian König 
---
 tests/amdgpu/vm_tests.c | 45 -
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/tests/amdgpu/vm_tests.c b/tests/amdgpu/vm_tests.c
index 7b6dc5d6..bbdeef4d 100644
--- a/tests/amdgpu/vm_tests.c
+++ b/tests/amdgpu/vm_tests.c
@@ -31,8 +31,8 @@ static  amdgpu_device_handle device_handle;
 static  uint32_t  major_version;
 static  uint32_t  minor_version;

-
 static void amdgpu_vmid_reserve_test(void);
+static void amdgpu_vm_mapping_test(void);

 CU_BOOL suite_vm_tests_enable(void)
 {
@@ -84,6 +84,7 @@ int suite_vm_tests_clean(void)

 CU_TestInfo vm_tests[] = {
 { "resere vmid test",  amdgpu_vmid_reserve_test },
+   { "vm mapping test",  amdgpu_vm_mapping_test },
 CU_TEST_INFO_NULL,
 };

@@ -167,3 +168,45 @@ static void amdgpu_vmid_reserve_test(void)
 r = amdgpu_cs_ctx_free(context_handle);
 CU_ASSERT_EQUAL(r, 0);
 }
+
+static void amdgpu_vm_mapping_test(void)
+{
+   struct amdgpu_bo_alloc_request req = {0};
+   struct drm_amdgpu_info_device dev_info;
+   const uint64_t size = 4096;
+   amdgpu_bo_handle buf;
+   uint64_t addr;
+   int r;
+
+   req.alloc_size = size;
+   req.phys_alignment = 0;
+   req.preferred_heap = AMDGPU_GEM_DOMAIN_GTT;
+   req.flags = 0;
+
+   r = amdgpu_bo_alloc(device_handle, , );
+   CU_ASSERT_EQUAL(r, 0);
+
+   r = amdgpu_query_info(device_handle, AMDGPU_INFO_DEV_INFO,
+ sizeof(dev_info), _info);
+   CU_ASSERT_EQUAL(r, 0);
+
+   addr = dev_info.virtual_address_offset;
+   r = amdgpu_bo_va_op(buf, 0, size, addr, 0, AMDGPU_VA_OP_MAP);
+   CU_ASSERT_EQUAL(r, 0);
+
+   addr = dev_info.virtual_address_max - size;
+   r = amdgpu_bo_va_op(buf, 0, size, addr, 0, AMDGPU_VA_OP_MAP);
+   CU_ASSERT_EQUAL(r, 0);
+
+   if (dev_info.high_va_offset) {
+   addr = dev_info.high_va_offset;
+   r = amdgpu_bo_va_op(buf, 0, size, addr, 0, AMDGPU_VA_OP_MAP);
+   CU_ASSERT_EQUAL(r, 0);
+
+   addr = dev_info.high_va_max - size;
+   r = amdgpu_bo_va_op(buf, 0, size, addr, 0, AMDGPU_VA_OP_MAP);
+   CU_ASSERT_EQUAL(r, 0);
+   }
+
+   amdgpu_bo_free(buf);
+}
--
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v4 3/4] drm: Document variable refresh properties

[Pekka Paalanen]

On Thu, 11 Oct 2018 12:39:41 -0400
Nicholas Kazlauskas  wrote:

> These include the drm_connector 'vrr_capable' and the drm_crtc
> 'vrr_enabled' properties.
> 
> Signed-off-by: Nicholas Kazlauskas 
> ---
>  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
> + * _connector and _crtc objects.
> + *
> + * "vrr_capable":
> + *   Optional _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 _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

> + *
> + *   Support for variable refresh rate will depend on the "vrr_capable"
> + *   property exposed on the _connector object.
> + */
> +
>  /**
>   * drm_connector_attach_vrr_capable_property - creates the
>   * vrr_capable property



pgp6GpcQTijeC.pgp
Description: OpenPGP digital signature
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm] amdgpu: add VM test to exercise max/min address space

[Christian König]

Make sure the kernel doesn't crash if we map something at the minimum/maximum 
address.

Signed-off-by: Christian König 
---
 tests/amdgpu/vm_tests.c | 45 -
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/tests/amdgpu/vm_tests.c b/tests/amdgpu/vm_tests.c
index 7b6dc5d6..bbdeef4d 100644
--- a/tests/amdgpu/vm_tests.c
+++ b/tests/amdgpu/vm_tests.c
@@ -31,8 +31,8 @@ static  amdgpu_device_handle device_handle;
 static  uint32_t  major_version;
 static  uint32_t  minor_version;
 
-
 static void amdgpu_vmid_reserve_test(void);
+static void amdgpu_vm_mapping_test(void);
 
 CU_BOOL suite_vm_tests_enable(void)
 {
@@ -84,6 +84,7 @@ int suite_vm_tests_clean(void)
 
 CU_TestInfo vm_tests[] = {
{ "resere vmid test",  amdgpu_vmid_reserve_test },
+   { "vm mapping test",  amdgpu_vm_mapping_test },
CU_TEST_INFO_NULL,
 };
 
@@ -167,3 +168,45 @@ static void amdgpu_vmid_reserve_test(void)
r = amdgpu_cs_ctx_free(context_handle);
CU_ASSERT_EQUAL(r, 0);
 }
+
+static void amdgpu_vm_mapping_test(void)
+{
+   struct amdgpu_bo_alloc_request req = {0};
+   struct drm_amdgpu_info_device dev_info;
+   const uint64_t size = 4096;
+   amdgpu_bo_handle buf;
+   uint64_t addr;
+   int r;
+
+   req.alloc_size = size;
+   req.phys_alignment = 0;
+   req.preferred_heap = AMDGPU_GEM_DOMAIN_GTT;
+   req.flags = 0;
+
+   r = amdgpu_bo_alloc(device_handle, , );
+   CU_ASSERT_EQUAL(r, 0);
+
+   r = amdgpu_query_info(device_handle, AMDGPU_INFO_DEV_INFO,
+ sizeof(dev_info), _info);
+   CU_ASSERT_EQUAL(r, 0);
+
+   addr = dev_info.virtual_address_offset;
+   r = amdgpu_bo_va_op(buf, 0, size, addr, 0, AMDGPU_VA_OP_MAP);
+   CU_ASSERT_EQUAL(r, 0);
+
+   addr = dev_info.virtual_address_max - size;
+   r = amdgpu_bo_va_op(buf, 0, size, addr, 0, AMDGPU_VA_OP_MAP);
+   CU_ASSERT_EQUAL(r, 0);
+
+   if (dev_info.high_va_offset) {
+   addr = dev_info.high_va_offset;
+   r = amdgpu_bo_va_op(buf, 0, size, addr, 0, AMDGPU_VA_OP_MAP);
+   CU_ASSERT_EQUAL(r, 0);
+
+   addr = dev_info.high_va_max - size;
+   r = amdgpu_bo_va_op(buf, 0, size, addr, 0, AMDGPU_VA_OP_MAP);
+   CU_ASSERT_EQUAL(r, 0);
+   }
+
+   amdgpu_bo_free(buf);
+}
-- 
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 0/3] A DRM API for adaptive sync and variable refresh rate support

[Michel Dänzer]

On 2018-10-25 7:57 p.m., Wentland, Harry wrote:
> On 2018-10-12 4:31 a.m., Koenig, Christian wrote:
>> Am 12.10.2018 um 10:26 schrieb Michel Dänzer:
>>> On 2018-10-11 9:44 p.m., Harry Wentland wrote:
 On 2018-10-03 04:25 AM, Mike Lothian wrote:
> I'm curious to know whether this will/could work over PRIME
 I don't see why this shouldn't work over PRIME as long as the 
 presenting GPU supports the new variable refresh rate API, but
 I know very little about prime, so maybe someone else can chime
 in and give a better opinion.
>>> It won't work for displays connected to a secondary GPU, because
>>> those aren't hooked up to the Present extension directly.
>>> 
>>> It can theoretically work with render offloading, if the primary
>>> GPU can scan out directly from the shared pixmap. This is only
>>> possible with current AMD APUs which support scatter/gather
>>> scanout (Carrizo and newer?).
>> 
>> Actually only Carizzo and Stoney at the moment because this is
>> buggy on Raven. Not sure if that is a pure software or a hardware
>> problem.
>> 
>> Christian.
>> 
>>> One gotcha is that xf86-video-amdgpu currently doesn't allow 
>>> flipping between buffers with different tiling parameters (BTW
>>> Harry, does that work with DC?), but that should be easy to fix.
>> 
> 
> Not currently. Fixable, but unfortunately not as easy as I'd hope.
> The good news is that I'm planning to rework that code so it would be
> easy to fix or should just happen on a per-flip basis.

FWIW, I wouldn't spend any effort specifically on making this work. It's
not intended to work with the non-atomic flip ioctl at least, and it
definitely doesn't work without DC, so xf86-video-amdgpu will have to
handle it anyway.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [igt-dev] [PATCH] RFC: Make igts for cross-driver stuff mandatory?

[Daniel Vetter]

On Fri, Oct 26, 2018 at 5:50 AM Zhou, David(ChunMing)
 wrote:
>
> Make igt for cross-driver, I think you should rename it first, not an intel 
> specific. NO company wants their employee working on other company stuff.
> You can rename it to DGT(drm graphics test), and published following  libdrm, 
> or directly merge to libdrm, then everyone  can use it and develop it in same 
> page, which is only my personal opinion.

We renamed it ot  IGT gpu tools, that was even enough for ARM folks.
If this is seriously what AMD expects before considering, I'm not sure
what to say.

Alex, Christian, is this the official AMD stance that you can't touch
stuff because of the letter i?
-Daniel


> Regards,
> David
>
> > -Original Message-
> > From: dri-devel  On Behalf Of Eric
> > Anholt
> > Sent: Friday, October 26, 2018 12:36 AM
> > To: Sean Paul ; Daniel Vetter 
> > Cc: IGT development ; Intel Graphics
> > Development ; DRI Development  > de...@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org
> > Subject: Re: [igt-dev] [PATCH] RFC: Make igts for cross-driver stuff
> > mandatory?
> >
> > Sean Paul  writes:
> >
> > > On Fri, Oct 19, 2018 at 10:50:49AM +0200, Daniel Vetter wrote:
> > >> Hi all,
> > >>
> > >> This is just to collect feedback on this idea, and see whether the
> > >> overall dri-devel community stands on all this. I think the past few
> > >> cross-vendor uapi extensions all came with igts attached, and
> > >> personally I think there's lots of value in having them: A
> > >> cross-vendor interface isn't useful if every driver implements it
> > >> slightly differently.
> > >>
> > >> I think there's 2 questions here:
> > >>
> > >> - Do we want to make such testcases mandatory?
> > >>
> > >
> > > Yes, more testing == better code.
> > >
> > >
> > >> - If yes, are we there yet, or is there something crucially missing
> > >>   still?
> > >
> > > In my experience, no. Last week while trying to replicate an intel-gfx
> > > CI failure, I tried compiling igt for one of my (intel) chromebooks.
> > > It seems like cross-compilation (or, in my case, just specifying
> > > prefix/ld_library_path/sbin_path) is broken on igt. If we want to
> > > impose restrictions across the entire subsystem, we need to make sure
> > > that everyone can build and deploy igt easily.
> > >
> > > I managed to hack around everything and get it working, but I still
> > > haven't tried switching out the toolchain. Once we have some GitLab CI
> > > to validate cross-compilation, then we can consider making IGT mandatory.
> > >
> > > It's possible that I'm just a meson n00b and didn't use the right
> > > incantation, so maybe it already works, but then we need better
> > documentation.
> > >
> > > I've pasted my horrible hacks below, I also didn't have libunwind, so
> > > removed its usage.
> >
> > I've also had to cut out libunwind for cross-compiling on many occasions.
> > Worst library.



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Fix compute ring 1.0.0 failure after reset

[Christian König]

Am 25.10.18 um 22:16 schrieb Andrey Grodzovsky:

Problem: After GPU reset on dGPUs with gfx8 compute ring
1.0.0 fails to pass the ring test. Ring registers inspection
shows that it's active and no hang is observed (rptr == wptr)
No significant diffs were observed between CP_HQD* registers
for the ring in good and bad shape.

Fix: No clear reason why but reversing the order of ring tests
fixes the problem.

Signed-off-by: Andrey Grodzovsky 


Mhm, maybe try adding a delay before the ring test?

Could be that the rings are started in reverse order as well and for 
some reason the first one is start tested to quickly after a reset.


Anyway patch is Acked-by: Christian König 

Thanks,
Christian.


---
  drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index b2e1376..02f8ca5 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -4811,8 +4811,10 @@ static int gfx_v8_0_kcq_resume(struct amdgpu_device 
*adev)
if (r)
goto done;
  
-	/* Test KCQs */

-   for (i = 0; i < adev->gfx.num_compute_rings; i++) {
+   /* Test KCQs - reversing the order of rings seems to fix ring test 
failure
+* after GPU reset
+*/
+   for (i = adev->gfx.num_compute_rings - 1; i >= 0; i--) {
ring = >gfx.compute_ring[i];
r = amdgpu_ring_test_helper(ring);
}


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/8] dma-buf: remove shared fence staging in reservation object

[Christian König]

Am 25.10.18 um 23:21 schrieb Chris Wilson:

Quoting Chris Wilson (2018-10-25 21:20:21)

Quoting Chris Wilson (2018-10-25 21:15:17)

Quoting Christian König (2018-10-04 14:12:43)

No need for that any more. Just replace the list when there isn't enough
room any more for the additional fence.

Just a heads up. After this series landed, we started hitting a
use-after-free when iterating the shared list.

<4> [109.613162] general protection fault:  [#1] PREEMPT SMP PTI
<4> [109.613177] CPU: 1 PID: 1357 Comm: gem_busy Tainted: G U
4.19.0-rc8-CI-CI_DRM_5035+ #1
<4> [109.613189] Hardware name: Dell Inc. XPS 8300  /0Y2MRG, BIOS A06 10/17/2011
<4> [109.613252] RIP: 0010:i915_gem_busy_ioctl+0x146/0x380 [i915]
<4> [109.613261] Code: 0b 43 04 49 83 c6 08 4d 39 e6 89 43 04 74 6d 4d 8b 3e e8 5d 54 
f4 e0 85 c0 74 0d 80 3d 08 71 1d 00 00 0f 84 bb 00 00 00 31 c0 <49> 81 7f 08 20 3a 2c 
a0 75 cc 41 8b 97 50 02 00 00 49 8b 8f a8 00
<4> [109.613283] RSP: 0018:c944bcf8 EFLAGS: 00010246
<4> [109.613292] RAX:  RBX: c944bdc0 RCX: 
0001
<4> [109.613302] RDX:  RSI:  RDI: 
822474a0
<4> [109.613311] RBP: c944bd28 R08: 88021e158680 R09: 
0001
<4> [109.613321] R10: 0040 R11:  R12: 
88021e1641b8
<4> [109.613331] R13: 0003 R14: 88021e1641b0 R15: 
6b6b6b6b6b6b6b6b
<4> [109.613341] FS:  7f9c9fc84980() GS:880227a4() 
knlGS:
<4> [109.613352] CS:  0010 DS:  ES:  CR0: 80050033
<4> [109.613360] CR2: 7f9c9fcb8000 CR3: 0002247d4005 CR4: 
000606e0

First guess would be

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index 5fb4fd461908..833698a0d548 100644
--- a/drivers/dma-buf/reservation.c
+++ b/drivers/dma-buf/reservation.c
@@ -169,7 +169,6 @@ void reservation_object_add_shared_fence(struct 
reservation_object *obj,
 }

 BUG_ON(fobj->shared_count >= fobj->shared_max);
-   fobj->shared_count++;

  replace:
 /*
@@ -177,6 +176,8 @@ void reservation_object_add_shared_fence(struct 
reservation_object *obj,
  * fobj->shared_count is protected by this lock too
  */
 RCU_INIT_POINTER(fobj->shared[i], fence);
+   if (i == fobj->shared_count)
+   fobj->shared_count++;
 write_seqcount_end(>seq);
 preempt_enable();
  }

Strictly, perhaps WRITE_ONCE(fobj->shared_count, i + 1); ?

Updating shared_count after setting the fence does the trick.


Ah, crap. I can stare at the code for ages and don't see that problem. 
Neither did any internal testing showed any issues.


Anyway your change is Reviewed-by: Christian König 



Thanks for the quick fix,
Christian.


-Chris


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: set backlight level limit to 1

[Guttula]

On 10/26/2018 12:04 AM, Wentland, Harry wrote:
> On 2018-10-25 2:58 a.m., Guttula, Suresh wrote:
>> This patch will work as workaround for silicon limitation
>> related to PWM dutycycle when the backlight level goes to 0.
>>
>> Actually PWM value is 16 bit value and valid range from 1-65535.
>> when ever user requested to set this PWM value to 0 which is not
>> fall in the range, in VBIOS taken care this by limiting to 1.
>> This patch here will do the same. Either driver or VBIOS can not
>> pass 0 value as it is not a valid range for PWM and it will
>> give a high PWM pulse which is not the intented behaviour as
>> per HW constraints.
>>
> 
> 
> Comments from Windows dev, Anthony, CCed here as well:
>> In Windows, we typically never set backlight PWM value to 0.
>> Windows brightness slider is from 0 - 100% brightness.
>> 0% brightness maps to some non-zero value. So user never gets the chance to 
>> set value to 0.
>>
>> I'm not 100% sure what kind of flashing they are seeing.
>> I thought 0 PWM works, and just means PWM signal always low, I think it 
>> would equate to panel backlight completely being off.
>> I do know of a flashing issue caused by fractional PWM at low PWM values, 
>> but this would not even be at 0. The flashing problem would be anywhere from 
>> 1 - 300 for example. (out of 65535)
> 
> Do you see flickering when value is at 0 or in the 1 > 0 transition? 1 would 
> be 65535/256 = 256 and put us right in the 1-300 range that has a flashing 
> problem.
> 
> Harry
> 

We can not make out if flicker is at transition from 1 to 0 or exactly 
at 0, because the flickering is very instantaneous.But, I am not able to 
see the issue @1 , only seen the issue after going down from 1.

Thanks,
Suresh G

>> Signed-off-by: suresh guttula 
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 
>>   1 file changed, 8 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 492230c..38f84b2 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -1518,6 +1518,14 @@ static int amdgpu_dm_backlight_update_status(struct 
>> backlight_device *bd)
>>   {
>>  struct amdgpu_display_manager *dm = bl_get_data(bd);
>>   
>> +/*
>> + * When we use brightness low key to reduce the brightness,
>> + * brightness level reaching to 0, with which we can see flash
>> + * screen on ui beacuse of HW limitation.To avoid that  we are
>> + * limiting level to 1
>> + */
>> +if (bd->props.brightness < 1)
>> +return 1;
>>  if (dc_link_set_backlight_level(dm->backlight_link,
>>  bd->props.brightness, 0, 0))
>>  return 0;
>>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix VM leaf walking

[Christian König]

Yeah, that came to my mind as well.

But this is the only case where we would use the return value and not 
use cursor->pfn as criteria to abort.


So to be consistent I rather don't want to do this,
Christian.

Am 25.10.18 um 17:43 schrieb Zhu, Rex:


How about add a return value for the function amdgpu_vm_pt_next?

And change the code as:

 ret = amdgpu_vm_pt_next(adev, cursor);
-   while (amdgpu_vm_pt_descendant(adev, cursor));
+   if (!ret)
+   while (amdgpu_vm_pt_descendant(adev, cursor));

Best Regards

Rex

*From:* amd-gfx  *On Behalf Of 
*Zhu, Rex

*Sent:* Thursday, October 25, 2018 11:34 PM
*To:* Deucher, Alexander ; Christian König 
; amd-gfx@lists.freedesktop.org

*Subject:* RE: [PATCH] drm/amdgpu: fix VM leaf walking

Patch is Tested-by:  Rex Zhu rex@amd.com 

Regards

Rex

*From:* amd-gfx > *On Behalf Of 
*Deucher, Alexander

*Sent:* Thursday, October 25, 2018 11:08 PM
*To:* Christian König >; 
amd-gfx@lists.freedesktop.org 

*Subject:* Re: [PATCH] drm/amdgpu: fix VM leaf walking

Acked-by: Alex Deucher >




*From:*amd-gfx > on behalf of Christian 
König >

*Sent:* Thursday, October 25, 2018 10:38 AM
*To:* amd-gfx@lists.freedesktop.org 
*Subject:* [PATCH] drm/amdgpu: fix VM leaf walking

Make sure we don't try to go down further after the leave walk already
ended. This fixes a crash with a new VM test.

Signed-off-by: Christian König >

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c

index db0cbf8d219d..352b30409060 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -542,7 +542,8 @@ static void amdgpu_vm_pt_next_leaf(struct 
amdgpu_device *adev,

    struct amdgpu_vm_pt_cursor *cursor)
 {
 amdgpu_vm_pt_next(adev, cursor);
-   while (amdgpu_vm_pt_descendant(adev, cursor));
+   if (cursor->pfn != ~0ll)
+   while (amdgpu_vm_pt_descendant(adev, cursor));
 }

 /**
--
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org 
https://lists.freedesktop.org/mailman/listinfo/amd-gfx



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx