| Message ID | 1515738096-16892-5-git-send-email-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Regards Shashank On 1/12/2018 11:51 AM, Nautiyal, Ankit K wrote: > From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > A new drm client cap is required to enable 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 information 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 > --- > drivers/gpu/drm/drm_ioctl.c | 5 +++++ > include/drm/drm_file.h | 7 +++++++ > include/uapi/drm/drm.h | 7 +++++++ > 3 files changed, 19 insertions(+) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 4aafe48..e092550 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -325,6 +325,11 @@ 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; > + 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 0e0c868..6e0e435 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -182,6 +182,13 @@ struct drm_file { > unsigned atomic:1; > > /** > + * @aspect_ratio_allowed: > + * > + * True if client has asked to expose the aspect-ratio flags with mode. Minor reword, may be "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..fe5f531 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 expose aspect ratio flags to userspace. > + */ Again "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; With these optional review comments, please feel free to use Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
Op 12-01-18 om 07:21 schreef Nautiyal, Ankit K: > From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > A new drm client cap is required to enable 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 information 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> What's missing from the commit message is why this needs to be a flag that userspace needs to enable, instead of something that only counts as an informative query. I gathered from danvet that blindly exposing things breaks existing userspace, so could that information be added to the commit? Also missing IGT tests, would be nice we at least get some verification things work as expected. ~Maarten
Hi Marteen, Thanks for the review comments. Please find my comments in-line. On Thursday 18 January 2018 03:46 PM, Maarten Lankhorst wrote: > Op 12-01-18 om 07:21 schreef Nautiyal, Ankit K: >> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> >> A new drm client cap is required to enable 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 information 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> > What's missing from the commit message is why this needs to be a flag that userspace needs to enable, > instead of something that only counts as an informative query. > > I gathered from danvet that blindly exposing things breaks existing userspace, so could that information be added to the commit? Agreed. This information indeed brings out the real reason for the patch. Will add the details in the commit message in the next patch-set. > > Also missing IGT tests, would be nice we at least get some verification things work as expected. Currently this is being tested with the corresponding userspace changes in weston compositor, but a simple IGT on the same lines of kms_3d can be prepared. Will work on that and include the reference in this series. I hope in the meanwhile, the other patches of the series would be continued to be reviewed. > > ~Maarten Thanks & regards, Ankit
On Thu, Jan 18, 2018 at 09:11:19PM +0530, ankit.k.nautiyal@intel.com wrote: > Hi Marteen, > > Thanks for the review comments. Please find my comments in-line. > > > On Thursday 18 January 2018 03:46 PM, Maarten Lankhorst wrote: > > Op 12-01-18 om 07:21 schreef Nautiyal, Ankit K: > >> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >> > >> A new drm client cap is required to enable 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 information 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> > > What's missing from the commit message is why this needs to be a flag that userspace needs to enable, > > instead of something that only counts as an informative query. > > > > I gathered from danvet that blindly exposing things breaks existing userspace, so could that information be added to the commit? > > Agreed. This information indeed brings out the real reason for the patch. > Will add the details in the commit message in the next patch-set. > > > > > Also missing IGT tests, would be nice we at least get some verification things work as expected. > > Currently this is being tested with the corresponding userspace changes > in weston compositor, but a simple IGT on the same lines of kms_3d > can be prepared. A separate test to check that the different aspect ratio modes are correctly enumerated, and filtered out when the cap is not set seems like a good idea. As for actaully testing different aspect ratio modes, maybe just intergrate that into testdisplay?
On 1/18/2018 9:34 PM, Ville Syrjälä wrote: > On Thu, Jan 18, 2018 at 09:11:19PM +0530, ankit.k.nautiyal@intel.com wrote: >> Hi Marteen, >> >> Thanks for the review comments. Please find my comments in-line. >> >> >> On Thursday 18 January 2018 03:46 PM, Maarten Lankhorst wrote: >>> Op 12-01-18 om 07:21 schreef Nautiyal, Ankit K: >>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>> >>>> A new drm client cap is required to enable 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 information 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> >>> What's missing from the commit message is why this needs to be a flag that userspace needs to enable, >>> instead of something that only counts as an informative query. >>> >>> I gathered from danvet that blindly exposing things breaks existing userspace, so could that information be added to the commit? >> Agreed. This information indeed brings out the real reason for the patch. >> Will add the details in the commit message in the next patch-set. >> >>> Also missing IGT tests, would be nice we at least get some verification things work as expected. >> Currently this is being tested with the corresponding userspace changes >> in weston compositor, but a simple IGT on the same lines of kms_3d >> can be prepared. > A separate test to check that the different aspect ratio modes are > correctly enumerated, and filtered out when the cap is not set seems > like a good idea. > > As for actaully testing different aspect ratio modes, maybe just > intergrate that into testdisplay? Alright. Will modify the testdisplay to have a "test_aspect_ratio_modes", which will just set the aspect ratio modes from the connector modelist, to verify the basic functionality. kms_aspect_ratio to do detailed testing will be taken later.
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 4aafe48..e092550 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -325,6 +325,11 @@ 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; + 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 0e0c868..6e0e435 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -182,6 +182,13 @@ struct drm_file { unsigned atomic:1; /** + * @aspect_ratio_allowed: + * + * True if client has asked to expose the aspect-ratio flags with 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..fe5f531 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 expose aspect ratio flags to userspace. + */ +#define DRM_CLIENT_CAP_ASPECT_RATIO 4 + /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ struct drm_set_client_cap { __u64 capability;