diff mbox

[v11,06/11] drm: Add DRM client cap for aspect-ratio

Message ID 1524231111-17440-7-git-send-email-ankit.k.nautiyal@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nautiyal, Ankit K April 20, 2018, 1:31 p.m. UTC
From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

To enable aspect-ratio support in DRM, blindly exposing the aspect
ratio information along with mode, can break things in existing
user-spaces which have no intention or support to use this aspect
ratio information.

To avoid this, a new drm client cap is required to enable a
user-space to advertise if it supports modes with aspect-ratio. Based
on this cap value, the kernel will take a call on exposing the aspect
ratio info in modes or not.

This patch adds the client cap for aspect-ratio.

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

V3: rebase
V4: As suggested by Marteen Lankhorst modified the commit message
    explaining the need to use the DRM cap for aspect-ratio. Also,
    tweaked the comment lines in the code for better understanding and
    clarity, as recommended by Shashank Sharma.
V5: rebase
V6: rebase
V7: rebase
V8: rebase
V9: rebase
V10: added comment explaining that no userspace breaks on aspect-ratio
     mode bits.

Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_ioctl.c | 9 +++++++++
 include/drm/drm_file.h      | 8 ++++++++
 include/uapi/drm/drm.h      | 7 +++++++
 3 files changed, 24 insertions(+)

Comments

Ville Syrjala April 20, 2018, 2:07 p.m. UTC | #1
On Fri, Apr 20, 2018 at 07:01:46PM +0530, Nautiyal, Ankit K wrote:
> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> To enable aspect-ratio support in DRM, blindly exposing the aspect
> ratio information along with mode, can break things in existing
> user-spaces which have no intention or support to use this aspect
> ratio information.
> 
> To avoid this, a new drm client cap is required to enable a
> user-space to advertise if it supports modes with aspect-ratio. Based
> on this cap value, the kernel will take a call on exposing the aspect
> ratio info in modes or not.
> 
> This patch adds the client cap for aspect-ratio.
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> V3: rebase
> V4: As suggested by Marteen Lankhorst modified the commit message
>     explaining the need to use the DRM cap for aspect-ratio. Also,
>     tweaked the comment lines in the code for better understanding and
>     clarity, as recommended by Shashank Sharma.
> V5: rebase
> V6: rebase
> V7: rebase
> V8: rebase
> V9: rebase
> V10: added comment explaining that no userspace breaks on aspect-ratio
>      mode bits.
> 
> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_ioctl.c | 9 +++++++++
>  include/drm/drm_file.h      | 8 ++++++++
>  include/uapi/drm/drm.h      | 7 +++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index af78291..39c8eab 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -325,6 +325,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  		file_priv->atomic = req->value;
>  		file_priv->universal_planes = req->value;
>  		break;
> +	case DRM_CLIENT_CAP_ASPECT_RATIO:
> +		if (req->value > 1)
> +			return -EINVAL;
> +	/*
> +	 * No Atomic userspace blows up on aspect ratio mode bits. Checked in
> +	 * wayland/weston, xserver, and hardware-composer modeset paths.
> +	 */

Bogus indentation.

Also where's the aspect_ratio_allowed handling for the atomic cap?
Or did we decide against it after all?

