| Message ID | 1524231111-17440-7-git-send-email-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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 --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;