[PATCH v5] compositor-drm: Add aspect-ratio parsing support

Tue Jul 3 10:54:47 UTC 2018

On Tue, 3 Jul 2018 15:56:10 +0530
"Nautiyal, Ankit K" <ankit.k.nautiyal at intel.com> wrote:

> Hi Pekka,
> 
> Thanks for the review and the comments.
> 
> I agree with all of the suggestions, will send a patch in a day or two.
> 
> Discussion about the CEA with aspect ratio and and Non- CEA modes is 
> currently in progress in another thread,
> 
> lets close it in the other thread itself.
> 
> Please find my responses inline for the remaining comments:
> 
> 
> On 6/28/2018 7:09 PM, Pekka Paalanen wrote:
> > On Sun, 20 May 2018 18:33:01 +0530
> > "Nautiyal, Ankit K"<ankit.k.nautiyal at intel.com>  wrote:
> >  
> >> From: Ankit Nautiyal<ankit.k.nautiyal at intel.com>
> >>
> >> The flag bits 19-22 of the connector modes, provide the aspect-ratio
> >> information. This information can be stored in flags bits of the
> >> weston mode structure, so that it can used for setting a mode with a
> >> particular aspect-ratio.
> >> Currently, DRM layer supports aspect-ratio with atomic-modesetting by
> >> default. For legacy modeset path, the user-space needs to set the
> >> drm client cap for aspect-ratio, if it wants aspect-ratio information
> >> in modes.
> >>
> >> This patch:
> >> - preserves aspect-ratio flags from kernel video modes and
> >>    accommodates it in wayland mode.
> >> - uses aspect-ratio to pick the appropriate mode during modeset.
> >> - changes the mode format in configuration file weston.ini to
> >>    accommodate aspect-ratio information as:
> >>    WIDTHxHEIGHT at REFRESH-RATE ASPECT-RATIO
> >>    The aspect-ratio should be given as <length:breadth>.
> >> - modifies the man pages to explain the usage of different mode format
> >>    configurations in weston.ini.
> >>
> >> v2: As per recommendation from Pekka Paalanen, Quentin Glidic,
> >> Daniel Stone, dropped the aspect-ratio info from wayland protocol,
> >> thereby avoiding exposure of aspect-ratio to the client.
> >>
> >> v3: As suggested by Pekka Paalanen, added aspect_ratio field to store
> >> aspect-ratio information from the drm. Also added drm client
> >> capability for aspect-ratio, as recommended by Daniel Vetter.
> >>
> >> v4: Minor modifications and fixes as suggested by Pekka Paalanen.
> >>
> >> v5: Rebased, fixed some styling issues, and added aspect-ratio
> >> information while printing weston_modes.
> >> Signed-off-by: Ankit Nautiyal<ankit.k.nautiyal at intel.com>
> >>
> >> Acked-by: Pekka Paalanen<pekka.paalanen at collabora.co.uk>  (v4)
> >> ---
> >>   libweston/compositor-drm.c | 115 +++++++++++++++++++++++++++++++++----
> >>   libweston/compositor-drm.h |  21 +++++++
> >>   libweston/compositor.h     |   1 +
> >>   man/weston.ini.man         |  13 +++--
> >>   4 files changed, 136 insertions(+), 14 deletions(-)  
> > Hi Ankit,
> >
> > it's been a long while since I last looked at this, so forgive me if I
> > go in circles with my review comments. I see the aspect ratio bits are
> > in Linus' RC kernel. I would like to get this merged for the next
> > release which basically means during the next week if you can.