> +		file_priv->aspect_ratio_allowed = req->value;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 5176c37..02b7dde 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -182,6 +182,14 @@ struct drm_file {
>  	unsigned atomic:1;
>  
>  	/**
> +	 * @aspect_ratio_allowed:
> +	 *
> +	 * True, if client can handle picture aspect ratios, and has requested
> +	 * to pass this information along with the mode.
> +	 */
> +	unsigned aspect_ratio_allowed:1;
> +
> +	/**
>  	 * @is_master:
>  	 *
>  	 * This client is the creator of @master. Protected by struct
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 6fdff59..9c660e1 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -680,6 +680,13 @@ struct drm_get_cap {
>   */
>  #define DRM_CLIENT_CAP_ATOMIC	3
>  
> +/**
> + * DRM_CLIENT_CAP_ASPECT_RATIO
> + *
> + * If set to 1, the DRM core will provide aspect ratio information in modes.
> + */
> +#define DRM_CLIENT_CAP_ASPECT_RATIO    4
> +
>  /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
>  struct drm_set_client_cap {
>  	__u64 capability;
> -- 
> 2.7.4
Nautiyal, Ankit K April 23, 2018, 5:13 a.m. UTC | #2
On 4/20/2018 7:37 PM, Ville Syrjälä wrote:
> On Fri, Apr 20, 2018 at 07:01:46PM +0530, Nautiyal, Ankit K wrote:
>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> To enable aspect-ratio support in DRM, blindly exposing the aspect
>> ratio information along with mode, can break things in existing
>> user-spaces which have no intention or support to use this aspect
>> ratio information.
>>
>> To avoid this, a new drm client cap is required to enable a
>> user-space to advertise if it supports modes with aspect-ratio. Based
>> on this cap value, the kernel will take a call on exposing the aspect
>> ratio info in modes or not.
>>
>> This patch adds the client cap for aspect-ratio.
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> V3: rebase
>> V4: As suggested by Marteen Lankhorst modified the commit message
>>      explaining the need to use the DRM cap for aspect-ratio. Also,
>>      tweaked the comment lines in the code for better understanding and
>>      clarity, as recommended by Shashank Sharma.
>> V5: rebase
>> V6: rebase
>> V7: rebase
>> V8: rebase
>> V9: rebase
>> V10: added comment explaining that no userspace breaks on aspect-ratio
>>       mode bits.
>>
>> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_ioctl.c | 9 +++++++++
>>   include/drm/drm_file.h      | 8 ++++++++
>>   include/uapi/drm/drm.h      | 7 +++++++
>>   3 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index af78291..39c8eab 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -325,6 +325,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>>   		file_priv->atomic = req->value;
>>   		file_priv->universal_planes = req->value;
>>   		break;
>> +	case DRM_CLIENT_CAP_ASPECT_RATIO:
>> +		if (req->value > 1)
>> +			return -EINVAL;
>> +	/*
>> +	 * No Atomic userspace blows up on aspect ratio mode bits. Checked in
>> +	 * wayland/weston, xserver, and hardware-composer modeset paths.
>> +	 */
> Bogus indentation.

Thanks to point that out, will fix this.

> Also where's the aspect_ratio_allowed handling for the atomic cap?
> Or did we decide against it after all?

As discussed, aspect ratio is handled in the atomic modeset path, where 
in the modeset requests with aspect-ratios
are rejected, if the aspect-ratio cap not set.
The part which is dropped is - hiding the aspect-ratio information, 
while returning a mode blob in the drm_mode_get_blob
if the cap is not set. The patch was dropped as no user-space currently 
uses getblob ioctl to get the mode-blob.

Regards,
Ankit
>
>> +		file_priv->aspect_ratio_allowed = req->value;
>> +		break;
>>   	default:
>>   		return -EINVAL;
>>   	}
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index 5176c37..02b7dde 100644
>> --- a/include/drm/drm_file.h
>> +++ b/include/drm/drm_file.h
>> @@ -182,6 +182,14 @@ struct drm_file {
>>   	unsigned atomic:1;
>>   
>>   	/**
>> +	 * @aspect_ratio_allowed:
>> +	 *
>> +	 * True, if client can handle picture aspect ratios, and has requested
>> +	 * to pass this information along with the mode.
>> +	 */
>> +	unsigned aspect_ratio_allowed:1;
>> +
>> +	/**
>>   	 * @is_master:
>>   	 *
>>   	 * This client is the creator of @master. Protected by struct
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 6fdff59..9c660e1 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -680,6 +680,13 @@ struct drm_get_cap {
>>    */
>>   #define DRM_CLIENT_CAP_ATOMIC	3
>>   
>> +/**
>> + * DRM_CLIENT_CAP_ASPECT_RATIO
>> + *
>> + * If set to 1, the DRM core will provide aspect ratio information in modes.
>> + */
>> +#define DRM_CLIENT_CAP_ASPECT_RATIO    4
>> +
>>   /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
>>   struct drm_set_client_cap {
>>   	__u64 capability;
>> -- 
>> 2.7.4
Ville Syrjala April 23, 2018, 10:11 a.m. UTC | #3
On Mon, Apr 23, 2018 at 10:43:47AM +0530, Nautiyal, Ankit K wrote:
> 
> 
> On 4/20/2018 7:37 PM, Ville Syrjälä wrote:
> > On Fri, Apr 20, 2018 at 07:01:46PM +0530, Nautiyal, Ankit K wrote:
> >> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>
> >> To enable aspect-ratio support in DRM, blindly exposing the aspect
> >> ratio information along with mode, can break things in existing
> >> user-spaces which have no intention or support to use this aspect
> >> ratio information.
> >>
> >> To avoid this, a new drm client cap is required to enable a
> >> user-space to advertise if it supports modes with aspect-ratio. Based
> >> on this cap value, the kernel will take a call on exposing the aspect
> >> ratio info in modes or not.
> >>
> >> This patch adds the client cap for aspect-ratio.
> >>
> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Cc: Shashank Sharma <shashank.sharma@intel.com>
> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>
> >> V3: rebase
> >> V4: As suggested by Marteen Lankhorst modified the commit message
> >>      explaining the need to use the DRM cap for aspect-ratio. Also,
> >>      tweaked the comment lines in the code for better understanding and
> >>      clarity, as recommended by Shashank Sharma.
> >> V5: rebase
> >> V6: rebase
> >> V7: rebase
> >> V8: rebase
> >> V9: rebase
> >> V10: added comment explaining that no userspace breaks on aspect-ratio
> >>       mode bits.
> >>
> >> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_ioctl.c | 9 +++++++++
> >>   include/drm/drm_file.h      | 8 ++++++++
> >>   include/uapi/drm/drm.h      | 7 +++++++
> >>   3 files changed, 24 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> >> index af78291..39c8eab 100644
> >> --- a/drivers/gpu/drm/drm_ioctl.c
> >> +++ b/drivers/gpu/drm/drm_ioctl.c
> >> @@ -325,6 +325,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
> >>   		file_priv->atomic = req->value;
> >>   		file_priv->universal_planes = req->value;
> >>   		break;
> >> +	case DRM_CLIENT_CAP_ASPECT_RATIO:
> >> +		if (req->value > 1)
> >> +			return -EINVAL;
> >> +	/*
> >> +	 * No Atomic userspace blows up on aspect ratio mode bits. Checked in
> >> +	 * wayland/weston, xserver, and hardware-composer modeset paths.
> >> +	 */
> > Bogus indentation.
> 
> Thanks to point that out, will fix this.
> 
> > Also where's the aspect_ratio_allowed handling for the atomic cap?
> > Or did we decide against it after all?
> 
> As discussed, aspect ratio is handled in the atomic modeset path, where 
> in the modeset requests with aspect-ratios
> are rejected, if the aspect-ratio cap not set.

That is not what we discussed on irc. What Daniel was suggesting is
always enabling the aspect ratio cap for atomic clients, just as we
already enable the univerals planes cap for atomic clients.
Ville Syrjala April 26, 2018, 2:23 p.m. UTC | #4
On Mon, Apr 23, 2018 at 01:11:25PM +0300, Ville Syrjälä wrote:
> On Mon, Apr 23, 2018 at 10:43:47AM +0530, Nautiyal, Ankit K wrote:
> > 
> > 
> > On 4/20/2018 7:37 PM, Ville Syrjälä wrote:
> > > On Fri, Apr 20, 2018 at 07:01:46PM +0530, Nautiyal, Ankit K wrote:
> > >> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > >>
> > >> To enable aspect-ratio support in DRM, blindly exposing the aspect
> > >> ratio information along with mode, can break things in existing
> > >> user-spaces which have no intention or support to use this aspect
> > >> ratio information.
> > >>
> > >> To avoid this, a new drm client cap is required to enable a
> > >> user-space to advertise if it supports modes with aspect-ratio. Based
> > >> on this cap value, the kernel will take a call on exposing the aspect
> > >> ratio info in modes or not.
> > >>
> > >> This patch adds the client cap for aspect-ratio.
> > >>
> > >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > >> Cc: Shashank Sharma <shashank.sharma@intel.com>
> > >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > >>
> > >> V3: rebase
> > >> V4: As suggested by Marteen Lankhorst modified the commit message
> > >>      explaining the need to use the DRM cap for aspect-ratio. Also,
> > >>      tweaked the comment lines in the code for better understanding and
> > >>      clarity, as recommended by Shashank Sharma.
> > >> V5: rebase
> > >> V6: rebase
> > >> V7: rebase
> > >> V8: rebase
> > >> V9: rebase
> > >> V10: added comment explaining that no userspace breaks on aspect-ratio
> > >>       mode bits.
> > >>
> > >> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> > >> ---
> > >>   drivers/gpu/drm/drm_ioctl.c | 9 +++++++++
> > >>   include/drm/drm_file.h      | 8 ++++++++
> > >>   include/uapi/drm/drm.h      | 7 +++++++
> > >>   3 files changed, 24 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > >> index af78291..39c8eab 100644
> > >> --- a/drivers/gpu/drm/drm_ioctl.c
> > >> +++ b/drivers/gpu/drm/drm_ioctl.c
> > >> @@ -325,6 +325,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
> > >>   		file_priv->atomic = req->value;
> > >>   		file_priv->universal_planes = req->value;
> > >>   		break;
> > >> +	case DRM_CLIENT_CAP_ASPECT_RATIO:
> > >> +		if (req->value > 1)
> > >> +			return -EINVAL;
> > >> +	/*
> > >> +	 * No Atomic userspace blows up on aspect ratio mode bits. Checked in
> > >> +	 * wayland/weston, xserver, and hardware-composer modeset paths.
> > >> +	 */
> > > Bogus indentation.
> > 
> > Thanks to point that out, will fix this.
> > 
> > > Also where's the aspect_ratio_allowed handling for the atomic cap?
> > > Or did we decide against it after all?
> > 
> > As discussed, aspect ratio is handled in the atomic modeset path, where 
> > in the modeset requests with aspect-ratios
> > are rejected, if the aspect-ratio cap not set.
> 
> That is not what we discussed on irc. What Daniel was suggesting is
> always enabling the aspect ratio cap for atomic clients, just as we
> already enable the univerals planes cap for atomic clients.

And to make sure we're on the same page finally

@@ -320,14 +320,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
        case DRM_CLIENT_CAP_ATOMIC:
                if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
                        return -EINVAL;
                if (req->value > 1)
                        return -EINVAL;
                file_priv->atomic = req->value;
                file_priv->universal_planes = req->value;
+               file_priv->aspect_ratio_allowed = req->value;
                break;
        default:
                return -EINVAL;
        }
 
        return 0;
 }

is what we're talking about here.
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index af78291..39c8eab 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -325,6 +325,15 @@  drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 		file_priv->atomic = req->value;
 		file_priv->universal_planes = req->value;
 		break;
+	case DRM_CLIENT_CAP_ASPECT_RATIO:
+		if (req->value > 1)
+			return -EINVAL;
+	/*
+	 * No Atomic userspace blows up on aspect ratio mode bits. Checked in
+	 * wayland/weston, xserver, and hardware-composer modeset paths.
+	 */
+		file_priv->aspect_ratio_allowed = req->value;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 5176c37..02b7dde 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -182,6 +182,14 @@  struct drm_file {
 	unsigned atomic:1;
 
 	/**
+	 * @aspect_ratio_allowed:
+	 *
+	 * True, if client can handle picture aspect ratios, and has requested
+	 * to pass this information along with the mode.
+	 */
+	unsigned aspect_ratio_allowed:1;
+
+	/**
 	 * @is_master:
 	 *
 	 * This client is the creator of @master. Protected by struct
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 6fdff59..9c660e1 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -680,6 +680,13 @@  struct drm_get_cap {
  */
 #define DRM_CLIENT_CAP_ATOMIC	3
 
+/**
+ * DRM_CLIENT_CAP_ASPECT_RATIO
+ *
+ * If set to 1, the DRM core will provide aspect ratio information in modes.
+ */
+#define DRM_CLIENT_CAP_ASPECT_RATIO    4
+
 /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
 struct drm_set_client_cap {
 	__u64 capability;