Re: [PATCH v7 2/5] drm: Add vrr_enabled property to drm CRTC

["Kazlauskas, Nicholas" <Nicholas.Kazlauskas-5C7GfCeVMHo@public.gmane.org>]

On 1/30/19 6:02 AM, Daniel Vetter wrote:
> On Wed, Jan 30, 2019 at 11:42 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Thu, Nov 8, 2018 at 3:44 PM Nicholas Kazlauskas
>> <nicholas.kazlauskas@amd.com> wrote:
>>>
>>> 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.
>>>
>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
>>> Cc: Manasi Navare <manasi.d.navare@intel.com>
>>> ---
>>>   drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++
>>>   drivers/gpu/drm/drm_crtc.c        | 2 ++
>>>   drivers/gpu/drm/drm_mode_config.c | 6 ++++++
>>>   include/drm/drm_crtc.h            | 9 +++++++++
>>>   include/drm/drm_mode_config.h     | 5 +++++
>>>   5 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>>> index d5b7f315098c..eec396a57b88 100644
>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>> @@ -433,6 +433,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>>                  ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>>>                  drm_property_blob_put(mode);
>>>                  return ret;
>>> +       } else if (property == config->prop_vrr_enabled) {
>>> +               state->vrr_enabled = val;
>>>          } else if (property == config->degamma_lut_property) {
>>>                  ret = drm_atomic_replace_property_blob_from_id(dev,
>>>                                          &state->degamma_lut,
>>> @@ -491,6 +493,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>>>                  *val = state->active;
>>>          else if (property == config->prop_mode_id)
>>>                  *val = (state->mode_blob) ? state->mode_blob->base.id : 0;
>>> +       else if (property == config->prop_vrr_enabled)
>>> +               *val = state->vrr_enabled;
>>>          else if (property == config->degamma_lut_property)
>>>                  *val = (state->degamma_lut) ? state->degamma_lut->base.id : 0;
>>>          else if (property == config->ctm_property)
>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>> index 268a182ae189..6f8ddfcfaba5 100644
>>> --- a/drivers/gpu/drm/drm_crtc.c
>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>> @@ -340,6 +340,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>>>                  drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
>>>                  drm_object_attach_property(&crtc->base,
>>>                                             config->prop_out_fence_ptr, 0);
>>> +               drm_object_attach_property(&crtc->base,
>>> +                                          config->prop_vrr_enabled, 0);
>>>          }
>>>
>>>          return 0;
>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>>> index ee80788f2c40..5670c67f28d4 100644
>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>> @@ -310,6 +310,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>>>                  return -ENOMEM;
>>>          dev->mode_config.prop_mode_id = prop;
>>>
>>> +       prop = drm_property_create_bool(dev, 0,
>>> +                       "VRR_ENABLED");
>>
>> VRR_ENABLED doesn't match vrr_enabled in the docs. But the connector
>> one seems to indeed be lowercase. And casing matters for properties
>>
>> Can you pls figure out who's right here and fix this confusion up?
> 
> I checked the igt quickly, patch is typed, will send out today or so.
> -Daniel

Yeah, it should be "VRR_ENABLED" in the docs (like the other CRTC 
default properties, eg. explicit fencing "IN_FENCE_FD” and "OUT_FENCE_PTR".

It looks a bit weird next to "vrr_capable" but it makes sense in the 
usage context for CRTC vs connector at least.

This should be fixed in the docs and it sounds like you have a patch so 
I'll drop my R-B when it's up.

Thanks!

Nicholas Kazlauskas

> 
>>
>> Thanks, Daniel
>>
>>> +       if (!prop)
>>> +               return -ENOMEM;
>>> +       dev->mode_config.prop_vrr_enabled = prop;
>>> +
>>>          prop = drm_property_create(dev,
>>>                          DRM_MODE_PROP_BLOB,
>>>                          "DEGAMMA_LUT", 0);
>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>> index b21437bc95bf..39c3900aab3c 100644
>>> --- a/include/drm/drm_crtc.h
>>> +++ b/include/drm/drm_crtc.h
>>> @@ -290,6 +290,15 @@ struct drm_crtc_state {
>>>           */
>>>          u32 pageflip_flags;
>>>
>>> +       /**
>>> +        * @vrr_enabled:
>>> +        *
>>> +        * Indicates if variable refresh rate should be enabled for the CRTC.
>>> +        * Support for the requested vrr state will depend on driver and
>>> +        * hardware capabiltiy - lacking support is not treated as failure.
>>> +        */
>>> +       bool vrr_enabled;
>>> +
>>>          /**
>>>           * @event:
>>>           *
>>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>>> index 928e4172a0bb..49f2fcfdb5fc 100644
>>> --- a/include/drm/drm_mode_config.h
>>> +++ b/include/drm/drm_mode_config.h
>>> @@ -639,6 +639,11 @@ struct drm_mode_config {
>>>           * connectors must be of and active must be set to disabled, too.
>>>           */
>>>          struct drm_property *prop_mode_id;
>>> +       /**
>>> +        * @prop_vrr_enabled: Default atomic CRTC property to indicate
>>> +        * whether variable refresh rate should be enabled on the CRTC.
>>> +        */
>>> +       struct drm_property *prop_vrr_enabled;
>>>
>>>          /**
>>>           * @dvi_i_subconnector_property: Optional DVI-I property to
>>> --
>>> 2.19.1
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>>
>> --
>> 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