> >> diff --git a/libweston/compositor-drm.h b/libweston/compositor-drm.h
> >> index 68f93eab..61ad44dc 100644
> >> --- a/libweston/compositor-drm.h
> >> +++ b/libweston/compositor-drm.h
> >> @@ -52,6 +52,27 @@ enum weston_drm_backend_output_mode {
> >>   	WESTON_DRM_BACKEND_OUTPUT_PREFERRED,
> >>   };
> >>   
> >> +/**
> >> + * aspect ratio info taken from the drmModeModeInfo flag bits 19-22,
> >> + * which should be used to fill the aspect ratio field in weston_mode.
> >> + */
> >> +#define DRM_MODE_FLAG_PIC_AR_BITS_POS	19
> >> +#ifndef DRM_MODE_FLAG_PIC_AR_MASK
> >> +#define DRM_MODE_FLAG_PIC_AR_MASK (0xF << DRM_MODE_FLAG_PIC_AR_BITS_POS)
> >> +#endif
> >> +  
> > The above is ok in compositor-drm.h, but it could be in
> > compositor-drm.c instead, since it's not used anywhere else so far.  
> 
> Noted. Will do in next patch-set.
> 
> > The below could be in compositor.h instead so that...
> >  
> >> +enum weston_mode_aspect_ratio {
> >> +	/** The picture aspect ratio values, for the aspect_ratio field of
> >> +	 *  weston_mode. The values here, are taken from
> >> +	 *  DRM_MODE_PICTURE_ASPECT_* from drm_mode.h.
> >> +	 */
> >> +	WESTON_MODE_PIC_AR_NONE = 0,	/* DRM_MODE_PICTURE_ASPECT_NONE */
> >> +	WESTON_MODE_PIC_AR_4_3 = 1,	/* DRM_MODE_PICTURE_ASPECT_4_3 */
> >> +	WESTON_MODE_PIC_AR_16_9 = 2,	/* DRM_MODE_PICTURE_ASPECT_16_9 */
> >> +	WESTON_MODE_PIC_AR_64_27 = 3,	/* DRM_MODE_PICTURE_ASPECT_64_27 */
> >> +	WESTON_MODE_PIC_AR_256_135 = 4,	/* DRM_MODE_PICTURE_ASPECT_256_135*/
> >> +};
> >> +
> >>   #define WESTON_DRM_OUTPUT_API_NAME "weston_drm_output_api_v1"
> >>   
> >>   struct weston_drm_output_api {
> >> diff --git a/libweston/compositor.h b/libweston/compositor.h
> >> index 47337d8a..3dedbbfe 100644
> >> --- a/libweston/compositor.h
> >> +++ b/libweston/compositor.h
> >> @@ -95,6 +95,7 @@ enum weston_led {
> >>   
> >>   struct weston_mode {
> >>   	uint32_t flags;
> >> +	uint8_t aspect_ratio;  
> > ...the type of this could be enum weston_mode_aspect_ratio. None of it
> > actually depends on libdrm, so compositor.h would be fine. The docs are
> > good.
> >
> > First I thought that aspect_ratio would belong in drm_mode rather than
> > weston_mode, but thinking ahead, we should get the mode choosing out of
> > libweston and into the compositor, so it makes sense to have
> > aspect_ratio in weston_mode for the future.  
> 
> Yes we can do that. Is there plan to move the choose_mode...( ) in 
> compositor ?
> What about other non-drm compositors. will they be using the 
> weston_mode::aspect_ratio?

Hi,

some time in the future I would like to see the compositor (main.c) in
charge of picking the video mode blob. Libweston would likely offer
helpers for comparing and matching modes, and some DRM-backend specific
API for KMS modes. How exactly that would turn out I don't know yet.

Aspect ratio (literally as aspect ratio, not as a VIC identifier) would
be good to expose to the compositor, so that if we add automatic aspect
ratio correction the compositor would know when it applies.

By aspect ratio correction I mean pixels from clients assumed to be
square, and to maintain image aspect ratio they would get scaled for
the output. I'm still not sure if that is a good idea and how it should
work, but right now it would seem sensible. If the monitor is
stretching the image and the compositor knows about it, it would be
good to counter-stretch windows that expect square pixels.

> >>   	int32_t width, height;
> >>   	uint32_t refresh;
> >>   	struct wl_list link;
> >> diff --git a/man/weston.ini.man b/man/weston.ini.man
> >> index f237fd60..e982cac9 100644
> >> --- a/man/weston.ini.man
> >> +++ b/man/weston.ini.man
> >> @@ -371,10 +371,15 @@ The DRM backend accepts different modes:
> >>   .PP
> >>   .RS 10
> >>   .nf
> >> -.BR "WIDTHxHEIGHT    " "Resolution size width and height in pixels"
> >> -.BR "preferred       " "Uses the preferred mode"
> >> -.BR "current         " "Uses the current crt controller mode"
> >> -.BR "off             " "Disables the output"
> >> +.BR "WIDTHxHEIGHT               " "Resolution size width and height in pixels"
> >> +.BR "WIDTHxHEIGHT at RR            " "Resolution as above and refresh-rate in Hertz"
> >> +.BR "WIDTHxHEIGHT at RR RATIO      " "Resolution as above and aspect-ratio as length:breadth"  
> > I think the length:breadth could be confusing. H:V? hor:vert? Or simply
> > A:B, assuming that aspect ratio is general knowledge.
> >
> > It would also be worth to list all the acceptable values, because there
> > are only few. One cannot give e.g. 8:6 and assume Weston figures out it
> > means 4:3. If they are explicitly listed, we could just use RATIO and
> > not split the value further.  
> Alright, we can just list the valid values for aspect-ratio.
> Instead of length:breadth we can just give RATIO.
> 
> > Since these are specific to the DRM-backend, they would be more logical
> > in weston-drm.man.  
> 
> Do you mean, to have the same information in weston-drm.man (just like 
> mode, modeline etc) , along with the weston.ini.man?
> 

Yes, and only in weston-drm.man for the parts that are application only
to the DRM-backend.

Currently the man pages are written roughly so that weston.ini.man
contains things that are not specific to a single backend, at least if
the backend has its own man-page.

> 
> >> +
> >> +e.g. 720x576 at 50 4:3, 1920x1080 at 60 16:9, 2560x1080 at 60 64:27, 4096x2160 at 60 256:135 etc.  
> > What is the point of giving the aspect ratio explicitly if the
> > resolution directly results in the same aspect ratio?
> >
> > Shouldn't the examples show cases where it actually matters, i.e. when
> > the display shows non-square pixels? E.g. 1024x768 16:9  
> These were just for the example. we can have better examples :)
> User can give 1920x1080@ 64:27, it will be applied provided if its in 
> the list of connector modes.

They do make sense if one knows about the VIC stuff. It would be good
for the man-page to explain about VIC, so that the meaning of a
seemingly redundant aspect ratio definition becomes clear.

> > The implementation also makes a difference between implicit aspect
> > ratio (that is, NONE) and explicit aspect ratio even if the two are
> > equal. Is that intentional? Is there an actual difference?
> >
> > If the kernel or the user defines an aspect ratio that is equal to the
> > resolution aspect ratio, should we convert the aspect ratio to
> > WESTON_MODE_PIC_AR_NONE?  
> Discussion continuing with Shashank in another thread.
> 
> >> +
> >> +.BR "preferred                  " "Uses the preferred mode"
> >> +.BR "current                    " "Uses the current crt controller mode"
> >> +.BR "off                        " "Disables the output"  
> > These are actually quite specific to the DRM-backend and would belong
> > more in weston-drm.man. Would be nice to move them in a separate patch
> > before the aspect ratio support if you wish.  
> 
> So shall we have two patches? first for adding aspect ratio info through 
> weston.ini in weston-drm.man,
> and second with the actual changes?

Yes, that would be the ideal patch split.

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180703/21d0f8df/attachment.sig>