Re: [Intel-gfx] [RFC V3 0/2] Attach and Set vrr_enabled property

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 17, 2022 at 12:56:34PM +0530, Bhanuprakash Modem wrote:
> This series will add a support to set the vrr_enabled property for
> crtc based on the platform support and the request from userspace.
> And userspace can also query to get the status of "vrr_enabled".
> 
> Test-with: 20220422075223.2792586-2-bhanuprakash.modem@intel.com
> 
> Bhanuprakash Modem (2):
>   drm/vrr: Attach vrr_enabled property to the drm crtc
>   drm/i915/vrr: Set drm crtc vrr_enabled property

I'm rather confused by this patch set:

- This seems to move the property from connector to crtc without any
  justification. For uapi that we want to have standardized (anything
  around kms really) that's no good, unless there's really a mandatory
  semantic reason pls stick to existing uapi.

- If the driver interface doesn't fit (maybe the helper should be on the
  crtc and adjust the property for all connector) pls roll that change out
  to all drivers.

- This is uapi, so needs igt tests and userspace. For igts we should make
  sure they're generic so that they apply across all drivers which already
  support this property, and not just create new intel-only testcases.

- Finally the property is set up, but not wired through. Or at least I'm
  not seeing how this can even work.

So no idea what exactly you're aiming for here and what kind of comments
you want, but this doesn't look like it's on the right path at all.

Cheers, Daniel


> 
>  drivers/gpu/drm/drm_crtc.c               | 26 ++++++++++++++++++++++++
>  drivers/gpu/drm/drm_mode_config.c        |  2 +-
>  drivers/gpu/drm/i915/display/intel_vrr.c |  8 ++++++++
>  include/drm/drm_crtc.h                   |  3 +++
>  4 files changed, 38 insertions(+), 1 deletion(-)
> 
> --
> 2.35.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch