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

2018-07-03 Thread Nautiyal, Ankit K 

Hi Shashank,

Thanks for the detailed explanation about the relationship between the 
aspect-ratio, CEA modes and VIC codes.


I will capture these and try to add this as part of man pages for a 
better understanding.


Regards,

Ankit

On 7/3/2018 4:05 PM, Sharma, Shashank wrote:

Regards

Shashank


On 7/3/2018 3:26 PM, Pekka Paalanen wrote:

On Tue, 3 Jul 2018 14:42:06 +0530
"Sharma, Shashank"  wrote:


Hello Pekka,

Thanks for your attention and code review of the patch series.
Ankit will respond on rest of the review comments, I would take one
related to mode with implicit aspect ratio information.
Please find my comment inline.

Regards
Shashank
On 6/28/2018 7:09 PM, Pekka Paalanen wrote:

On Sun, 20 May 2018 18:33:01 +0530
"Nautiyal, Ankit K"  wrote:

From: Ankit Nautiyal 

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 .
- 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 

Acked-by: Pekka Paalanen  (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/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@RR" "Resolution as above and 
refresh-rate in Hertz"
+.BR "WIDTHxHEIGHT@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.

Since these are specific to the DRM-backend, they would be more 
logical

in weston-drm.man.

+
+e.g. 720x576@50 4:3, 1920x1080@60 16:9, 2560x1080@60 64:27, 
4096x2160@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

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?

Yes there is an actual difference, let me try to explain this.
For this, we should first understand difference between CEA-modes Vs
non-CEA-modes. As you might already know, CEA defines timings of a 
video

mode, which is considered as standard for HDMI certification and
compliance testing. CEA defines each and every parameter of a video
mode, like h/vactive, h/vfront, h/vback including aspect ratio

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

2018-07-03 Thread Pekka Paalanen 
On Tue, 3 Jul 2018 15:56:10 +0530
"Nautiyal, Ankit K"  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"  wrote:
> >  
> >> From: Ankit Nautiyal
> >>
> >> 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 .
> >> - 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
> >>
> >> Acked-by: Pekka Paalanen  (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
> >

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

2018-07-03 Thread Sharma, Shashank 

Regards

Shashank


On 7/3/2018 3:26 PM, Pekka Paalanen wrote:

On Tue, 3 Jul 2018 14:42:06 +0530
"Sharma, Shashank"  wrote:


Hello Pekka,

Thanks for your attention and code review of the patch series.
Ankit will respond on rest of the review comments, I would take one
related to mode with implicit aspect ratio information.
Please find my comment inline.

Regards
Shashank
On 6/28/2018 7:09 PM, Pekka Paalanen wrote:

On Sun, 20 May 2018 18:33:01 +0530
"Nautiyal, Ankit K"  wrote:
  

From: Ankit Nautiyal 

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 .
- 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 

Acked-by: Pekka Paalanen  (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/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@RR" "Resolution as above and refresh-rate in 
Hertz"
+.BR "WIDTHxHEIGHT@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.

Since these are specific to the DRM-backend, they would be more logical
in weston-drm.man.
  

+
+e.g. 720x576@50 4:3, 1920x1080@60 16:9, 2560x1080@60 64:27, 4096x2160@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

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?

Yes there is an actual difference, let me try to explain this.
For this, we should first understand difference between CEA-modes Vs
non-CEA-modes. As you might already know, CEA defines timings of a video
mode, which is considered as standard for HDMI certification and
compliance testing. CEA defines each and every parameter of a video
mode, like h/vactive, h/vfront, h/vback including aspect ratio
information. This unique videmode is given a Video Identification Code
(VIC) which is unique. For example, VIC=4 is 1280x720@60 aspect 16:9.

Hi,

this was new to me, thank you for explaining.


Now, the videomode would be considered a CEA mode, only if it contains
aspect ratio information. In other

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

2018-07-03 Thread Nautiyal, Ankit K 

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"  wrote:


From: Ankit Nautiyal

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 .
- 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

Acked-by: Pekka Paalanen  (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.

I didn't see the aspect ratio bits in libdrm master yet, I guess it
should be ok to re-import the headers from kernel to libdrm by now.


diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 287431eb..e1d79e49 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -73,6 +73,10 @@
  #define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
  #endif
  
+#ifndef DRM_CLIENT_CAP_ASPECT_RATIO

+#define DRM_CLIENT_CAP_ASPECT_RATIO4
+#endif
+
  #ifndef DRM_CAP_CURSOR_WIDTH
  #define DRM_CAP_CURSOR_WIDTH 0x8
  #endif
@@ -272,6 +276,8 @@ struct drm_backend {
uint32_t pageflip_timeout;
  
  	bool shutting_down;

+
+   bool aspect_ratio_supported;
  };
  
  struct drm_mode {

@@ -3049,6 +3055,19 @@ err:
drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
  }
  
+/*

+ * Get the aspect-ratio from drmModeModeInfo mode flags.
+ *
+ * @param drm_mode_flags- flags from drmModeModeInfo structure.
+ * @returns aspect-ratio as encoded in enum 'weston_mode_aspect_ratio'.
+ */
+static uint8_t
+drm_to_weston_mode_aspect_ratio(uint32_t drm_mode_flags)
+{
+   return (drm_mode_flags & DRM_MODE_FLAG_PIC_AR_MASK) >>
+   DRM_MODE_FLAG_PIC_AR_BITS_POS;
+}
+
  static void
  drm_assign_planes(struct weston_output *output_base, void *repaint_data)
  {
@@ -3179,25 +3198,44 @@ drm_assign_planes(struct weston_output *output_base, 
void *repaint_data)
  static struct drm_mode *
  choose_mode (struct drm_output *output, struct weston_mode *target_mode)
  {
-   struct drm_mode *tmp_mode = NULL, *mode;
+   struct drm_mode *tmp_mode = NULL, *mode_fall_back = NULL, *mode;
+   uint8_t src_aspect = 0;
+   uint8_t target_aspect = 0;
+   struct drm_backend *b;
  
+	b = to_drm_backend(output->base.compositor);

+   target_aspect = target_mode->aspect_ratio;
+   src_aspect = output->base.current_mode->aspect_ratio;
if (output->base.current_mode->width == target_mode->width &&
output->base.current_mode->height == target_mode->height &&
(output->base.current_mode->refresh == target_mode->refresh ||
-target_mode->refresh == 0))
-   return to_drm_mode(output->base.current_mode);
+target_mode->refresh == 0)) {
+   if (!b->aspect_ratio_supported || src_aspect == target_aspect)
+   return (struct drm_mode *)output->base.current_mode;

Please keep to_drm_mode() instead of

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

2018-07-03 Thread Pekka Paalanen 
On Tue, 3 Jul 2018 14:42:06 +0530
"Sharma, Shashank"  wrote:

> Hello Pekka,
> 
> Thanks for your attention and code review of the patch series.
> Ankit will respond on rest of the review comments, I would take one 
> related to mode with implicit aspect ratio information.
> Please find my comment inline.
> 
> Regards
> Shashank
> On 6/28/2018 7:09 PM, Pekka Paalanen wrote:
> > On Sun, 20 May 2018 18:33:01 +0530
> > "Nautiyal, Ankit K"  wrote:
> >  
> >> From: Ankit Nautiyal 
> >>
> >> 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 .
> >> - 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 
> >>
> >> Acked-by: Pekka Paalanen  (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/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@RR" "Resolution as above and refresh-rate 
> >> in Hertz"
> >> +.BR "WIDTHxHEIGHT@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.
> >
> > Since these are specific to the DRM-backend, they would be more logical
> > in weston-drm.man.
> >  
> >> +
> >> +e.g. 720x576@50 4:3, 1920x1080@60 16:9, 2560x1080@60 64:27, 4096x2160@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
> >
> > 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?  
> Yes there is an actual difference, let me try to explain this.
> For this, we should first understand difference between CEA-modes Vs 
> non-CEA-modes. As you might already know, CEA defines timings of a video 
> mode, which is considered as standard for HDMI certification and 
> compliance testing. CEA defines each and every

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

2018-07-03 Thread Sharma, Shashank 

Hello Pekka,

Thanks for your attention and code review of the patch series.
Ankit will respond on rest of the review comments, I would take one 
related to mode with implicit aspect ratio information.

Please find my comment inline.

Regards
Shashank
On 6/28/2018 7:09 PM, Pekka Paalanen wrote:

On Sun, 20 May 2018 18:33:01 +0530
"Nautiyal, Ankit K"  wrote:


From: Ankit Nautiyal 

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 .
- 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 

Acked-by: Pekka Paalanen  (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.

I didn't see the aspect ratio bits in libdrm master yet, I guess it
should be ok to re-import the headers from kernel to libdrm by now.


diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 287431eb..e1d79e49 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -73,6 +73,10 @@
  #define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
  #endif
  
+#ifndef DRM_CLIENT_CAP_ASPECT_RATIO

+#define DRM_CLIENT_CAP_ASPECT_RATIO4
+#endif
+
  #ifndef DRM_CAP_CURSOR_WIDTH
  #define DRM_CAP_CURSOR_WIDTH 0x8
  #endif
@@ -272,6 +276,8 @@ struct drm_backend {
uint32_t pageflip_timeout;
  
  	bool shutting_down;

+
+   bool aspect_ratio_supported;
  };
  
  struct drm_mode {

@@ -3049,6 +3055,19 @@ err:
drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
  }
  
+/*

+ * Get the aspect-ratio from drmModeModeInfo mode flags.
+ *
+ * @param drm_mode_flags- flags from drmModeModeInfo structure.
+ * @returns aspect-ratio as encoded in enum 'weston_mode_aspect_ratio'.
+ */
+static uint8_t
+drm_to_weston_mode_aspect_ratio(uint32_t drm_mode_flags)
+{
+   return (drm_mode_flags & DRM_MODE_FLAG_PIC_AR_MASK) >>
+   DRM_MODE_FLAG_PIC_AR_BITS_POS;
+}
+
  static void
  drm_assign_planes(struct weston_output *output_base, void *repaint_data)
  {
@@ -3179,25 +3198,44 @@ drm_assign_planes(struct weston_output *output_base, 
void *repaint_data)
  static struct drm_mode *
  choose_mode (struct drm_output *output, struct weston_mode *target_mode)
  {
-   struct drm_mode *tmp_mode = NULL, *mode;
+   struct drm_mode *tmp_mode = NULL, *mode_fall_back = NULL, *mode;
+   uint8_t src_aspect = 0;
+   uint8_t target_aspect = 0;
+   struct drm_backend *b;
  
+	b = to_drm_backend(output->base.compositor);

+   target_aspect = target_mode->aspect_ratio;
+   src_aspect = output->base.current_mode->aspect_ratio;
if (output->base.current_mode->width == target_mode->width &&
output->base.current_mode->height == target_mode->height &&
(output->base.current_mode->refresh == target_mode->refresh ||
-target_mode->refresh == 0))
-   return to_drm_mode(output->base.current_mode);
+target_mode->refresh == 0)) {
+   if (!b->aspect_ratio_supported || src_aspect == target_aspect)
+   return (struct drm_mode *)output->base.current_mode;

Please keep to_drm_mode() instead of converting it back to a cast.


+   }
  
  	wl_list_for_each(mode, >base.mode_list,

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

2018-06-28 Thread Pekka Paalanen 
On Sun, 20 May 2018 18:33:01 +0530
"Nautiyal, Ankit K"  wrote:

> From: Ankit Nautiyal 
> 
> 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 .
> - 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 
> 
> Acked-by: Pekka Paalanen  (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.

I didn't see the aspect ratio bits in libdrm master yet, I guess it
should be ok to re-import the headers from kernel to libdrm by now.

> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 287431eb..e1d79e49 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -73,6 +73,10 @@
>  #define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
>  #endif
>  
> +#ifndef DRM_CLIENT_CAP_ASPECT_RATIO
> +#define DRM_CLIENT_CAP_ASPECT_RATIO  4
> +#endif
> +
>  #ifndef DRM_CAP_CURSOR_WIDTH
>  #define DRM_CAP_CURSOR_WIDTH 0x8
>  #endif
> @@ -272,6 +276,8 @@ struct drm_backend {
>   uint32_t pageflip_timeout;
>  
>   bool shutting_down;
> +
> + bool aspect_ratio_supported;
>  };
>  
>  struct drm_mode {
> @@ -3049,6 +3055,19 @@ err:
>   drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
>  }
>  
> +/*
> + * Get the aspect-ratio from drmModeModeInfo mode flags.
> + *
> + * @param drm_mode_flags- flags from drmModeModeInfo structure.
> + * @returns aspect-ratio as encoded in enum 'weston_mode_aspect_ratio'.
> + */
> +static uint8_t
> +drm_to_weston_mode_aspect_ratio(uint32_t drm_mode_flags)
> +{
> + return (drm_mode_flags & DRM_MODE_FLAG_PIC_AR_MASK) >>
> + DRM_MODE_FLAG_PIC_AR_BITS_POS;
> +}
> +
>  static void
>  drm_assign_planes(struct weston_output *output_base, void *repaint_data)
>  {
> @@ -3179,25 +3198,44 @@ drm_assign_planes(struct weston_output *output_base, 
> void *repaint_data)
>  static struct drm_mode *
>  choose_mode (struct drm_output *output, struct weston_mode *target_mode)
>  {
> - struct drm_mode *tmp_mode = NULL, *mode;
> + struct drm_mode *tmp_mode = NULL, *mode_fall_back = NULL, *mode;
> + uint8_t src_aspect = 0;
> + uint8_t target_aspect = 0;
> + struct drm_backend *b;
>  
> + b = to_drm_backend(output->base.compositor);
> + target_aspect = target_mode->aspect_ratio;
> + src_aspect = output->base.current_mode->aspect_ratio;
>   if (output->base.current_mode->width == target_mode->width &&
>   output->base.current_mode->height == target_mode->height &&
>   (output->base.current_mode->refresh == target_mode->refresh ||
> -  target_mode->refresh == 0))
> - return to_drm_mode(output->base.current_mode);
> +  target_mode->refresh == 0)) {
> + if (!b->aspect_ratio_supported || src_aspect == target_aspect)
> + return (struct drm_mode *)output->base.current_mode;

Please keep to_drm_mode() instead of converting it back to a cast.

> + }
>  
>   wl_list_for_each(mode, >base.mode_list, base.link) {
> + uint32_t flags = mode->mode_info.flags;
> +
> + src_aspect = drm_to_weston_mode_aspect_ratio(flags);

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

2018-05-20 Thread Nautiyal, Ankit K 
From: Ankit Nautiyal 

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 .
- 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 

Acked-by: Pekka Paalanen  (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(-)

diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 287431eb..e1d79e49 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -73,6 +73,10 @@
 #define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
 #endif
 
+#ifndef DRM_CLIENT_CAP_ASPECT_RATIO
+#define DRM_CLIENT_CAP_ASPECT_RATIO4
+#endif
+
 #ifndef DRM_CAP_CURSOR_WIDTH
 #define DRM_CAP_CURSOR_WIDTH 0x8
 #endif
@@ -272,6 +276,8 @@ struct drm_backend {
uint32_t pageflip_timeout;
 
bool shutting_down;
+
+   bool aspect_ratio_supported;
 };
 
 struct drm_mode {
@@ -3049,6 +3055,19 @@ err:
drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
 }
 
+/*
+ * Get the aspect-ratio from drmModeModeInfo mode flags.
+ *
+ * @param drm_mode_flags- flags from drmModeModeInfo structure.
+ * @returns aspect-ratio as encoded in enum 'weston_mode_aspect_ratio'.
+ */
+static uint8_t
+drm_to_weston_mode_aspect_ratio(uint32_t drm_mode_flags)
+{
+   return (drm_mode_flags & DRM_MODE_FLAG_PIC_AR_MASK) >>
+   DRM_MODE_FLAG_PIC_AR_BITS_POS;
+}
+
 static void
 drm_assign_planes(struct weston_output *output_base, void *repaint_data)
 {
@@ -3179,25 +3198,44 @@ drm_assign_planes(struct weston_output *output_base, 
void *repaint_data)
 static struct drm_mode *
 choose_mode (struct drm_output *output, struct weston_mode *target_mode)
 {
-   struct drm_mode *tmp_mode = NULL, *mode;
+   struct drm_mode *tmp_mode = NULL, *mode_fall_back = NULL, *mode;
+   uint8_t src_aspect = 0;
+   uint8_t target_aspect = 0;
+   struct drm_backend *b;
 
+   b = to_drm_backend(output->base.compositor);
+   target_aspect = target_mode->aspect_ratio;
+   src_aspect = output->base.current_mode->aspect_ratio;
if (output->base.current_mode->width == target_mode->width &&
output->base.current_mode->height == target_mode->height &&
(output->base.current_mode->refresh == target_mode->refresh ||
-target_mode->refresh == 0))
-   return to_drm_mode(output->base.current_mode);
+target_mode->refresh == 0)) {
+   if (!b->aspect_ratio_supported || src_aspect == target_aspect)
+   return (struct drm_mode *)output->base.current_mode;
+   }
 
wl_list_for_each(mode, >base.mode_list, base.link) {
+   uint32_t flags = mode->mode_info.flags;
+
+   src_aspect = drm_to_weston_mode_aspect_ratio(flags);
if (mode->mode_info.hdisplay == target_mode->width &&
mode->mode_info.vdisplay == target_mode->height) {
if (mode->base.refresh == target_mode->refresh ||
target_mode->refresh == 0) {
-   return mode;
-   } else if (!tmp_mode)
+   if (!b->aspect_ratio_supported ||
+   src_aspect == target_aspect)
+   return mode;
+   else if (!mode_fall_back)
+