[PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.

Adding new mode flags willy nilly breaks existing userspace. We need to
coordinate this better, potentially with a new client cap that only
exposes the aspect ratio flags when userspace is prepared for them
(similar to what we do with stereo 3D modes).

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Lin, Jia <lin.a.jia@intel.com>
Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
Cc: Jim Bride <jim.bride@linux.intel.com>
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c | 12 ------------
 include/uapi/drm/drm_mode.h |  6 ------
 2 files changed, 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index f64ac86deb84..725faa6409aa 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1513,12 +1513,6 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
 	case HDMI_PICTURE_ASPECT_16_9:
 		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
 		break;
-	case HDMI_PICTURE_ASPECT_64_27:
-		out->flags |= DRM_MODE_FLAG_PIC_AR_64_27;
-		break;
-	case DRM_MODE_PICTURE_ASPECT_256_135:
-		out->flags |= DRM_MODE_FLAG_PIC_AR_256_135;
-		break;
 	case HDMI_PICTURE_ASPECT_RESERVED:
 	default:
 		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
@@ -1580,12 +1574,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
 	case DRM_MODE_FLAG_PIC_AR_16_9:
 		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
 		break;
-	case DRM_MODE_FLAG_PIC_AR_64_27:
-		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27;
-		break;
-	case DRM_MODE_FLAG_PIC_AR_256_135:
-		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135;
-		break;
 	default:
 		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
 		break;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 084b50a02dc5..5c142b1387ac 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -81,8 +81,6 @@ extern "C" {
 #define DRM_MODE_PICTURE_ASPECT_NONE		0
 #define DRM_MODE_PICTURE_ASPECT_4_3		1
 #define DRM_MODE_PICTURE_ASPECT_16_9		2
-#define DRM_MODE_PICTURE_ASPECT_64_27		3
-#define DRM_MODE_PICTURE_ASPECT_256_135		4
 
 /* Aspect ratio flag bitmask (4 bits 22:19) */
 #define DRM_MODE_FLAG_PIC_AR_MASK		(0x0F<<19)
@@ -92,10 +90,6 @@ extern "C" {
 			(DRM_MODE_PICTURE_ASPECT_4_3<<19)
 #define  DRM_MODE_FLAG_PIC_AR_16_9 \
 			(DRM_MODE_PICTURE_ASPECT_16_9<<19)
-#define  DRM_MODE_FLAG_PIC_AR_64_27 \
-			(DRM_MODE_PICTURE_ASPECT_64_27<<19)
-#define  DRM_MODE_FLAG_PIC_AR_256_135 \
-			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
 
 /* DPMS flags */
 /* bit compatible with the xorg definitions. */
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] Revert "drm: Add aspect ratio parsing in DRM layer"

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This reverts commit 6dffd431e2296cda08e7e4f0242e02df1d1698cd.

Adding new mode flags willy nilly breaks existing userspace. We need to
coordinate this better, potentially with a new client cap that only
exposes the aspect ratio flags when userspace is prepared for them
(similar to what we do with stereo 3D modes).

This also broke things so that we would always send out VIC==0 in
the AVI infoframe unless the user specified an aspect ratio via
the mode flags. And the automagic RGB full vs. limited range
handling was similartly broken as the user mode would never match
any CEA mode.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Lin, Jia <lin.a.jia@intel.com>
Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
Cc: Jim Bride <jim.bride@linux.intel.com>
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 725faa6409aa..18442b5bb34c 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1001,7 +1001,6 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 	    mode1->vsync_end == mode2->vsync_end &&
 	    mode1->vtotal == mode2->vtotal &&
 	    mode1->vscan == mode2->vscan &&
-	    mode1->picture_aspect_ratio == mode2->picture_aspect_ratio &&
 	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
 	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
 		return true;
@@ -1504,21 +1503,6 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
 	out->vrefresh = in->vrefresh;
 	out->flags = in->flags;
 	out->type = in->type;
-	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
-
-	switch (in->picture_aspect_ratio) {
-	case HDMI_PICTURE_ASPECT_4_3:
-		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
-		break;
-	case HDMI_PICTURE_ASPECT_16_9:
-		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
-		break;
-	case HDMI_PICTURE_ASPECT_RESERVED:
-	default:
-		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
-		break;
-	}
-
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
 }
@@ -1564,21 +1548,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
 
-	/* Clearing picture aspect ratio bits from out flags */
-	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
-
-	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
-	case DRM_MODE_FLAG_PIC_AR_4_3:
-		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
-		break;
-	case DRM_MODE_FLAG_PIC_AR_16_9:
-		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
-		break;
-	default:
-		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
-		break;
-	}
-
 	out->status = drm_mode_validate_basic(out);
 	if (out->status != MODE_OK)
 		goto out;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 2/2] Revert "drm: Add aspect ratio parsing in DRM layer"

[Sharma, Shashank]

NACK until we get to the right reason.

-----Original Message-----
From: ville.syrjala@linux.intel.com [mailto:ville.syrjala@linux.intel.com] 
Sent: Thursday, November 3, 2016 6:02 PM
To: dri-devel@lists.freedesktop.org
Cc: Sharma, Shashank <shashank.sharma@intel.com>; Lin; Jia, Lin A <lin.a.jia@intel.com>; Sharma, Akashdeep <akashdeep.sharma@intel.com>; Jim Bride <jim.bride@linux.intel.com>; Jose Abreu <Jose.Abreu@synopsys.com>; Daniel Vetter <daniel.vetter@ffwll.ch>; Emil Velikov <emil.l.velikov@gmail.com>
Subject: [PATCH 2/2] Revert "drm: Add aspect ratio parsing in DRM layer"

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This reverts commit 6dffd431e2296cda08e7e4f0242e02df1d1698cd.

Adding new mode flags willy nilly breaks existing userspace. We need to coordinate this better, potentially with a new client cap that only exposes the aspect ratio flags when userspace is prepared for them (similar to what we do with stereo 3D modes).

This also broke things so that we would always send out VIC==0 in the AVI infoframe unless the user specified an aspect ratio via the mode flags. And the automagic RGB full vs. limited range handling was similartly broken as the user mode would never match any CEA mode.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Lin, Jia <lin.a.jia@intel.com>
Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
Cc: Jim Bride <jim.bride@linux.intel.com>
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 725faa6409aa..18442b5bb34c 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1001,7 +1001,6 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 	    mode1->vsync_end == mode2->vsync_end &&
 	    mode1->vtotal == mode2->vtotal &&
 	    mode1->vscan == mode2->vscan &&
-	    mode1->picture_aspect_ratio == mode2->picture_aspect_ratio &&
 	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
 	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
 		return true;
@@ -1504,21 +1503,6 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
 	out->vrefresh = in->vrefresh;
 	out->flags = in->flags;
 	out->type = in->type;
-	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
-
-	switch (in->picture_aspect_ratio) {
-	case HDMI_PICTURE_ASPECT_4_3:
-		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
-		break;
-	case HDMI_PICTURE_ASPECT_16_9:
-		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
-		break;
-	case HDMI_PICTURE_ASPECT_RESERVED:
-	default:
-		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
-		break;
-	}
-
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;  } @@ -1564,21 +1548,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
 
-	/* Clearing picture aspect ratio bits from out flags */
-	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
-
-	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
-	case DRM_MODE_FLAG_PIC_AR_4_3:
-		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
-		break;
-	case DRM_MODE_FLAG_PIC_AR_16_9:
-		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
-		break;
-	default:
-		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
-		break;
-	}
-
 	out->status = drm_mode_validate_basic(out);
 	if (out->status != MODE_OK)
 		goto out;
--
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Thu, Nov 03, 2016 at 03:02:53PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 03, 2016 at 12:47:39PM +0000, Sharma, Shashank wrote:
> > Hi Ville, 
> > (-dri-devel)
> > I would appreciate an internal discussion before going to dri-devel. a
> 
> Added back. Private discussions are pointless.

Now added for reals.

> 
> > 
> > What did this patch break ? 
> 
> Intel ddx. It will reject any mode with flags it doesn't know about. 
> Other ddxen like -ati or -modesetting will also suffer, but potentially
> in a different way since they don't even check the flags and instead
> just directly stuff them into the randr mode flags, which will result
> in an out of spec mode as far randr is concerned.
> 
> > This is exposed in the same way, as the 3D flags.
> 
> Not it isn't. We have a client cap for 3D flags.
> 
> > The information is already available in form of flags, all you have to do in userspace is read and print that. Its already being done in Android. 
> 
> Please stop relying on Android for testing upstream patches. Test with
> actual userspace people are using.
> 
> >  
> > NACK until we get to the right reason. 
> > 
> > Regards
> > Shashank
> > -----Original Message-----
> > From: ville.syrjala@linux.intel.com [mailto:ville.syrjala@linux.intel.com] 
> > Sent: Thursday, November 3, 2016 6:02 PM
> > To: dri-devel@lists.freedesktop.org
> > Cc: Sharma, Shashank <shashank.sharma@intel.com>; Lin; Jia, Lin A <lin.a.jia@intel.com>; Sharma, Akashdeep <akashdeep.sharma@intel.com>; Jim Bride <jim.bride@linux.intel.com>; Jose Abreu <Jose.Abreu@synopsys.com>; Daniel Vetter <daniel.vetter@ffwll.ch>; Emil Velikov <emil.l.velikov@gmail.com>
> > Subject: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.
> > 
> > Adding new mode flags willy nilly breaks existing userspace. We need to coordinate this better, potentially with a new client cap that only exposes the aspect ratio flags when userspace is prepared for them (similar to what we do with stereo 3D modes).
> > 
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Cc: Lin, Jia <lin.a.jia@intel.com>
> > Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
> > Cc: Jim Bride <jim.bride@linux.intel.com>
> > Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Emil Velikov <emil.l.velikov@gmail.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_modes.c | 12 ------------  include/uapi/drm/drm_mode.h |  6 ------
> >  2 files changed, 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index f64ac86deb84..725faa6409aa 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1513,12 +1513,6 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
> >  	case HDMI_PICTURE_ASPECT_16_9:
> >  		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
> >  		break;
> > -	case HDMI_PICTURE_ASPECT_64_27:
> > -		out->flags |= DRM_MODE_FLAG_PIC_AR_64_27;
> > -		break;
> > -	case DRM_MODE_PICTURE_ASPECT_256_135:
> > -		out->flags |= DRM_MODE_FLAG_PIC_AR_256_135;
> > -		break;
> >  	case HDMI_PICTURE_ASPECT_RESERVED:
> >  	default:
> >  		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; @@ -1580,12 +1574,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
> >  	case DRM_MODE_FLAG_PIC_AR_16_9:
> >  		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> >  		break;
> > -	case DRM_MODE_FLAG_PIC_AR_64_27:
> > -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27;
> > -		break;
> > -	case DRM_MODE_FLAG_PIC_AR_256_135:
> > -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135;
> > -		break;
> >  	default:
> >  		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >  		break;
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 084b50a02dc5..5c142b1387ac 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -81,8 +81,6 @@ extern "C" {
> >  #define DRM_MODE_PICTURE_ASPECT_NONE		0
> >  #define DRM_MODE_PICTURE_ASPECT_4_3		1
> >  #define DRM_MODE_PICTURE_ASPECT_16_9		2
> > -#define DRM_MODE_PICTURE_ASPECT_64_27		3
> > -#define DRM_MODE_PICTURE_ASPECT_256_135		4
> >  
> >  /* Aspect ratio flag bitmask (4 bits 22:19) */
> >  #define DRM_MODE_FLAG_PIC_AR_MASK		(0x0F<<19)
> > @@ -92,10 +90,6 @@ extern "C" {
> >  			(DRM_MODE_PICTURE_ASPECT_4_3<<19)
> >  #define  DRM_MODE_FLAG_PIC_AR_16_9 \
> >  			(DRM_MODE_PICTURE_ASPECT_16_9<<19)
> > -#define  DRM_MODE_FLAG_PIC_AR_64_27 \
> > -			(DRM_MODE_PICTURE_ASPECT_64_27<<19)
> > -#define  DRM_MODE_FLAG_PIC_AR_256_135 \
> > -			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
> >  
> >  /* DPMS flags */
> >  /* bit compatible with the xorg definitions. */
> > --
> > 2.7.4
> > 
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Thu, Nov 03, 2016 at 03:04:04PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 03, 2016 at 03:02:53PM +0200, Ville Syrjälä wrote:
> > On Thu, Nov 03, 2016 at 12:47:39PM +0000, Sharma, Shashank wrote:
> > > Hi Ville, 
> > > (-dri-devel)
> > > I would appreciate an internal discussion before going to dri-devel. a
> > 
> > Added back. Private discussions are pointless.
> 
> Now added for reals.
> 
> > 
> > > 
> > > What did this patch break ? 
> > 
> > Intel ddx. It will reject any mode with flags it doesn't know about. 
> > Other ddxen like -ati or -modesetting will also suffer, but potentially
> > in a different way since they don't even check the flags and instead
> > just directly stuff them into the randr mode flags, which will result
> > in an out of spec mode as far randr is concerned.

BTW outside of the userspace issues there were some issues with the way
the mode matching was being done. Before I realized the userspace breakage
I had cooked up a few additonal patch to adjust the mode matching a bit.
I pushed those to [1] in case anyone wants to take a look, but obviously
they need to be put on hold a bit until we sort out the userspace part.

[1] git://github.com/vsyrjala/linux.git cea_f_vics

> > 
> > > This is exposed in the same way, as the 3D flags.
> > 
> > Not it isn't. We have a client cap for 3D flags.
> > 
> > > The information is already available in form of flags, all you have to do in userspace is read and print that. Its already being done in Android. 
> > 
> > Please stop relying on Android for testing upstream patches. Test with
> > actual userspace people are using.
> > 
> > >  
> > > NACK until we get to the right reason. 
> > > 
> > > Regards
> > > Shashank
> > > -----Original Message-----
> > > From: ville.syrjala@linux.intel.com [mailto:ville.syrjala@linux.intel.com] 
> > > Sent: Thursday, November 3, 2016 6:02 PM
> > > To: dri-devel@lists.freedesktop.org
> > > Cc: Sharma, Shashank <shashank.sharma@intel.com>; Lin; Jia, Lin A <lin.a.jia@intel.com>; Sharma, Akashdeep <akashdeep.sharma@intel.com>; Jim Bride <jim.bride@linux.intel.com>; Jose Abreu <Jose.Abreu@synopsys.com>; Daniel Vetter <daniel.vetter@ffwll.ch>; Emil Velikov <emil.l.velikov@gmail.com>
> > > Subject: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"
> > > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.
> > > 
> > > Adding new mode flags willy nilly breaks existing userspace. We need to coordinate this better, potentially with a new client cap that only exposes the aspect ratio flags when userspace is prepared for them (similar to what we do with stereo 3D modes).
> > > 
> > > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > > Cc: Lin, Jia <lin.a.jia@intel.com>
> > > Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
> > > Cc: Jim Bride <jim.bride@linux.intel.com>
> > > Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Emil Velikov <emil.l.velikov@gmail.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_modes.c | 12 ------------  include/uapi/drm/drm_mode.h |  6 ------
> > >  2 files changed, 18 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index f64ac86deb84..725faa6409aa 100644
> > > --- a/drivers/gpu/drm/drm_modes.c
> > > +++ b/drivers/gpu/drm/drm_modes.c
> > > @@ -1513,12 +1513,6 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
> > >  	case HDMI_PICTURE_ASPECT_16_9:
> > >  		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
> > >  		break;
> > > -	case HDMI_PICTURE_ASPECT_64_27:
> > > -		out->flags |= DRM_MODE_FLAG_PIC_AR_64_27;
> > > -		break;
> > > -	case DRM_MODE_PICTURE_ASPECT_256_135:
> > > -		out->flags |= DRM_MODE_FLAG_PIC_AR_256_135;
> > > -		break;
> > >  	case HDMI_PICTURE_ASPECT_RESERVED:
> > >  	default:
> > >  		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; @@ -1580,12 +1574,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
> > >  	case DRM_MODE_FLAG_PIC_AR_16_9:
> > >  		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> > >  		break;
> > > -	case DRM_MODE_FLAG_PIC_AR_64_27:
> > > -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27;
> > > -		break;
> > > -	case DRM_MODE_FLAG_PIC_AR_256_135:
> > > -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135;
> > > -		break;
> > >  	default:
> > >  		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> > >  		break;
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 084b50a02dc5..5c142b1387ac 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -81,8 +81,6 @@ extern "C" {
> > >  #define DRM_MODE_PICTURE_ASPECT_NONE		0
> > >  #define DRM_MODE_PICTURE_ASPECT_4_3		1
> > >  #define DRM_MODE_PICTURE_ASPECT_16_9		2
> > > -#define DRM_MODE_PICTURE_ASPECT_64_27		3
> > > -#define DRM_MODE_PICTURE_ASPECT_256_135		4
> > >  
> > >  /* Aspect ratio flag bitmask (4 bits 22:19) */
> > >  #define DRM_MODE_FLAG_PIC_AR_MASK		(0x0F<<19)
> > > @@ -92,10 +90,6 @@ extern "C" {
> > >  			(DRM_MODE_PICTURE_ASPECT_4_3<<19)
> > >  #define  DRM_MODE_FLAG_PIC_AR_16_9 \
> > >  			(DRM_MODE_PICTURE_ASPECT_16_9<<19)
> > > -#define  DRM_MODE_FLAG_PIC_AR_64_27 \
> > > -			(DRM_MODE_PICTURE_ASPECT_64_27<<19)
> > > -#define  DRM_MODE_FLAG_PIC_AR_256_135 \
> > > -			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
> > >  
> > >  /* DPMS flags */
> > >  /* bit compatible with the xorg definitions. */
> > > --
> > > 2.7.4
> > > 
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

Regards

Shashank


On 11/3/2016 6:39 PM, Ville Syrjälä wrote:
> On Thu, Nov 03, 2016 at 03:04:04PM +0200, Ville Syrjälä wrote:
>> On Thu, Nov 03, 2016 at 03:02:53PM +0200, Ville Syrjälä wrote:
>>> On Thu, Nov 03, 2016 at 12:47:39PM +0000, Sharma, Shashank wrote:
>>>> Hi Ville,
>>>> (-dri-devel)
>>>> I would appreciate an internal discussion before going to dri-devel. a
>>> Added back. Private discussions are pointless.
>> Now added for reals.
Noted for future.
>>
>>>> What did this patch break ?
>>> Intel ddx. It will reject any mode with flags it doesn't know about.
>>> Other ddxen like -ati or -modesetting will also suffer, but potentially
>>> in a different way since they don't even check the flags and instead
>>> just directly stuff them into the randr mode flags, which will result
>>> in an out of spec mode as far randr is concerned.
We are adding new flags to indicate aspect ratios, so its obvious that 
the implementation should be modified to accept new flags.
I dont see why you should revert patch for it. This is instead option to 
extend the support.
> BTW outside of the userspace issues there were some issues with the way
> the mode matching was being done. Before I realized the userspace breakage
Again, this is not a break, we need to extend the test-setup for new flags.
> I had cooked up a few additonal patch to adjust the mode matching a bit.
> I pushed those to [1] in case anyone wants to take a look, but obviously
> they need to be put on hold a bit until we sort out the userspace part.
>
> [1] git://github.com/vsyrjala/linux.git cea_f_vics
I think we had several rounds of code reviews on this, since months. So 
I don't really see why are we discussing this now.
>>>> This is exposed in the same way, as the 3D flags.
>>> Not it isn't. We have a client cap for 3D flags.
>>>
>>>> The information is already available in form of flags, all you have to do in userspace is read and print that. Its already being done in Android.
>>> Please stop relying on Android for testing upstream patches. Test with
>>> actual userspace people are using.
Please note that android goes through HDMI compliance certification, and 
several commercial test infrastructures, so
that's one of the best infrastructure to test. And when we have common 
branch for all of Linux flavors, we should consider all.
I dont really think how breaking / blocking Android implementation is 
going to help others.
Also, the breaks what you are reporting are with the test tools / 
infrastructures, which is specific to one setup.

Honestly, thats one of the setup which uses aspect ratio in practical 
use cases, so I dont find a reason why not.
>>>
>>>>   
>>>> NACK until we get to the right reason.

My nack continues.
>>>>
>>>> Regards
>>>> Shashank
>>>> -----Original Message-----
>>>> From: ville.syrjala@linux.intel.com [mailto:ville.syrjala@linux.intel.com]
>>>> Sent: Thursday, November 3, 2016 6:02 PM
>>>> To: dri-devel@lists.freedesktop.org
>>>> Cc: Sharma, Shashank <shashank.sharma@intel.com>; Lin; Jia, Lin A <lin.a.jia@intel.com>; Sharma, Akashdeep <akashdeep.sharma@intel.com>; Jim Bride <jim.bride@linux.intel.com>; Jose Abreu <Jose.Abreu@synopsys.com>; Daniel Vetter <daniel.vetter@ffwll.ch>; Emil Velikov <emil.l.velikov@gmail.com>
>>>> Subject: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"
>>>>
>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>
>>>> This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.
>>>>
>>>> Adding new mode flags willy nilly breaks existing userspace. We need to coordinate this better, potentially with a new client cap that only exposes the aspect ratio flags when userspace is prepared for them (similar to what we do with stereo 3D modes).
>>>>
>>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>>> Cc: Lin, Jia <lin.a.jia@intel.com>
>>>> Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
>>>> Cc: Jim Bride <jim.bride@linux.intel.com>
>>>> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_modes.c | 12 ------------  include/uapi/drm/drm_mode.h |  6 ------
>>>>   2 files changed, 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index f64ac86deb84..725faa6409aa 100644
>>>> --- a/drivers/gpu/drm/drm_modes.c
>>>> +++ b/drivers/gpu/drm/drm_modes.c
>>>> @@ -1513,12 +1513,6 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>>>>   	case HDMI_PICTURE_ASPECT_16_9:
>>>>   		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
>>>>   		break;
>>>> -	case HDMI_PICTURE_ASPECT_64_27:
>>>> -		out->flags |= DRM_MODE_FLAG_PIC_AR_64_27;
>>>> -		break;
>>>> -	case DRM_MODE_PICTURE_ASPECT_256_135:
>>>> -		out->flags |= DRM_MODE_FLAG_PIC_AR_256_135;
>>>> -		break;
>>>>   	case HDMI_PICTURE_ASPECT_RESERVED:
>>>>   	default:
>>>>   		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; @@ -1580,12 +1574,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>>>>   	case DRM_MODE_FLAG_PIC_AR_16_9:
>>>>   		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
>>>>   		break;
>>>> -	case DRM_MODE_FLAG_PIC_AR_64_27:
>>>> -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27;
>>>> -		break;
>>>> -	case DRM_MODE_FLAG_PIC_AR_256_135:
>>>> -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135;
>>>> -		break;
>>>>   	default:
>>>>   		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>>   		break;
>>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 084b50a02dc5..5c142b1387ac 100644
>>>> --- a/include/uapi/drm/drm_mode.h
>>>> +++ b/include/uapi/drm/drm_mode.h
>>>> @@ -81,8 +81,6 @@ extern "C" {
>>>>   #define DRM_MODE_PICTURE_ASPECT_NONE		0
>>>>   #define DRM_MODE_PICTURE_ASPECT_4_3		1
>>>>   #define DRM_MODE_PICTURE_ASPECT_16_9		2
>>>> -#define DRM_MODE_PICTURE_ASPECT_64_27		3
>>>> -#define DRM_MODE_PICTURE_ASPECT_256_135		4
>>>>   
>>>>   /* Aspect ratio flag bitmask (4 bits 22:19) */
>>>>   #define DRM_MODE_FLAG_PIC_AR_MASK		(0x0F<<19)
>>>> @@ -92,10 +90,6 @@ extern "C" {
>>>>   			(DRM_MODE_PICTURE_ASPECT_4_3<<19)
>>>>   #define  DRM_MODE_FLAG_PIC_AR_16_9 \
>>>>   			(DRM_MODE_PICTURE_ASPECT_16_9<<19)
>>>> -#define  DRM_MODE_FLAG_PIC_AR_64_27 \
>>>> -			(DRM_MODE_PICTURE_ASPECT_64_27<<19)
>>>> -#define  DRM_MODE_FLAG_PIC_AR_256_135 \
>>>> -			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
>>>>   
>>>>   /* DPMS flags */
>>>>   /* bit compatible with the xorg definitions. */
>>>> --
>>>> 2.7.4
>>>>
>>> -- 
>>> Ville Syrjälä
>>> Intel OTC
>> -- 
>> Ville Syrjälä
>> Intel OTC
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Thu, Nov 03, 2016 at 06:40:11PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/3/2016 6:32 PM, Ville Syrjälä wrote:
> > On Thu, Nov 03, 2016 at 12:47:39PM +0000, Sharma, Shashank wrote:
> >> Hi Ville,
> >> (-dri-devel)
> >> I would appreciate an internal discussion before going to dri-devel. a
> > Added back. Private discussions are pointless.
> Noted.
> >> What did this patch break ?
> > Intel ddx. It will reject any mode with flags it doesn't know about.
> > Other ddxen like -ati or -modesetting will also suffer, but potentially
> > in a different way since they don't even check the flags and instead
> > just directly stuff them into the randr mode flags, which will result
> > in an out of spec mode as far randr is concerned.
> Then its a fault of intel-ddx design. We are adding new flags to 
> indicate aspect ratios, so its obvious that the implementation should be 
> modified to accept new flags.
> I dont see why you should revert patch for it.

Rule 1 of kernel development: You simply don't break existing userspace

> >> This is exposed in the same way, as the 3D flags.
> > Not it isn't. We have a client cap for 3D flags.
> >
> >> The information is already available in form of flags, all you have to do in userspace is read and print that. Its already being done in Android.
> > Please stop relying on Android for testing upstream patches. Test with
> > actual userspace people are using.
> Please note that android goes through HDMI compliance certification, and 
> several commercial test infrastructures, so
> that's one of the best infrastructure to test. And when we have common 
> branch for all of Linux flavors, we should consider all.

You can test with Android as much as you like. That probably won't
catch ABI breaks and whatnot though since you don't generally just plop
a new kernel onto an existing Android device.

So you really must test with normal userspace as well. That has a long
history and so any ABI/behaviour change is actually a big deal.

> >>   
> >> NACK until we get to the right reason.
> >>
> >> Regards
> >> Shashank
> >> -----Original Message-----
> >> From: ville.syrjala@linux.intel.com [mailto:ville.syrjala@linux.intel.com]
> >> Sent: Thursday, November 3, 2016 6:02 PM
> >> To: dri-devel@lists.freedesktop.org
> >> Cc: Sharma, Shashank <shashank.sharma@intel.com>; Lin; Jia, Lin A <lin.a.jia@intel.com>; Sharma, Akashdeep <akashdeep.sharma@intel.com>; Jim Bride <jim.bride@linux.intel.com>; Jose Abreu <Jose.Abreu@synopsys.com>; Daniel Vetter <daniel.vetter@ffwll.ch>; Emil Velikov <emil.l.velikov@gmail.com>
> >> Subject: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"
> >>
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.
> >>
> >> Adding new mode flags willy nilly breaks existing userspace. We need to coordinate this better, potentially with a new client cap that only exposes the aspect ratio flags when userspace is prepared for them (similar to what we do with stereo 3D modes).
> >>
> >> Cc: Shashank Sharma <shashank.sharma@intel.com>
> >> Cc: Lin, Jia <lin.a.jia@intel.com>
> >> Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
> >> Cc: Jim Bride <jim.bride@linux.intel.com>
> >> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_modes.c | 12 ------------  include/uapi/drm/drm_mode.h |  6 ------
> >>   2 files changed, 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index f64ac86deb84..725faa6409aa 100644
> >> --- a/drivers/gpu/drm/drm_modes.c
> >> +++ b/drivers/gpu/drm/drm_modes.c
> >> @@ -1513,12 +1513,6 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
> >>   	case HDMI_PICTURE_ASPECT_16_9:
> >>   		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
> >>   		break;
> >> -	case HDMI_PICTURE_ASPECT_64_27:
> >> -		out->flags |= DRM_MODE_FLAG_PIC_AR_64_27;
> >> -		break;
> >> -	case DRM_MODE_PICTURE_ASPECT_256_135:
> >> -		out->flags |= DRM_MODE_FLAG_PIC_AR_256_135;
> >> -		break;
> >>   	case HDMI_PICTURE_ASPECT_RESERVED:
> >>   	default:
> >>   		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; @@ -1580,12 +1574,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
> >>   	case DRM_MODE_FLAG_PIC_AR_16_9:
> >>   		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> >>   		break;
> >> -	case DRM_MODE_FLAG_PIC_AR_64_27:
> >> -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27;
> >> -		break;
> >> -	case DRM_MODE_FLAG_PIC_AR_256_135:
> >> -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135;
> >> -		break;
> >>   	default:
> >>   		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>   		break;
> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 084b50a02dc5..5c142b1387ac 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -81,8 +81,6 @@ extern "C" {
> >>   #define DRM_MODE_PICTURE_ASPECT_NONE		0
> >>   #define DRM_MODE_PICTURE_ASPECT_4_3		1
> >>   #define DRM_MODE_PICTURE_ASPECT_16_9		2
> >> -#define DRM_MODE_PICTURE_ASPECT_64_27		3
> >> -#define DRM_MODE_PICTURE_ASPECT_256_135		4
> >>   
> >>   /* Aspect ratio flag bitmask (4 bits 22:19) */
> >>   #define DRM_MODE_FLAG_PIC_AR_MASK		(0x0F<<19)
> >> @@ -92,10 +90,6 @@ extern "C" {
> >>   			(DRM_MODE_PICTURE_ASPECT_4_3<<19)
> >>   #define  DRM_MODE_FLAG_PIC_AR_16_9 \
> >>   			(DRM_MODE_PICTURE_ASPECT_16_9<<19)
> >> -#define  DRM_MODE_FLAG_PIC_AR_64_27 \
> >> -			(DRM_MODE_PICTURE_ASPECT_64_27<<19)
> >> -#define  DRM_MODE_FLAG_PIC_AR_256_135 \
> >> -			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
> >>   
> >>   /* DPMS flags */
> >>   /* bit compatible with the xorg definitions. */
> >> --
> >> 2.7.4
> >>

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

Regards

Shashank


On 11/3/2016 6:56 PM, Ville Syrjälä wrote:
> On Thu, Nov 03, 2016 at 06:40:11PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 11/3/2016 6:32 PM, Ville Syrjälä wrote:
>>> On Thu, Nov 03, 2016 at 12:47:39PM +0000, Sharma, Shashank wrote:
>>>> Hi Ville,
>>>> (-dri-devel)
>>>> I would appreciate an internal discussion before going to dri-devel. a
>>> Added back. Private discussions are pointless.
>> Noted.
>>>> What did this patch break ?
>>> Intel ddx. It will reject any mode with flags it doesn't know about.
>>> Other ddxen like -ati or -modesetting will also suffer, but potentially
>>> in a different way since they don't even check the flags and instead
>>> just directly stuff them into the randr mode flags, which will result
>>> in an out of spec mode as far randr is concerned.
>> Then its a fault of intel-ddx design. We are adding new flags to
>> indicate aspect ratios, so its obvious that the implementation should be
>> modified to accept new flags.
>> I dont see why you should revert patch for it.
> Rule 1 of kernel development: You simply don't break existing userspace
Again, we are adding something new, for which userspace should be modified.
If the userspace in written in such a way that it wont accept changes, 
its not saleable design.

Even in this case, we should go and extend the userspace (which I am 
ready to do) instead of reverting the patch.
If you still think you should send this revert, I am removing my NACK. 
Pls Go ahead.
>>>> This is exposed in the same way, as the 3D flags.
>>> Not it isn't. We have a client cap for 3D flags.
>>>
>>>> The information is already available in form of flags, all you have to do in userspace is read and print that. Its already being done in Android.
>>> Please stop relying on Android for testing upstream patches. Test with
>>> actual userspace people are using.
>> Please note that android goes through HDMI compliance certification, and
>> several commercial test infrastructures, so
>> that's one of the best infrastructure to test. And when we have common
>> branch for all of Linux flavors, we should consider all.
> You can test with Android as much as you like. That probably won't
> catch ABI breaks and whatnot though since you don't generally just plop
> a new kernel onto an existing Android device.
>
> So you really must test with normal userspace as well. That has a long
> history and so any ABI/behaviour change is actually a big deal.
>
>>>>    
>>>> NACK until we get to the right reason.
>>>>
>>>> Regards
>>>> Shashank
>>>> -----Original Message-----
>>>> From: ville.syrjala@linux.intel.com [mailto:ville.syrjala@linux.intel.com]
>>>> Sent: Thursday, November 3, 2016 6:02 PM
>>>> To: dri-devel@lists.freedesktop.org
>>>> Cc: Sharma, Shashank <shashank.sharma@intel.com>; Lin; Jia, Lin A <lin.a.jia@intel.com>; Sharma, Akashdeep <akashdeep.sharma@intel.com>; Jim Bride <jim.bride@linux.intel.com>; Jose Abreu <Jose.Abreu@synopsys.com>; Daniel Vetter <daniel.vetter@ffwll.ch>; Emil Velikov <emil.l.velikov@gmail.com>
>>>> Subject: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"
>>>>
>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>
>>>> This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.
>>>>
>>>> Adding new mode flags willy nilly breaks existing userspace. We need to coordinate this better, potentially with a new client cap that only exposes the aspect ratio flags when userspace is prepared for them (similar to what we do with stereo 3D modes).
>>>>
>>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>>> Cc: Lin, Jia <lin.a.jia@intel.com>
>>>> Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
>>>> Cc: Jim Bride <jim.bride@linux.intel.com>
>>>> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_modes.c | 12 ------------  include/uapi/drm/drm_mode.h |  6 ------
>>>>    2 files changed, 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index f64ac86deb84..725faa6409aa 100644
>>>> --- a/drivers/gpu/drm/drm_modes.c
>>>> +++ b/drivers/gpu/drm/drm_modes.c
>>>> @@ -1513,12 +1513,6 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>>>>    	case HDMI_PICTURE_ASPECT_16_9:
>>>>    		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
>>>>    		break;
>>>> -	case HDMI_PICTURE_ASPECT_64_27:
>>>> -		out->flags |= DRM_MODE_FLAG_PIC_AR_64_27;
>>>> -		break;
>>>> -	case DRM_MODE_PICTURE_ASPECT_256_135:
>>>> -		out->flags |= DRM_MODE_FLAG_PIC_AR_256_135;
>>>> -		break;
>>>>    	case HDMI_PICTURE_ASPECT_RESERVED:
>>>>    	default:
>>>>    		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; @@ -1580,12 +1574,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>>>>    	case DRM_MODE_FLAG_PIC_AR_16_9:
>>>>    		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
>>>>    		break;
>>>> -	case DRM_MODE_FLAG_PIC_AR_64_27:
>>>> -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27;
>>>> -		break;
>>>> -	case DRM_MODE_FLAG_PIC_AR_256_135:
>>>> -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135;
>>>> -		break;
>>>>    	default:
>>>>    		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>>    		break;
>>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 084b50a02dc5..5c142b1387ac 100644
>>>> --- a/include/uapi/drm/drm_mode.h
>>>> +++ b/include/uapi/drm/drm_mode.h
>>>> @@ -81,8 +81,6 @@ extern "C" {
>>>>    #define DRM_MODE_PICTURE_ASPECT_NONE		0
>>>>    #define DRM_MODE_PICTURE_ASPECT_4_3		1
>>>>    #define DRM_MODE_PICTURE_ASPECT_16_9		2
>>>> -#define DRM_MODE_PICTURE_ASPECT_64_27		3
>>>> -#define DRM_MODE_PICTURE_ASPECT_256_135		4
>>>>    
>>>>    /* Aspect ratio flag bitmask (4 bits 22:19) */
>>>>    #define DRM_MODE_FLAG_PIC_AR_MASK		(0x0F<<19)
>>>> @@ -92,10 +90,6 @@ extern "C" {
>>>>    			(DRM_MODE_PICTURE_ASPECT_4_3<<19)
>>>>    #define  DRM_MODE_FLAG_PIC_AR_16_9 \
>>>>    			(DRM_MODE_PICTURE_ASPECT_16_9<<19)
>>>> -#define  DRM_MODE_FLAG_PIC_AR_64_27 \
>>>> -			(DRM_MODE_PICTURE_ASPECT_64_27<<19)
>>>> -#define  DRM_MODE_FLAG_PIC_AR_256_135 \
>>>> -			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
>>>>    
>>>>    /* DPMS flags */
>>>>    /* bit compatible with the xorg definitions. */
>>>> --
>>>> 2.7.4
>>>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Thu, Nov 03, 2016 at 07:04:37PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/3/2016 6:56 PM, Ville Syrjälä wrote:
> > On Thu, Nov 03, 2016 at 06:40:11PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 11/3/2016 6:32 PM, Ville Syrjälä wrote:
> >>> On Thu, Nov 03, 2016 at 12:47:39PM +0000, Sharma, Shashank wrote:
> >>>> Hi Ville,
> >>>> (-dri-devel)
> >>>> I would appreciate an internal discussion before going to dri-devel. a
> >>> Added back. Private discussions are pointless.
> >> Noted.
> >>>> What did this patch break ?
> >>> Intel ddx. It will reject any mode with flags it doesn't know about.
> >>> Other ddxen like -ati or -modesetting will also suffer, but potentially
> >>> in a different way since they don't even check the flags and instead
> >>> just directly stuff them into the randr mode flags, which will result
> >>> in an out of spec mode as far randr is concerned.
> >> Then its a fault of intel-ddx design. We are adding new flags to
> >> indicate aspect ratios, so its obvious that the implementation should be
> >> modified to accept new flags.
> >> I dont see why you should revert patch for it.
> > Rule 1 of kernel development: You simply don't break existing userspace
> Again, we are adding something new, for which userspace should be modified.
> If the userspace in written in such a way that it wont accept changes, 
> its not saleable design.

Whether the design is good or not is besides the point. It already
exists, so we have to live with it.

> 
> Even in this case, we should go and extend the userspace (which I am 
> ready to do) instead of reverting the patch.

We can't retroactively fix the binaries on everyone's machine.

> If you still think you should send this revert, I am removing my NACK. 
> Pls Go ahead.

The other option is to not revert and instead slap a fix on top. But 
that would have to be done reasonably quickly so that the thing is
ready in time for 4.10. We're closing in on 4.9-rc4 now so I guess
we should still have a few weeks to fix things up. Whether that's
enough I don't know. If not, then we should revert.

> >>>> This is exposed in the same way, as the 3D flags.
> >>> Not it isn't. We have a client cap for 3D flags.
> >>>
> >>>> The information is already available in form of flags, all you have to do in userspace is read and print that. Its already being done in Android.
> >>> Please stop relying on Android for testing upstream patches. Test with
> >>> actual userspace people are using.
> >> Please note that android goes through HDMI compliance certification, and
> >> several commercial test infrastructures, so
> >> that's one of the best infrastructure to test. And when we have common
> >> branch for all of Linux flavors, we should consider all.
> > You can test with Android as much as you like. That probably won't
> > catch ABI breaks and whatnot though since you don't generally just plop
> > a new kernel onto an existing Android device.
> >
> > So you really must test with normal userspace as well. That has a long
> > history and so any ABI/behaviour change is actually a big deal.
> >
> >>>>    
> >>>> NACK until we get to the right reason.
> >>>>
> >>>> Regards
> >>>> Shashank
> >>>> -----Original Message-----
> >>>> From: ville.syrjala@linux.intel.com [mailto:ville.syrjala@linux.intel.com]
> >>>> Sent: Thursday, November 3, 2016 6:02 PM
> >>>> To: dri-devel@lists.freedesktop.org
> >>>> Cc: Sharma, Shashank <shashank.sharma@intel.com>; Lin; Jia, Lin A <lin.a.jia@intel.com>; Sharma, Akashdeep <akashdeep.sharma@intel.com>; Jim Bride <jim.bride@linux.intel.com>; Jose Abreu <Jose.Abreu@synopsys.com>; Daniel Vetter <daniel.vetter@ffwll.ch>; Emil Velikov <emil.l.velikov@gmail.com>
> >>>> Subject: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"
> >>>>
> >>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>
> >>>> This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.
> >>>>
> >>>> Adding new mode flags willy nilly breaks existing userspace. We need to coordinate this better, potentially with a new client cap that only exposes the aspect ratio flags when userspace is prepared for them (similar to what we do with stereo 3D modes).
> >>>>
> >>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
> >>>> Cc: Lin, Jia <lin.a.jia@intel.com>
> >>>> Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
> >>>> Cc: Jim Bride <jim.bride@linux.intel.com>
> >>>> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/drm_modes.c | 12 ------------  include/uapi/drm/drm_mode.h |  6 ------
> >>>>    2 files changed, 18 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index f64ac86deb84..725faa6409aa 100644
> >>>> --- a/drivers/gpu/drm/drm_modes.c
> >>>> +++ b/drivers/gpu/drm/drm_modes.c
> >>>> @@ -1513,12 +1513,6 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
> >>>>    	case HDMI_PICTURE_ASPECT_16_9:
> >>>>    		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
> >>>>    		break;
> >>>> -	case HDMI_PICTURE_ASPECT_64_27:
> >>>> -		out->flags |= DRM_MODE_FLAG_PIC_AR_64_27;
> >>>> -		break;
> >>>> -	case DRM_MODE_PICTURE_ASPECT_256_135:
> >>>> -		out->flags |= DRM_MODE_FLAG_PIC_AR_256_135;
> >>>> -		break;
> >>>>    	case HDMI_PICTURE_ASPECT_RESERVED:
> >>>>    	default:
> >>>>    		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; @@ -1580,12 +1574,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
> >>>>    	case DRM_MODE_FLAG_PIC_AR_16_9:
> >>>>    		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> >>>>    		break;
> >>>> -	case DRM_MODE_FLAG_PIC_AR_64_27:
> >>>> -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27;
> >>>> -		break;
> >>>> -	case DRM_MODE_FLAG_PIC_AR_256_135:
> >>>> -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135;
> >>>> -		break;
> >>>>    	default:
> >>>>    		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>>>    		break;
> >>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 084b50a02dc5..5c142b1387ac 100644
> >>>> --- a/include/uapi/drm/drm_mode.h
> >>>> +++ b/include/uapi/drm/drm_mode.h
> >>>> @@ -81,8 +81,6 @@ extern "C" {
> >>>>    #define DRM_MODE_PICTURE_ASPECT_NONE		0
> >>>>    #define DRM_MODE_PICTURE_ASPECT_4_3		1
> >>>>    #define DRM_MODE_PICTURE_ASPECT_16_9		2
> >>>> -#define DRM_MODE_PICTURE_ASPECT_64_27		3
> >>>> -#define DRM_MODE_PICTURE_ASPECT_256_135		4
> >>>>    
> >>>>    /* Aspect ratio flag bitmask (4 bits 22:19) */
> >>>>    #define DRM_MODE_FLAG_PIC_AR_MASK		(0x0F<<19)
> >>>> @@ -92,10 +90,6 @@ extern "C" {
> >>>>    			(DRM_MODE_PICTURE_ASPECT_4_3<<19)
> >>>>    #define  DRM_MODE_FLAG_PIC_AR_16_9 \
> >>>>    			(DRM_MODE_PICTURE_ASPECT_16_9<<19)
> >>>> -#define  DRM_MODE_FLAG_PIC_AR_64_27 \
> >>>> -			(DRM_MODE_PICTURE_ASPECT_64_27<<19)
> >>>> -#define  DRM_MODE_FLAG_PIC_AR_256_135 \
> >>>> -			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
> >>>>    
> >>>>    /* DPMS flags */
> >>>>    /* bit compatible with the xorg definitions. */
> >>>> --
> >>>> 2.7.4
> >>>>

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Daniel Vetter]

On Thu, Nov 3, 2016 at 1:47 PM, Sharma, Shashank
<shashank.sharma@intel.com> wrote:
> Hi Ville,
> (-dri-devel)
> I would appreciate an internal discussion before going to dri-devel.

Nack on internal discssion. We do development in the open.

> What did this patch break ?
> This is exposed in the same way, as the 3D flags.
> The information is already available in form of flags, all you have to do in userspace is read and print that. Its already being done in Android.

Yeah, the patch is missing details on the bug report and how it falls
apart, but if that's added revert might be the appropriate thing to do
here. Maintainer-ack on the revert if that crucial information is
added.
-Daniel

> NACK until we get to the right reason.
>
> Regards
> Shashank
> -----Original Message-----
> From: ville.syrjala@linux.intel.com [mailto:ville.syrjala@linux.intel.com]
> Sent: Thursday, November 3, 2016 6:02 PM
> To: dri-devel@lists.freedesktop.org
> Cc: Sharma, Shashank <shashank.sharma@intel.com>; Lin; Jia, Lin A <lin.a.jia@intel.com>; Sharma, Akashdeep <akashdeep.sharma@intel.com>; Jim Bride <jim.bride@linux.intel.com>; Jose Abreu <Jose.Abreu@synopsys.com>; Daniel Vetter <daniel.vetter@ffwll.ch>; Emil Velikov <emil.l.velikov@gmail.com>
> Subject: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"
>
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.
>
> Adding new mode flags willy nilly breaks existing userspace. We need to coordinate this better, potentially with a new client cap that only exposes the aspect ratio flags when userspace is prepared for them (similar to what we do with stereo 3D modes).
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Lin, Jia <lin.a.jia@intel.com>
> Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
> Cc: Jim Bride <jim.bride@linux.intel.com>
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_modes.c | 12 ------------  include/uapi/drm/drm_mode.h |  6 ------
>  2 files changed, 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index f64ac86deb84..725faa6409aa 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1513,12 +1513,6 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>         case HDMI_PICTURE_ASPECT_16_9:
>                 out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
>                 break;
> -       case HDMI_PICTURE_ASPECT_64_27:
> -               out->flags |= DRM_MODE_FLAG_PIC_AR_64_27;
> -               break;
> -       case DRM_MODE_PICTURE_ASPECT_256_135:
> -               out->flags |= DRM_MODE_FLAG_PIC_AR_256_135;
> -               break;
>         case HDMI_PICTURE_ASPECT_RESERVED:
>         default:
>                 out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; @@ -1580,12 +1574,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>         case DRM_MODE_FLAG_PIC_AR_16_9:
>                 out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
>                 break;
> -       case DRM_MODE_FLAG_PIC_AR_64_27:
> -               out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27;
> -               break;
> -       case DRM_MODE_FLAG_PIC_AR_256_135:
> -               out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135;
> -               break;
>         default:
>                 out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>                 break;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 084b50a02dc5..5c142b1387ac 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -81,8 +81,6 @@ extern "C" {
>  #define DRM_MODE_PICTURE_ASPECT_NONE           0
>  #define DRM_MODE_PICTURE_ASPECT_4_3            1
>  #define DRM_MODE_PICTURE_ASPECT_16_9           2
> -#define DRM_MODE_PICTURE_ASPECT_64_27          3
> -#define DRM_MODE_PICTURE_ASPECT_256_135                4
>
>  /* Aspect ratio flag bitmask (4 bits 22:19) */
>  #define DRM_MODE_FLAG_PIC_AR_MASK              (0x0F<<19)
> @@ -92,10 +90,6 @@ extern "C" {
>                         (DRM_MODE_PICTURE_ASPECT_4_3<<19)
>  #define  DRM_MODE_FLAG_PIC_AR_16_9 \
>                         (DRM_MODE_PICTURE_ASPECT_16_9<<19)
> -#define  DRM_MODE_FLAG_PIC_AR_64_27 \
> -                       (DRM_MODE_PICTURE_ASPECT_64_27<<19)
> -#define  DRM_MODE_FLAG_PIC_AR_256_135 \
> -                       (DRM_MODE_PICTURE_ASPECT_256_135<<19)
>
>  /* DPMS flags */
>  /* bit compatible with the xorg definitions. */
> --
> 2.7.4
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Daniel Vetter]

On Thu, Nov 3, 2016 at 2:49 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> If you still think you should send this revert, I am removing my NACK.
>> Pls Go ahead.
>
> The other option is to not revert and instead slap a fix on top. But
> that would have to be done reasonably quickly so that the thing is
> ready in time for 4.10. We're closing in on 4.9-rc4 now so I guess
> we should still have a few weeks to fix things up. Whether that's
> enough I don't know. If not, then we should revert.

1 week and then revert is the guideline. Please don't bend the rules
for regressions all the time, it makes things painful for everyone
else.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

On 11/3/2016 9:33 PM, Daniel Vetter wrote:
> On Thu, Nov 3, 2016 at 2:49 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>>> If you still think you should send this revert, I am removing my NACK.
>>> Pls Go ahead.
>> The other option is to not revert and instead slap a fix on top. But
>> that would have to be done reasonably quickly so that the thing is
>> ready in time for 4.10. We're closing in on 4.9-rc4 now so I guess
>> we should still have a few weeks to fix things up. Whether that's
>> enough I don't know. If not, then we should revert.
> 1 week and then revert is the guideline. Please don't bend the rules
> for regressions all the time, it makes things painful for everyone
> else.
> -Daniel
I don't remember breaking it for first time itself.
As I mentioned in the other thread, please add the regression details.
After that, revert it if you want (I saw you have already given 
maintainers-ack)

Regards
Shashank

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

If I was not very clear for the first time, every time we send a patch 
to drm-intel/dri-devel, we do basic testing on Gnome-desktop too (Not 
only Android).

So even these aspect ratio patches were tested with full gnome-desktop, 
and it worked well.


Regards
Shashank
On 11/3/2016 9:49 PM, Sharma, Shashank wrote:
>
>
> On 11/3/2016 9:33 PM, Daniel Vetter wrote:
>> On Thu, Nov 3, 2016 at 2:49 PM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>>>> If you still think you should send this revert, I am removing my NACK.
>>>> Pls Go ahead.
>>> The other option is to not revert and instead slap a fix on top. But
>>> that would have to be done reasonably quickly so that the thing is
>>> ready in time for 4.10. We're closing in on 4.9-rc4 now so I guess
>>> we should still have a few weeks to fix things up. Whether that's
>>> enough I don't know. If not, then we should revert.
>> 1 week and then revert is the guideline. Please don't bend the rules
>> for regressions all the time, it makes things painful for everyone
>> else.
>> -Daniel
> I don't remember breaking it for first time itself.
> As I mentioned in the other thread, please add the regression details.
> After that, revert it if you want (I saw you have already given 
> maintainers-ack)
>
> Regards
> Shashank
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Emil Velikov]

On 7 November 2016 at 07:43, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> If I was not very clear for the first time, every time we send a patch to
> drm-intel/dri-devel, we do basic testing on Gnome-desktop too (Not only
> Android).
>
> So even these aspect ratio patches were tested with full gnome-desktop, and
> it worked well.
>
>
I guess the part that's not so obvious is that Linux
desktop/distributions provide a very wide permutation of components
and configs used. While on a local (test) setup only a small/limited
set is available. Esp. on Android where things are _very_ tightly
coupled.

Obviously nobody likes when we have to carry kernel patches which
workaround "broken" userspace, but it's a kernel policy and we all
have to live with it. That's the main reason people are so
careful/pedantic when it comes to UABI. And as you can see even then
there are bits that we miss :'-(

Regards,
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

Regards

Shashank


On 11/7/2016 8:56 PM, Emil Velikov wrote:
> On 7 November 2016 at 07:43, Sharma, Shashank <shashank.sharma@intel.com> wrote:
>> If I was not very clear for the first time, every time we send a patch to
>> drm-intel/dri-devel, we do basic testing on Gnome-desktop too (Not only
>> Android).
>>
>> So even these aspect ratio patches were tested with full gnome-desktop, and
>> it worked well.
>>
>>
> I guess the part that's not so obvious is that Linux
> desktop/distributions provide a very wide permutation of components
> and configs used. While on a local (test) setup only a small/limited
> set is available. Esp. on Android where things are _very_ tightly
> coupled.
I agree, Emil.
I was only mentioning my testing with Gnome, to confirm that intel-ddx 
is not broken with Gnome desktop.
And I was testing on both Android as well as X.
> Obviously nobody likes when we have to carry kernel patches which
> workaround "broken" userspace, but it's a kernel policy and we all
> have to live with it. That's the main reason people are so
> careful/pedantic when it comes to UABI. And as you can see even then
> there are bits that we miss :'-(
I agree, again. But I was thinking if reverting the patch was the best 
way, else it would be impossible to
add something new in the kernel. I hope experts like you and others can 
suggest the right way.
>
> Regards,
> Emil

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Emil Velikov]

On 7 November 2016 at 15:48, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> Regards
>
> Shashank
>
>
> On 11/7/2016 8:56 PM, Emil Velikov wrote:
>>
>> On 7 November 2016 at 07:43, Sharma, Shashank <shashank.sharma@intel.com>
>> wrote:
>>>
>>> If I was not very clear for the first time, every time we send a patch to
>>> drm-intel/dri-devel, we do basic testing on Gnome-desktop too (Not only
>>> Android).
>>>
>>> So even these aspect ratio patches were tested with full gnome-desktop,
>>> and
>>> it worked well.
>>>
>>>
>> I guess the part that's not so obvious is that Linux
>> desktop/distributions provide a very wide permutation of components
>> and configs used. While on a local (test) setup only a small/limited
>> set is available. Esp. on Android where things are _very_ tightly
>> coupled.
>
> I agree, Emil.
> I was only mentioning my testing with Gnome, to confirm that intel-ddx is
> not broken with Gnome desktop.
> And I was testing on both Android as well as X.
>>
>> Obviously nobody likes when we have to carry kernel patches which
>> workaround "broken" userspace, but it's a kernel policy and we all
>> have to live with it. That's the main reason people are so
>> careful/pedantic when it comes to UABI. And as you can see even then
>> there are bits that we miss :'-(
>
> I agree, again. But I was thinking if reverting the patch was the best way,
> else it would be impossible to
> add something new in the kernel. I hope experts like you and others can
> suggest the right way.
Afaict, Daniel Vetter and Ville mentioned the best route - "1 week and
then revert is the guideline" and "... with a new client cap ..."
(grep for DRM_CAP_) respectively.

It the former is missing from the documentation, so feel free to point
out where you'd expect it to be. Or even better send a patch
describing it based on your experience. Things explained form your POV
would read better (and be easier to find) for people unfamiliar with
the topic.

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Thu, Nov 03, 2016 at 02:31:43PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.
> 
> Adding new mode flags willy nilly breaks existing userspace. We need to
> coordinate this better, potentially with a new client cap that only
> exposes the aspect ratio flags when userspace is prepared for them
> (similar to what we do with stereo 3D modes).

As a demonstration here's the change in the xrandr mode list after doing
the revert:

 HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
-   1920x1080     60.00*+
-   1920x1080i    60.00    50.00  
+   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98  
+   1920x1080i    60.00    50.00    59.94  
    1600x1200     60.00  
    1680x1050     59.88  
    1280x1024     75.02    60.02  
@@ -13,30 +13,29 @@
    1360x768      60.02  
    1280x800      59.91  
    1152x864      75.00  
-   1280x720      60.00    50.00  
+   1280x720      60.00    50.00    59.94  
    1024x768      75.03    70.07    60.00  
    832x624       74.55  
    800x600       72.19    75.00    60.32  
-   640x480       75.00    72.81    66.67    59.94  
+   720x576       50.00  
+   720x480       60.00    59.94  
+   640x480       75.00    72.81    66.67    60.00    59.94  
    720x400       70.08  

This was with sna, which does this:
 #define KNOWN_MODE_FLAGS ((1<<14)-1)
 if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
 	mode->status = MODE_BAD; /* unknown flags => unhandled */
so all the modes with an aspect ratio just vanished.

-modesetting and -ati on the other hand just copy over the unknown
bits into the xrandr mode structure, which sounds dubious at best:
 mode->Flags = kmode->flags; //& FLAG_BITS;
I've not checked what damage it can actually cause.


It looks like a few modes disappeared from the kernel's mode list
as well, presumably because some cea modes in the list originated from
DTDs and whanot so they don't have an aspect ratio and that causes 
add_alternate_cea_modes() to ignore them. So not populating an aspect
ratio for cea modes originating from a source other than 
edid_cea_modes[] looks like another bug to me as well.

Another bug I think might be the ordering of the modes with aspect ratio
specified. IIRC the spec says that the preferred aspect ratio should be
listed first in the EDID, but I don't think we preserve that ordering
in the final mode list. I guess we could fix that by somehow noting
which aspect ratio is preferred and sort based on that, or we try to
preserve the order from the EDID until we're ready to sort, and then do
the sorting with a stable algorithm.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Fri, Nov 11, 2016 at 07:00:17PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 03, 2016 at 02:31:43PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.
> > 
> > Adding new mode flags willy nilly breaks existing userspace. We need to
> > coordinate this better, potentially with a new client cap that only
> > exposes the aspect ratio flags when userspace is prepared for them
> > (similar to what we do with stereo 3D modes).
> 
> As a demonstration here's the change in the xrandr mode list after doing
> the revert:
> 
>  HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
> -   1920x1080     60.00*+
> -   1920x1080i    60.00    50.00  
> +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98  
> +   1920x1080i    60.00    50.00    59.94  
>     1600x1200     60.00  
>     1680x1050     59.88  
>     1280x1024     75.02    60.02  
> @@ -13,30 +13,29 @@
>     1360x768      60.02  
>     1280x800      59.91  
>     1152x864      75.00  
> -   1280x720      60.00    50.00  
> +   1280x720      60.00    50.00    59.94  
>     1024x768      75.03    70.07    60.00  
>     832x624       74.55  
>     800x600       72.19    75.00    60.32  
> -   640x480       75.00    72.81    66.67    59.94  
> +   720x576       50.00  
> +   720x480       60.00    59.94  
> +   640x480       75.00    72.81    66.67    60.00    59.94  
>     720x400       70.08  
> 
> This was with sna, which does this:
>  #define KNOWN_MODE_FLAGS ((1<<14)-1)
>  if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
>  	mode->status = MODE_BAD; /* unknown flags => unhandled */
> so all the modes with an aspect ratio just vanished.
> 
> -modesetting and -ati on the other hand just copy over the unknown
> bits into the xrandr mode structure, which sounds dubious at best:
>  mode->Flags = kmode->flags; //& FLAG_BITS;
> I've not checked what damage it can actually cause.
> 
> 
> It looks like a few modes disappeared from the kernel's mode list
> as well, presumably because some cea modes in the list originated from
> DTDs and whanot so they don't have an aspect ratio and that causes 
> add_alternate_cea_modes() to ignore them. So not populating an aspect
> ratio for cea modes originating from a source other than 
> edid_cea_modes[] looks like another bug to me as well.

Oh and I guess this is also the reason most people didn't notice
anything wrong. The preferred mode usually (or maybe always?) comes from
some other source than edid_cea_modes[] and hence doesn't tend to
go AWOL.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Alex Deucher]

On Fri, Nov 11, 2016 at 12:00 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Nov 03, 2016 at 02:31:43PM +0200, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.
>>
>> Adding new mode flags willy nilly breaks existing userspace. We need to
>> coordinate this better, potentially with a new client cap that only
>> exposes the aspect ratio flags when userspace is prepared for them
>> (similar to what we do with stereo 3D modes).
>
> As a demonstration here's the change in the xrandr mode list after doing
> the revert:
>
>  HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
> -   1920x1080     60.00*+
> -   1920x1080i    60.00    50.00
> +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98
> +   1920x1080i    60.00    50.00    59.94
>     1600x1200     60.00
>     1680x1050     59.88
>     1280x1024     75.02    60.02
> @@ -13,30 +13,29 @@
>     1360x768      60.02
>     1280x800      59.91
>     1152x864      75.00
> -   1280x720      60.00    50.00
> +   1280x720      60.00    50.00    59.94
>     1024x768      75.03    70.07    60.00
>     832x624       74.55
>     800x600       72.19    75.00    60.32
> -   640x480       75.00    72.81    66.67    59.94
> +   720x576       50.00
> +   720x480       60.00    59.94
> +   640x480       75.00    72.81    66.67    60.00    59.94
>     720x400       70.08
>
> This was with sna, which does this:
>  #define KNOWN_MODE_FLAGS ((1<<14)-1)
>  if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
>         mode->status = MODE_BAD; /* unknown flags => unhandled */
> so all the modes with an aspect ratio just vanished.
>
> -modesetting and -ati on the other hand just copy over the unknown
> bits into the xrandr mode structure, which sounds dubious at best:
>  mode->Flags = kmode->flags; //& FLAG_BITS;
> I've not checked what damage it can actually cause.

What problems could this cause?  Presumably the kernel driver has
validated the modes already or they wouldn't showing up in the first
place.  Or is your concern that something in the xserver itself may
barf on the new flags?

Alex
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Daniel Vetter]

On Fri, Nov 11, 2016 at 6:07 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>> This was with sna, which does this:
>>  #define KNOWN_MODE_FLAGS ((1<<14)-1)
>>  if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
>>         mode->status = MODE_BAD; /* unknown flags => unhandled */
>> so all the modes with an aspect ratio just vanished.
>>
>> -modesetting and -ati on the other hand just copy over the unknown
>> bits into the xrandr mode structure, which sounds dubious at best:
>>  mode->Flags = kmode->flags; //& FLAG_BITS;
>> I've not checked what damage it can actually cause.
>
> What problems could this cause?  Presumably the kernel driver has
> validated the modes already or they wouldn't showing up in the first
> place.  Or is your concern that something in the xserver itself may
> barf on the new flags?

See above snipped from sna, we now lost a bunch of modes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Fri, Nov 11, 2016 at 12:07:29PM -0500, Alex Deucher wrote:
> On Fri, Nov 11, 2016 at 12:00 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Nov 03, 2016 at 02:31:43PM +0200, ville.syrjala@linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.
> >>
> >> Adding new mode flags willy nilly breaks existing userspace. We need to
> >> coordinate this better, potentially with a new client cap that only
> >> exposes the aspect ratio flags when userspace is prepared for them
> >> (similar to what we do with stereo 3D modes).
> >
> > As a demonstration here's the change in the xrandr mode list after doing
> > the revert:
> >
> >  HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
> > -   1920x1080     60.00*+
> > -   1920x1080i    60.00    50.00
> > +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98
> > +   1920x1080i    60.00    50.00    59.94
> >     1600x1200     60.00
> >     1680x1050     59.88
> >     1280x1024     75.02    60.02
> > @@ -13,30 +13,29 @@
> >     1360x768      60.02
> >     1280x800      59.91
> >     1152x864      75.00
> > -   1280x720      60.00    50.00
> > +   1280x720      60.00    50.00    59.94
> >     1024x768      75.03    70.07    60.00
> >     832x624       74.55
> >     800x600       72.19    75.00    60.32
> > -   640x480       75.00    72.81    66.67    59.94
> > +   720x576       50.00
> > +   720x480       60.00    59.94
> > +   640x480       75.00    72.81    66.67    60.00    59.94
> >     720x400       70.08
> >
> > This was with sna, which does this:
> >  #define KNOWN_MODE_FLAGS ((1<<14)-1)
> >  if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
> >         mode->status = MODE_BAD; /* unknown flags => unhandled */
> > so all the modes with an aspect ratio just vanished.
> >
> > -modesetting and -ati on the other hand just copy over the unknown
> > bits into the xrandr mode structure, which sounds dubious at best:
> >  mode->Flags = kmode->flags; //& FLAG_BITS;
> > I've not checked what damage it can actually cause.
> 
> What problems could this cause?

As I wrote, I haven't analyzed the potential damage from this. Either
something in the server might go wonky, or maybe we even leak that stuff
over the protocol all the way to the clients? Not sure.

> Presumably the kernel driver has
> validated the modes already or they wouldn't showing up in the first
> place.  Or is your concern that something in the xserver itself may
> barf on the new flags?
> 
> Alex

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

Regards
Shashank
> the revert:
>
>   HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
> -   1920x1080     60.00*+
> -   1920x1080i    60.00    50.00
> +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98
> +   1920x1080i    60.00    50.00    59.94
>      1600x1200     60.00
>      1680x1050     59.88
>      1280x1024     75.02    60.02
> @@ -13,30 +13,29 @@
>      1360x768      60.02
>      1280x800      59.91
>      1152x864      75.00
> -   1280x720      60.00    50.00
> +   1280x720      60.00    50.00    59.94
>      1024x768      75.03    70.07    60.00
>      832x624       74.55
>      800x600       72.19    75.00    60.32
> -   640x480       75.00    72.81    66.67    59.94
> +   720x576       50.00
> +   720x480       60.00    59.94
> +   640x480       75.00    72.81    66.67    60.00    59.94
>      720x400       70.08
None of these aspect ratios are new modes / new aspect ratios from HDMI 
2.0/CEA-861-F
These are the existing modes, and should be independent of reverted 
patches.
> This was with sna, which does this:
>   #define KNOWN_MODE_FLAGS ((1<<14)-1)
>   if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
>   	mode->status = MODE_BAD; /* unknown flags => unhandled */
> so all the modes with an aspect ratio just vanished.
>
> -modesetting and -ati on the other hand just copy over the unknown
> bits into the xrandr mode structure, which sounds dubious at best:
>   mode->Flags = kmode->flags; //& FLAG_BITS;
> I've not checked what damage it can actually cause.
>
>
> It looks like a few modes disappeared from the kernel's mode list
> as well, presumably because some cea modes in the list originated from
> DTDs and whanot so they don't have an aspect ratio and that causes
> add_alternate_cea_modes() to ignore them. So not populating an aspect
> ratio for cea modes originating from a source other than
> edid_cea_modes[] looks like another bug to me as well.
I am writing a patch series to cap the aspect ratio implementation under 
a drm_cap_hdmi2_aspect_ratios
This is how its going to work (inspired from the 2D/stereo series from 
damien L)

- Add a new capability hdmi2_ar
- by default parsing the new hdmi 2.0 aspect ratio will be disabled 
under check of this cap
- during bootup time, while initializing the display, a userspace can 
get_cap on the hdmi2_aspect_ratio
- If it wants HDMI 2.0 aspect ratio support, it will set the cap, and 
kernel will expose these aspect ratios
> Another bug I think might be the ordering of the modes with aspect ratio
> specified. IIRC the spec says that the preferred aspect ratio should be
> listed first in the EDID, but I don't think we preserve that ordering
> in the final mode list. I guess we could fix that by somehow noting
> which aspect ratio is preferred and sort based on that, or we try to
> preserve the order from the EDID until we're ready to sort, and then do
> the sorting with a stable algorithm.
AFAIK The mode order and priority is decided and arranged in userspace, 
based on various factors like
- preferred mode.
- previously applied mode in previous sessions (like for android tvs)
- Bigger h/w vs better refresh rate ?
- Xserver applies its own algorithms to decide which mode should be 
shown first.

I dont think kernel needs to bother about it.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Mon, Nov 14, 2016 at 08:14:34PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> > the revert:
> >
> >   HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
> > -   1920x1080     60.00*+
> > -   1920x1080i    60.00    50.00
> > +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98
> > +   1920x1080i    60.00    50.00    59.94
> >      1600x1200     60.00
> >      1680x1050     59.88
> >      1280x1024     75.02    60.02
> > @@ -13,30 +13,29 @@
> >      1360x768      60.02
> >      1280x800      59.91
> >      1152x864      75.00
> > -   1280x720      60.00    50.00
> > +   1280x720      60.00    50.00    59.94
> >      1024x768      75.03    70.07    60.00
> >      832x624       74.55
> >      800x600       72.19    75.00    60.32
> > -   640x480       75.00    72.81    66.67    59.94
> > +   720x576       50.00
> > +   720x480       60.00    59.94
> > +   640x480       75.00    72.81    66.67    60.00    59.94
> >      720x400       70.08
> None of these aspect ratios are new modes / new aspect ratios from HDMI 
> 2.0/CEA-861-F
> These are the existing modes, and should be independent of reverted 
> patches.

They're affected because your patches changed them by adding the aspect
ratio flags to them.

> > This was with sna, which does this:
> >   #define KNOWN_MODE_FLAGS ((1<<14)-1)
> >   if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
> >   	mode->status = MODE_BAD; /* unknown flags => unhandled */
> > so all the modes with an aspect ratio just vanished.
> >
> > -modesetting and -ati on the other hand just copy over the unknown
> > bits into the xrandr mode structure, which sounds dubious at best:
> >   mode->Flags = kmode->flags; //& FLAG_BITS;
> > I've not checked what damage it can actually cause.
> >
> >
> > It looks like a few modes disappeared from the kernel's mode list
> > as well, presumably because some cea modes in the list originated from
> > DTDs and whanot so they don't have an aspect ratio and that causes
> > add_alternate_cea_modes() to ignore them. So not populating an aspect
> > ratio for cea modes originating from a source other than
> > edid_cea_modes[] looks like another bug to me as well.
> I am writing a patch series to cap the aspect ratio implementation under 
> a drm_cap_hdmi2_aspect_ratios
> This is how its going to work (inspired from the 2D/stereo series from 
> damien L)
> 
> - Add a new capability hdmi2_ar

It should be just a generic "expose aspect ratio flags to userspace?"

> - by default parsing the new hdmi 2.0 aspect ratio will be disabled 
> under check of this cap
> - during bootup time, while initializing the display, a userspace can 
> get_cap on the hdmi2_aspect_ratio
> - If it wants HDMI 2.0 aspect ratio support, it will set the cap, and 
> kernel will expose these aspect ratios
> > Another bug I think might be the ordering of the modes with aspect ratio
> > specified. IIRC the spec says that the preferred aspect ratio should be
> > listed first in the EDID, but I don't think we preserve that ordering
> > in the final mode list. I guess we could fix that by somehow noting
> > which aspect ratio is preferred and sort based on that, or we try to
> > preserve the order from the EDID until we're ready to sort, and then do
> > the sorting with a stable algorithm.
> AFAIK The mode order and priority is decided and arranged in userspace, 
> based on various factors like
> - preferred mode.
> - previously applied mode in previous sessions (like for android tvs)
> - Bigger h/w vs better refresh rate ?
> - Xserver applies its own algorithms to decide which mode should be 
> shown first.

Xorg does sort on its own. But since it doesn't know anything about
aspect ratios and whatnot I wouldn't rely on that for anything. I
also wouldn't expect eg. wayland compositors to do their own sorting.
And yeah, looks like weston at least doesn't do any sorting whatsoever.

> 
> I dont think kernel needs to bother about it.

So I'm going to say that we in fact do need to bother.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

Regards

Shashank


On 11/14/2016 9:19 PM, Ville Syrjälä wrote:
> On Mon, Nov 14, 2016 at 08:14:34PM +0530, Sharma, Shashank wrote:
>> Regards
>> Shashank
>>> the revert:
>>>
>>>    HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
>>> -   1920x1080     60.00*+
>>> -   1920x1080i    60.00    50.00
>>> +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98
>>> +   1920x1080i    60.00    50.00    59.94
>>>       1600x1200     60.00
>>>       1680x1050     59.88
>>>       1280x1024     75.02    60.02
>>> @@ -13,30 +13,29 @@
>>>       1360x768      60.02
>>>       1280x800      59.91
>>>       1152x864      75.00
>>> -   1280x720      60.00    50.00
>>> +   1280x720      60.00    50.00    59.94
>>>       1024x768      75.03    70.07    60.00
>>>       832x624       74.55
>>>       800x600       72.19    75.00    60.32
>>> -   640x480       75.00    72.81    66.67    59.94
>>> +   720x576       50.00
>>> +   720x480       60.00    59.94
>>> +   640x480       75.00    72.81    66.67    60.00    59.94
>>>       720x400       70.08
>> None of these aspect ratios are new modes / new aspect ratios from HDMI
>> 2.0/CEA-861-F
>> These are the existing modes, and should be independent of reverted
>> patches.
> They're affected because your patches changed them by adding the aspect
> ratio flags to them.
Yes, But they are independent of reverted patch, which adds aspect ratio 
for HDMI 2.0 ratios (64:27 and 256:135)
>>> This was with sna, which does this:
>>>    #define KNOWN_MODE_FLAGS ((1<<14)-1)
>>>    if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
>>>    	mode->status = MODE_BAD; /* unknown flags => unhandled */
>>> so all the modes with an aspect ratio just vanished.
>>>
>>> -modesetting and -ati on the other hand just copy over the unknown
>>> bits into the xrandr mode structure, which sounds dubious at best:
>>>    mode->Flags = kmode->flags; //& FLAG_BITS;
>>> I've not checked what damage it can actually cause.
>>>
>>>
>>> It looks like a few modes disappeared from the kernel's mode list
>>> as well, presumably because some cea modes in the list originated from
>>> DTDs and whanot so they don't have an aspect ratio and that causes
>>> add_alternate_cea_modes() to ignore them. So not populating an aspect
>>> ratio for cea modes originating from a source other than
>>> edid_cea_modes[] looks like another bug to me as well.
>> I am writing a patch series to cap the aspect ratio implementation under
>> a drm_cap_hdmi2_aspect_ratios
>> This is how its going to work (inspired from the 2D/stereo series from
>> damien L)
>>
>> - Add a new capability hdmi2_ar
> It should be just a generic "expose aspect ratio flags to userspace?"
Makes sense, in this way we can even revert the aspect_ratio property 
for HDMI connector, as discussed during
the code review sessions of this patch series. In this way, when kernel 
will expose the aspect ratios, it will either
do the aspect ratios as per EDID, or wont.
>
>> - by default parsing the new hdmi 2.0 aspect ratio will be disabled
>> under check of this cap
>> - during bootup time, while initializing the display, a userspace can
>> get_cap on the hdmi2_aspect_ratio
>> - If it wants HDMI 2.0 aspect ratio support, it will set the cap, and
>> kernel will expose these aspect ratios
>>> Another bug I think might be the ordering of the modes with aspect ratio
>>> specified. IIRC the spec says that the preferred aspect ratio should be
>>> listed first in the EDID, but I don't think we preserve that ordering
>>> in the final mode list. I guess we could fix that by somehow noting
>>> which aspect ratio is preferred and sort based on that, or we try to
>>> preserve the order from the EDID until we're ready to sort, and then do
>>> the sorting with a stable algorithm.
>> AFAIK The mode order and priority is decided and arranged in userspace,
>> based on various factors like
>> - preferred mode.
>> - previously applied mode in previous sessions (like for android tvs)
>> - Bigger h/w vs better refresh rate ?
>> - Xserver applies its own algorithms to decide which mode should be
>> shown first.
> Xorg does sort on its own. But since it doesn't know anything about
> aspect ratios and whatnot I wouldn't rely on that for anything. I
> also wouldn't expect eg. wayland compositors to do their own sorting.
> And yeah, looks like weston at least doesn't do any sorting whatsoever.
>
>> I dont think kernel needs to bother about it.
> So I'm going to say that we in fact do need to bother.
>
IMHO, making policies for UI is not a part of kernel design, a UI 
manager (Hardware composed, X or Wayland) should take care of it, as
they have access to much information (Like previously applied mode, user 
preference etc). When it comes to sorting of modes, the only general rule
across drivers like FB, V4L2, I have seen is the first mode in the list 
should be preferred mode, which we are still keeping. And after that our 
probed_modes were
anyways not sorted now, so it doesn't matter further.

If X server doesn't know what to do with aspect ratio flags, it can 
chose not to set the cap, and if HWC knows, it can chose to set. This is 
the same situation as 2D stereo modes
which are existing already.

Regards
Shashank
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Mon, Nov 14, 2016 at 09:37:18PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/14/2016 9:19 PM, Ville Syrjälä wrote:
> > On Mon, Nov 14, 2016 at 08:14:34PM +0530, Sharma, Shashank wrote:
> >> Regards
> >> Shashank
> >>> the revert:
> >>>
> >>>    HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
> >>> -   1920x1080     60.00*+
> >>> -   1920x1080i    60.00    50.00
> >>> +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98
> >>> +   1920x1080i    60.00    50.00    59.94
> >>>       1600x1200     60.00
> >>>       1680x1050     59.88
> >>>       1280x1024     75.02    60.02
> >>> @@ -13,30 +13,29 @@
> >>>       1360x768      60.02
> >>>       1280x800      59.91
> >>>       1152x864      75.00
> >>> -   1280x720      60.00    50.00
> >>> +   1280x720      60.00    50.00    59.94
> >>>       1024x768      75.03    70.07    60.00
> >>>       832x624       74.55
> >>>       800x600       72.19    75.00    60.32
> >>> -   640x480       75.00    72.81    66.67    59.94
> >>> +   720x576       50.00
> >>> +   720x480       60.00    59.94
> >>> +   640x480       75.00    72.81    66.67    60.00    59.94
> >>>       720x400       70.08
> >> None of these aspect ratios are new modes / new aspect ratios from HDMI
> >> 2.0/CEA-861-F
> >> These are the existing modes, and should be independent of reverted
> >> patches.
> > They're affected because your patches changed them by adding the aspect
> > ratio flags to them.
> Yes, But they are independent of reverted patch, which adds aspect ratio 
> for HDMI 2.0 ratios (64:27 and 256:135)

The second patch had to be reverted so that the first patch would revert
cleanly.

> >>> This was with sna, which does this:
> >>>    #define KNOWN_MODE_FLAGS ((1<<14)-1)
> >>>    if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
> >>>    	mode->status = MODE_BAD; /* unknown flags => unhandled */
> >>> so all the modes with an aspect ratio just vanished.
> >>>
> >>> -modesetting and -ati on the other hand just copy over the unknown
> >>> bits into the xrandr mode structure, which sounds dubious at best:
> >>>    mode->Flags = kmode->flags; //& FLAG_BITS;
> >>> I've not checked what damage it can actually cause.
> >>>
> >>>
> >>> It looks like a few modes disappeared from the kernel's mode list
> >>> as well, presumably because some cea modes in the list originated from
> >>> DTDs and whanot so they don't have an aspect ratio and that causes
> >>> add_alternate_cea_modes() to ignore them. So not populating an aspect
> >>> ratio for cea modes originating from a source other than
> >>> edid_cea_modes[] looks like another bug to me as well.
> >> I am writing a patch series to cap the aspect ratio implementation under
> >> a drm_cap_hdmi2_aspect_ratios
> >> This is how its going to work (inspired from the 2D/stereo series from
> >> damien L)
> >>
> >> - Add a new capability hdmi2_ar
> > It should be just a generic "expose aspect ratio flags to userspace?"
> Makes sense, in this way we can even revert the aspect_ratio property 
> for HDMI connector, as discussed during
> the code review sessions of this patch series. In this way, when kernel 
> will expose the aspect ratios, it will either
> do the aspect ratios as per EDID, or wont.
> >
> >> - by default parsing the new hdmi 2.0 aspect ratio will be disabled
> >> under check of this cap
> >> - during bootup time, while initializing the display, a userspace can
> >> get_cap on the hdmi2_aspect_ratio
> >> - If it wants HDMI 2.0 aspect ratio support, it will set the cap, and
> >> kernel will expose these aspect ratios
> >>> Another bug I think might be the ordering of the modes with aspect ratio
> >>> specified. IIRC the spec says that the preferred aspect ratio should be
> >>> listed first in the EDID, but I don't think we preserve that ordering
> >>> in the final mode list. I guess we could fix that by somehow noting
> >>> which aspect ratio is preferred and sort based on that, or we try to
> >>> preserve the order from the EDID until we're ready to sort, and then do
> >>> the sorting with a stable algorithm.
> >> AFAIK The mode order and priority is decided and arranged in userspace,
> >> based on various factors like
> >> - preferred mode.
> >> - previously applied mode in previous sessions (like for android tvs)
> >> - Bigger h/w vs better refresh rate ?
> >> - Xserver applies its own algorithms to decide which mode should be
> >> shown first.
> > Xorg does sort on its own. But since it doesn't know anything about
> > aspect ratios and whatnot I wouldn't rely on that for anything. I
> > also wouldn't expect eg. wayland compositors to do their own sorting.
> > And yeah, looks like weston at least doesn't do any sorting whatsoever.
> >
> >> I dont think kernel needs to bother about it.
> > So I'm going to say that we in fact do need to bother.
> >
> IMHO, making policies for UI is not a part of kernel design, a UI 
> manager (Hardware composed, X or Wayland) should take care of it, as
> they have access to much information (Like previously applied mode, user 
> preference etc). When it comes to sorting of modes, the only general rule
> across drivers like FB, V4L2, I have seen is the first mode in the list 
> should be preferred mode, which we are still keeping. And after that our 
> probed_modes were
> anyways not sorted now, so it doesn't matter further.

Having userspace be responsible for sorting the aspect ratios would
perhaps require that userspace parses the EDID, which is pretty crazy.
I guess it could try to deduce something from the physical aspect ratio
of the display, but I'm not sure that's quite what we want either.

Also we already sort the modes in the kernel anyway, so it's not like
we'd be doing something new by also considering the aspect ratios.
I would at the very least want to avoid a totally random order between
modes that differ only by the aspect ratio.

> 
> If X server doesn't know what to do with aspect ratio flags, it can 
> chose not to set the cap, and if HWC knows, it can chose to set. This is 
> the same situation as 2D stereo modes
> which are existing already.
> 
> Regards
> Shashank

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

Regards

Shashank


On 11/14/2016 9:50 PM, Ville Syrjälä wrote:
> On Mon, Nov 14, 2016 at 09:37:18PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 11/14/2016 9:19 PM, Ville Syrjälä wrote:
>>> On Mon, Nov 14, 2016 at 08:14:34PM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>> Shashank
>>>>> the revert:
>>>>>
>>>>>     HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
>>>>> -   1920x1080     60.00*+
>>>>> -   1920x1080i    60.00    50.00
>>>>> +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98
>>>>> +   1920x1080i    60.00    50.00    59.94
>>>>>        1600x1200     60.00
>>>>>        1680x1050     59.88
>>>>>        1280x1024     75.02    60.02
>>>>> @@ -13,30 +13,29 @@
>>>>>        1360x768      60.02
>>>>>        1280x800      59.91
>>>>>        1152x864      75.00
>>>>> -   1280x720      60.00    50.00
>>>>> +   1280x720      60.00    50.00    59.94
>>>>>        1024x768      75.03    70.07    60.00
>>>>>        832x624       74.55
>>>>>        800x600       72.19    75.00    60.32
>>>>> -   640x480       75.00    72.81    66.67    59.94
>>>>> +   720x576       50.00
>>>>> +   720x480       60.00    59.94
>>>>> +   640x480       75.00    72.81    66.67    60.00    59.94
>>>>>        720x400       70.08
>>>> None of these aspect ratios are new modes / new aspect ratios from HDMI
>>>> 2.0/CEA-861-F
>>>> These are the existing modes, and should be independent of reverted
>>>> patches.
>>> They're affected because your patches changed them by adding the aspect
>>> ratio flags to them.
>> Yes, But they are independent of reverted patch, which adds aspect ratio
>> for HDMI 2.0 ratios (64:27 and 256:135)
> The second patch had to be reverted so that the first patch would revert
> cleanly.
>
>>>>> This was with sna, which does this:
>>>>>     #define KNOWN_MODE_FLAGS ((1<<14)-1)
>>>>>     if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
>>>>>     	mode->status = MODE_BAD; /* unknown flags => unhandled */
>>>>> so all the modes with an aspect ratio just vanished.
>>>>>
>>>>> -modesetting and -ati on the other hand just copy over the unknown
>>>>> bits into the xrandr mode structure, which sounds dubious at best:
>>>>>     mode->Flags = kmode->flags; //& FLAG_BITS;
>>>>> I've not checked what damage it can actually cause.
>>>>>
>>>>>
>>>>> It looks like a few modes disappeared from the kernel's mode list
>>>>> as well, presumably because some cea modes in the list originated from
>>>>> DTDs and whanot so they don't have an aspect ratio and that causes
>>>>> add_alternate_cea_modes() to ignore them. So not populating an aspect
>>>>> ratio for cea modes originating from a source other than
>>>>> edid_cea_modes[] looks like another bug to me as well.
>>>> I am writing a patch series to cap the aspect ratio implementation under
>>>> a drm_cap_hdmi2_aspect_ratios
>>>> This is how its going to work (inspired from the 2D/stereo series from
>>>> damien L)
>>>>
>>>> - Add a new capability hdmi2_ar
>>> It should be just a generic "expose aspect ratio flags to userspace?"
>> Makes sense, in this way we can even revert the aspect_ratio property
>> for HDMI connector, as discussed during
>> the code review sessions of this patch series. In this way, when kernel
>> will expose the aspect ratios, it will either
>> do the aspect ratios as per EDID, or wont.
>>>> - by default parsing the new hdmi 2.0 aspect ratio will be disabled
>>>> under check of this cap
>>>> - during bootup time, while initializing the display, a userspace can
>>>> get_cap on the hdmi2_aspect_ratio
>>>> - If it wants HDMI 2.0 aspect ratio support, it will set the cap, and
>>>> kernel will expose these aspect ratios
>>>>> Another bug I think might be the ordering of the modes with aspect ratio
>>>>> specified. IIRC the spec says that the preferred aspect ratio should be
>>>>> listed first in the EDID, but I don't think we preserve that ordering
>>>>> in the final mode list. I guess we could fix that by somehow noting
>>>>> which aspect ratio is preferred and sort based on that, or we try to
>>>>> preserve the order from the EDID until we're ready to sort, and then do
>>>>> the sorting with a stable algorithm.
>>>> AFAIK The mode order and priority is decided and arranged in userspace,
>>>> based on various factors like
>>>> - preferred mode.
>>>> - previously applied mode in previous sessions (like for android tvs)
>>>> - Bigger h/w vs better refresh rate ?
>>>> - Xserver applies its own algorithms to decide which mode should be
>>>> shown first.
>>> Xorg does sort on its own. But since it doesn't know anything about
>>> aspect ratios and whatnot I wouldn't rely on that for anything. I
>>> also wouldn't expect eg. wayland compositors to do their own sorting.
>>> And yeah, looks like weston at least doesn't do any sorting whatsoever.
>>>
>>>> I dont think kernel needs to bother about it.
>>> So I'm going to say that we in fact do need to bother.
>>>
>> IMHO, making policies for UI is not a part of kernel design, a UI
>> manager (Hardware composed, X or Wayland) should take care of it, as
>> they have access to much information (Like previously applied mode, user
>> preference etc). When it comes to sorting of modes, the only general rule
>> across drivers like FB, V4L2, I have seen is the first mode in the list
>> should be preferred mode, which we are still keeping. And after that our
>> probed_modes were
>> anyways not sorted now, so it doesn't matter further.
> Having userspace be responsible for sorting the aspect ratios would
> perhaps require that userspace parses the EDID, which is pretty crazy.
Why ?
userspace has to just set cap for aspect ratio, and kernel can read 
EDID, parse the CEA block, populate the aspect ratios flags
and add the modes (Just what this patch was doing, except the cap part)
Once userspace has the getResources/getConnector call filled, it can 
access all the modes (with and without aspect) and do the sorting
in any way it wants.
> I guess it could try to deduce something from the physical aspect ratio
> of the display, but I'm not sure that's quite what we want either.
>
> Also we already sort the modes in the kernel anyway, so it's not like
> we'd be doing something new by also considering the aspect ratios.
> I would at the very least want to avoid a totally random order between
> modes that differ only by the aspect ratio.
Path: get_connector -> probe_single_connector_mode -> drm_add_edid_modes
Again, IMHO, we don't sort the modes in kernel, we populate modes in a 
particular order, which is:
(From drm_edid.c::drm_add_edid_modes)
##############################################################
/*
      * EDID spec says modes should be preferred in this order:
      * - preferred detailed mode
      * - other detailed modes from base block
      * - detailed modes from extension blocks
      * - CVT 3-byte code modes
      * - standard timing codes
      * - established timing codes
      * - modes inferred from GTF or CVT range information
      *
      * We get this pretty much right.
      *
      * XXX order for additional mode types in extension blocks?
      */
     num_modes += add_detailed_modes(connector, edid, quirks);
     num_modes += add_cvt_modes(connector, edid);
     num_modes += add_standard_modes(connector, edid);
     num_modes += add_established_modes(connector, edid);
     num_modes += add_cea_modes(connector, edid);
     num_modes += add_alternate_cea_modes(connector, edid);
     num_modes += add_displayid_detailed_modes(connector, edid);
###############################################################

Here the modes are added in the connector, in the same order they are 
arranged into their respective blocks in EDID.
But the order to read the block is a preferred order (no sorting).

Now, in this patch series, we are adding aspect ratio information in 
edid_cea_modes db, which is going to affect only 
add_cea/alternate_cea_modes() call, and the modes accordingly.
Please let me know if I misunderstood something here.

Regards
Shashank
>> If X server doesn't know what to do with aspect ratio flags, it can
>> chose not to set the cap, and if HWC knows, it can chose to set. This is
>> the same situation as 2D stereo modes
>> which are existing already.
>>
>> Regards
>> Shashank

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Mon, Nov 14, 2016 at 10:12:04PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/14/2016 9:50 PM, Ville Syrjälä wrote:
> > On Mon, Nov 14, 2016 at 09:37:18PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 11/14/2016 9:19 PM, Ville Syrjälä wrote:
> >>> On Mon, Nov 14, 2016 at 08:14:34PM +0530, Sharma, Shashank wrote:
> >>>> Regards
> >>>> Shashank
> >>>>> the revert:
> >>>>>
> >>>>>     HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
> >>>>> -   1920x1080     60.00*+
> >>>>> -   1920x1080i    60.00    50.00
> >>>>> +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98
> >>>>> +   1920x1080i    60.00    50.00    59.94
> >>>>>        1600x1200     60.00
> >>>>>        1680x1050     59.88
> >>>>>        1280x1024     75.02    60.02
> >>>>> @@ -13,30 +13,29 @@
> >>>>>        1360x768      60.02
> >>>>>        1280x800      59.91
> >>>>>        1152x864      75.00
> >>>>> -   1280x720      60.00    50.00
> >>>>> +   1280x720      60.00    50.00    59.94
> >>>>>        1024x768      75.03    70.07    60.00
> >>>>>        832x624       74.55
> >>>>>        800x600       72.19    75.00    60.32
> >>>>> -   640x480       75.00    72.81    66.67    59.94
> >>>>> +   720x576       50.00
> >>>>> +   720x480       60.00    59.94
> >>>>> +   640x480       75.00    72.81    66.67    60.00    59.94
> >>>>>        720x400       70.08
> >>>> None of these aspect ratios are new modes / new aspect ratios from HDMI
> >>>> 2.0/CEA-861-F
> >>>> These are the existing modes, and should be independent of reverted
> >>>> patches.
> >>> They're affected because your patches changed them by adding the aspect
> >>> ratio flags to them.
> >> Yes, But they are independent of reverted patch, which adds aspect ratio
> >> for HDMI 2.0 ratios (64:27 and 256:135)
> > The second patch had to be reverted so that the first patch would revert
> > cleanly.
> >
> >>>>> This was with sna, which does this:
> >>>>>     #define KNOWN_MODE_FLAGS ((1<<14)-1)
> >>>>>     if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
> >>>>>     	mode->status = MODE_BAD; /* unknown flags => unhandled */
> >>>>> so all the modes with an aspect ratio just vanished.
> >>>>>
> >>>>> -modesetting and -ati on the other hand just copy over the unknown
> >>>>> bits into the xrandr mode structure, which sounds dubious at best:
> >>>>>     mode->Flags = kmode->flags; //& FLAG_BITS;
> >>>>> I've not checked what damage it can actually cause.
> >>>>>
> >>>>>
> >>>>> It looks like a few modes disappeared from the kernel's mode list
> >>>>> as well, presumably because some cea modes in the list originated from
> >>>>> DTDs and whanot so they don't have an aspect ratio and that causes
> >>>>> add_alternate_cea_modes() to ignore them. So not populating an aspect
> >>>>> ratio for cea modes originating from a source other than
> >>>>> edid_cea_modes[] looks like another bug to me as well.
> >>>> I am writing a patch series to cap the aspect ratio implementation under
> >>>> a drm_cap_hdmi2_aspect_ratios
> >>>> This is how its going to work (inspired from the 2D/stereo series from
> >>>> damien L)
> >>>>
> >>>> - Add a new capability hdmi2_ar
> >>> It should be just a generic "expose aspect ratio flags to userspace?"
> >> Makes sense, in this way we can even revert the aspect_ratio property
> >> for HDMI connector, as discussed during
> >> the code review sessions of this patch series. In this way, when kernel
> >> will expose the aspect ratios, it will either
> >> do the aspect ratios as per EDID, or wont.
> >>>> - by default parsing the new hdmi 2.0 aspect ratio will be disabled
> >>>> under check of this cap
> >>>> - during bootup time, while initializing the display, a userspace can
> >>>> get_cap on the hdmi2_aspect_ratio
> >>>> - If it wants HDMI 2.0 aspect ratio support, it will set the cap, and
> >>>> kernel will expose these aspect ratios
> >>>>> Another bug I think might be the ordering of the modes with aspect ratio
> >>>>> specified. IIRC the spec says that the preferred aspect ratio should be
> >>>>> listed first in the EDID, but I don't think we preserve that ordering
> >>>>> in the final mode list. I guess we could fix that by somehow noting
> >>>>> which aspect ratio is preferred and sort based on that, or we try to
> >>>>> preserve the order from the EDID until we're ready to sort, and then do
> >>>>> the sorting with a stable algorithm.
> >>>> AFAIK The mode order and priority is decided and arranged in userspace,
> >>>> based on various factors like
> >>>> - preferred mode.
> >>>> - previously applied mode in previous sessions (like for android tvs)
> >>>> - Bigger h/w vs better refresh rate ?
> >>>> - Xserver applies its own algorithms to decide which mode should be
> >>>> shown first.
> >>> Xorg does sort on its own. But since it doesn't know anything about
> >>> aspect ratios and whatnot I wouldn't rely on that for anything. I
> >>> also wouldn't expect eg. wayland compositors to do their own sorting.
> >>> And yeah, looks like weston at least doesn't do any sorting whatsoever.
> >>>
> >>>> I dont think kernel needs to bother about it.
> >>> So I'm going to say that we in fact do need to bother.
> >>>
> >> IMHO, making policies for UI is not a part of kernel design, a UI
> >> manager (Hardware composed, X or Wayland) should take care of it, as
> >> they have access to much information (Like previously applied mode, user
> >> preference etc). When it comes to sorting of modes, the only general rule
> >> across drivers like FB, V4L2, I have seen is the first mode in the list
> >> should be preferred mode, which we are still keeping. And after that our
> >> probed_modes were
> >> anyways not sorted now, so it doesn't matter further.
> > Having userspace be responsible for sorting the aspect ratios would
> > perhaps require that userspace parses the EDID, which is pretty crazy.
> Why ?
> userspace has to just set cap for aspect ratio, and kernel can read 
> EDID, parse the CEA block, populate the aspect ratios flags
> and add the modes (Just what this patch was doing, except the cap part)
> Once userspace has the getResources/getConnector call filled, it can 
> access all the modes (with and without aspect) and do the sorting
> in any way it wants.
> > I guess it could try to deduce something from the physical aspect ratio
> > of the display, but I'm not sure that's quite what we want either.
> >
> > Also we already sort the modes in the kernel anyway, so it's not like
> > we'd be doing something new by also considering the aspect ratios.
> > I would at the very least want to avoid a totally random order between
> > modes that differ only by the aspect ratio.
> Path: get_connector -> probe_single_connector_mode -> drm_add_edid_modes
> Again, IMHO, we don't sort the modes in kernel, we populate modes in a 
> particular order, which is:
> (From drm_edid.c::drm_add_edid_modes)
> ##############################################################
> /*
>       * EDID spec says modes should be preferred in this order:
>       * - preferred detailed mode
>       * - other detailed modes from base block
>       * - detailed modes from extension blocks
>       * - CVT 3-byte code modes
>       * - standard timing codes
>       * - established timing codes
>       * - modes inferred from GTF or CVT range information
>       *
>       * We get this pretty much right.
>       *
>       * XXX order for additional mode types in extension blocks?
>       */
>      num_modes += add_detailed_modes(connector, edid, quirks);
>      num_modes += add_cvt_modes(connector, edid);
>      num_modes += add_standard_modes(connector, edid);
>      num_modes += add_established_modes(connector, edid);
>      num_modes += add_cea_modes(connector, edid);
>      num_modes += add_alternate_cea_modes(connector, edid);
>      num_modes += add_displayid_detailed_modes(connector, edid);
> ###############################################################
> 
> Here the modes are added in the connector, in the same order they are 
> arranged into their respective blocks in EDID.
> But the order to read the block is a preferred order (no sorting).
> 
> Now, in this patch series, we are adding aspect ratio information in 
> edid_cea_modes db, which is going to affect only 
> add_cea/alternate_cea_modes() call, and the modes accordingly.
> Please let me know if I misunderstood something here.

We explicitly sort the modes after this.

> 
> Regards
> Shashank
> >> If X server doesn't know what to do with aspect ratio flags, it can
> >> chose not to set the cap, and if HWC knows, it can chose to set. This is
> >> the same situation as 2D stereo modes
> >> which are existing already.
> >>
> >> Regards
> >> Shashank

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

On 11/14/2016 10:15 PM, Ville Syrjälä wrote:
> On Mon, Nov 14, 2016 at 10:12:04PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 11/14/2016 9:50 PM, Ville Syrjälä wrote:
>>> On Mon, Nov 14, 2016 at 09:37:18PM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 11/14/2016 9:19 PM, Ville Syrjälä wrote:
>>>>> On Mon, Nov 14, 2016 at 08:14:34PM +0530, Sharma, Shashank wrote:
>>>>>> Regards
>>>>>> Shashank
>>>>>>> the revert:
>>>>>>>
>>>>>>>      HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
>>>>>>> -   1920x1080     60.00*+
>>>>>>> -   1920x1080i    60.00    50.00
>>>>>>> +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98
>>>>>>> +   1920x1080i    60.00    50.00    59.94
>>>>>>>         1600x1200     60.00
>>>>>>>         1680x1050     59.88
>>>>>>>         1280x1024     75.02    60.02
>>>>>>> @@ -13,30 +13,29 @@
>>>>>>>         1360x768      60.02
>>>>>>>         1280x800      59.91
>>>>>>>         1152x864      75.00
>>>>>>> -   1280x720      60.00    50.00
>>>>>>> +   1280x720      60.00    50.00    59.94
>>>>>>>         1024x768      75.03    70.07    60.00
>>>>>>>         832x624       74.55
>>>>>>>         800x600       72.19    75.00    60.32
>>>>>>> -   640x480       75.00    72.81    66.67    59.94
>>>>>>> +   720x576       50.00
>>>>>>> +   720x480       60.00    59.94
>>>>>>> +   640x480       75.00    72.81    66.67    60.00    59.94
>>>>>>>         720x400       70.08
>>>>>> None of these aspect ratios are new modes / new aspect ratios from HDMI
>>>>>> 2.0/CEA-861-F
>>>>>> These are the existing modes, and should be independent of reverted
>>>>>> patches.
>>>>> They're affected because your patches changed them by adding the aspect
>>>>> ratio flags to them.
>>>> Yes, But they are independent of reverted patch, which adds aspect ratio
>>>> for HDMI 2.0 ratios (64:27 and 256:135)
>>> The second patch had to be reverted so that the first patch would revert
>>> cleanly.
>>>
>>>>>>> This was with sna, which does this:
>>>>>>>      #define KNOWN_MODE_FLAGS ((1<<14)-1)
>>>>>>>      if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
>>>>>>>      	mode->status = MODE_BAD; /* unknown flags => unhandled */
>>>>>>> so all the modes with an aspect ratio just vanished.
>>>>>>>
>>>>>>> -modesetting and -ati on the other hand just copy over the unknown
>>>>>>> bits into the xrandr mode structure, which sounds dubious at best:
>>>>>>>      mode->Flags = kmode->flags; //& FLAG_BITS;
>>>>>>> I've not checked what damage it can actually cause.
>>>>>>>
>>>>>>>
>>>>>>> It looks like a few modes disappeared from the kernel's mode list
>>>>>>> as well, presumably because some cea modes in the list originated from
>>>>>>> DTDs and whanot so they don't have an aspect ratio and that causes
>>>>>>> add_alternate_cea_modes() to ignore them. So not populating an aspect
>>>>>>> ratio for cea modes originating from a source other than
>>>>>>> edid_cea_modes[] looks like another bug to me as well.
>>>>>> I am writing a patch series to cap the aspect ratio implementation under
>>>>>> a drm_cap_hdmi2_aspect_ratios
>>>>>> This is how its going to work (inspired from the 2D/stereo series from
>>>>>> damien L)
>>>>>>
>>>>>> - Add a new capability hdmi2_ar
>>>>> It should be just a generic "expose aspect ratio flags to userspace?"
>>>> Makes sense, in this way we can even revert the aspect_ratio property
>>>> for HDMI connector, as discussed during
>>>> the code review sessions of this patch series. In this way, when kernel
>>>> will expose the aspect ratios, it will either
>>>> do the aspect ratios as per EDID, or wont.
>>>>>> - by default parsing the new hdmi 2.0 aspect ratio will be disabled
>>>>>> under check of this cap
>>>>>> - during bootup time, while initializing the display, a userspace can
>>>>>> get_cap on the hdmi2_aspect_ratio
>>>>>> - If it wants HDMI 2.0 aspect ratio support, it will set the cap, and
>>>>>> kernel will expose these aspect ratios
>>>>>>> Another bug I think might be the ordering of the modes with aspect ratio
>>>>>>> specified. IIRC the spec says that the preferred aspect ratio should be
>>>>>>> listed first in the EDID, but I don't think we preserve that ordering
>>>>>>> in the final mode list. I guess we could fix that by somehow noting
>>>>>>> which aspect ratio is preferred and sort based on that, or we try to
>>>>>>> preserve the order from the EDID until we're ready to sort, and then do
>>>>>>> the sorting with a stable algorithm.
>>>>>> AFAIK The mode order and priority is decided and arranged in userspace,
>>>>>> based on various factors like
>>>>>> - preferred mode.
>>>>>> - previously applied mode in previous sessions (like for android tvs)
>>>>>> - Bigger h/w vs better refresh rate ?
>>>>>> - Xserver applies its own algorithms to decide which mode should be
>>>>>> shown first.
>>>>> Xorg does sort on its own. But since it doesn't know anything about
>>>>> aspect ratios and whatnot I wouldn't rely on that for anything. I
>>>>> also wouldn't expect eg. wayland compositors to do their own sorting.
>>>>> And yeah, looks like weston at least doesn't do any sorting whatsoever.
>>>>>
>>>>>> I dont think kernel needs to bother about it.
>>>>> So I'm going to say that we in fact do need to bother.
>>>>>
>>>> IMHO, making policies for UI is not a part of kernel design, a UI
>>>> manager (Hardware composed, X or Wayland) should take care of it, as
>>>> they have access to much information (Like previously applied mode, user
>>>> preference etc). When it comes to sorting of modes, the only general rule
>>>> across drivers like FB, V4L2, I have seen is the first mode in the list
>>>> should be preferred mode, which we are still keeping. And after that our
>>>> probed_modes were
>>>> anyways not sorted now, so it doesn't matter further.
>>> Having userspace be responsible for sorting the aspect ratios would
>>> perhaps require that userspace parses the EDID, which is pretty crazy.
>> Why ?
>> userspace has to just set cap for aspect ratio, and kernel can read
>> EDID, parse the CEA block, populate the aspect ratios flags
>> and add the modes (Just what this patch was doing, except the cap part)
>> Once userspace has the getResources/getConnector call filled, it can
>> access all the modes (with and without aspect) and do the sorting
>> in any way it wants.
>>> I guess it could try to deduce something from the physical aspect ratio
>>> of the display, but I'm not sure that's quite what we want either.
>>>
>>> Also we already sort the modes in the kernel anyway, so it's not like
>>> we'd be doing something new by also considering the aspect ratios.
>>> I would at the very least want to avoid a totally random order between
>>> modes that differ only by the aspect ratio.
>> Path: get_connector -> probe_single_connector_mode -> drm_add_edid_modes
>> Again, IMHO, we don't sort the modes in kernel, we populate modes in a
>> particular order, which is:
>> (From drm_edid.c::drm_add_edid_modes)
>> ##############################################################
>> /*
>>        * EDID spec says modes should be preferred in this order:
>>        * - preferred detailed mode
>>        * - other detailed modes from base block
>>        * - detailed modes from extension blocks
>>        * - CVT 3-byte code modes
>>        * - standard timing codes
>>        * - established timing codes
>>        * - modes inferred from GTF or CVT range information
>>        *
>>        * We get this pretty much right.
>>        *
>>        * XXX order for additional mode types in extension blocks?
>>        */
>>       num_modes += add_detailed_modes(connector, edid, quirks);
>>       num_modes += add_cvt_modes(connector, edid);
>>       num_modes += add_standard_modes(connector, edid);
>>       num_modes += add_established_modes(connector, edid);
>>       num_modes += add_cea_modes(connector, edid);
>>       num_modes += add_alternate_cea_modes(connector, edid);
>>       num_modes += add_displayid_detailed_modes(connector, edid);
>> ###############################################################
>>
>> Here the modes are added in the connector, in the same order they are
>> arranged into their respective blocks in EDID.
>> But the order to read the block is a preferred order (no sorting).
>>
>> Now, in this patch series, we are adding aspect ratio information in
>> edid_cea_modes db, which is going to affect only
>> add_cea/alternate_cea_modes() call, and the modes accordingly.
>> Please let me know if I misunderstood something here.
> We explicitly sort the modes after this.
>   

In any case, I guess addition of a cap for aspect ratio should fix the 
current objections for this implementation.

And I will keep it 0 by default, so that no aspect ratio information is 
added until userspace sets the cap to 1 on its own.


Regards

Shashank

>> Regards
>> Shashank
>>>> If X server doesn't know what to do with aspect ratio flags, it can
>>>> chose not to set the cap, and if HWC knows, it can chose to set. This is
>>>> the same situation as 2D stereo modes
>>>> which are existing already.
>>>>
>>>> Regards
>>>> Shashank

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Daniel Vetter]

On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
> On 11/14/2016 10:15 PM, Ville Syrjälä wrote:
> > On Mon, Nov 14, 2016 at 10:12:04PM +0530, Sharma, Shashank wrote:
> > > Regards
> > > 
> > > Shashank
> > > 
> > > 
> > > On 11/14/2016 9:50 PM, Ville Syrjälä wrote:
> > > > On Mon, Nov 14, 2016 at 09:37:18PM +0530, Sharma, Shashank wrote:
> > > > > Regards
> > > > > 
> > > > > Shashank
> > > > > 
> > > > > 
> > > > > On 11/14/2016 9:19 PM, Ville Syrjälä wrote:
> > > > > > On Mon, Nov 14, 2016 at 08:14:34PM +0530, Sharma, Shashank wrote:
> > > > > > > Regards
> > > > > > > Shashank
> > > > > > > > the revert:
> > > > > > > > 
> > > > > > > >      HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
> > > > > > > > -   1920x1080     60.00*+
> > > > > > > > -   1920x1080i    60.00    50.00
> > > > > > > > +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98
> > > > > > > > +   1920x1080i    60.00    50.00    59.94
> > > > > > > >         1600x1200     60.00
> > > > > > > >         1680x1050     59.88
> > > > > > > >         1280x1024     75.02    60.02
> > > > > > > > @@ -13,30 +13,29 @@
> > > > > > > >         1360x768      60.02
> > > > > > > >         1280x800      59.91
> > > > > > > >         1152x864      75.00
> > > > > > > > -   1280x720      60.00    50.00
> > > > > > > > +   1280x720      60.00    50.00    59.94
> > > > > > > >         1024x768      75.03    70.07    60.00
> > > > > > > >         832x624       74.55
> > > > > > > >         800x600       72.19    75.00    60.32
> > > > > > > > -   640x480       75.00    72.81    66.67    59.94
> > > > > > > > +   720x576       50.00
> > > > > > > > +   720x480       60.00    59.94
> > > > > > > > +   640x480       75.00    72.81    66.67    60.00    59.94
> > > > > > > >         720x400       70.08
> > > > > > > None of these aspect ratios are new modes / new aspect ratios from HDMI
> > > > > > > 2.0/CEA-861-F
> > > > > > > These are the existing modes, and should be independent of reverted
> > > > > > > patches.
> > > > > > They're affected because your patches changed them by adding the aspect
> > > > > > ratio flags to them.
> > > > > Yes, But they are independent of reverted patch, which adds aspect ratio
> > > > > for HDMI 2.0 ratios (64:27 and 256:135)
> > > > The second patch had to be reverted so that the first patch would revert
> > > > cleanly.
> > > > 
> > > > > > > > This was with sna, which does this:
> > > > > > > >      #define KNOWN_MODE_FLAGS ((1<<14)-1)
> > > > > > > >      if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
> > > > > > > >      	mode->status = MODE_BAD; /* unknown flags => unhandled */
> > > > > > > > so all the modes with an aspect ratio just vanished.
> > > > > > > > 
> > > > > > > > -modesetting and -ati on the other hand just copy over the unknown
> > > > > > > > bits into the xrandr mode structure, which sounds dubious at best:
> > > > > > > >      mode->Flags = kmode->flags; //& FLAG_BITS;
> > > > > > > > I've not checked what damage it can actually cause.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > It looks like a few modes disappeared from the kernel's mode list
> > > > > > > > as well, presumably because some cea modes in the list originated from
> > > > > > > > DTDs and whanot so they don't have an aspect ratio and that causes
> > > > > > > > add_alternate_cea_modes() to ignore them. So not populating an aspect
> > > > > > > > ratio for cea modes originating from a source other than
> > > > > > > > edid_cea_modes[] looks like another bug to me as well.
> > > > > > > I am writing a patch series to cap the aspect ratio implementation under
> > > > > > > a drm_cap_hdmi2_aspect_ratios
> > > > > > > This is how its going to work (inspired from the 2D/stereo series from
> > > > > > > damien L)
> > > > > > > 
> > > > > > > - Add a new capability hdmi2_ar
> > > > > > It should be just a generic "expose aspect ratio flags to userspace?"
> > > > > Makes sense, in this way we can even revert the aspect_ratio property
> > > > > for HDMI connector, as discussed during
> > > > > the code review sessions of this patch series. In this way, when kernel
> > > > > will expose the aspect ratios, it will either
> > > > > do the aspect ratios as per EDID, or wont.
> > > > > > > - by default parsing the new hdmi 2.0 aspect ratio will be disabled
> > > > > > > under check of this cap
> > > > > > > - during bootup time, while initializing the display, a userspace can
> > > > > > > get_cap on the hdmi2_aspect_ratio
> > > > > > > - If it wants HDMI 2.0 aspect ratio support, it will set the cap, and
> > > > > > > kernel will expose these aspect ratios
> > > > > > > > Another bug I think might be the ordering of the modes with aspect ratio
> > > > > > > > specified. IIRC the spec says that the preferred aspect ratio should be
> > > > > > > > listed first in the EDID, but I don't think we preserve that ordering
> > > > > > > > in the final mode list. I guess we could fix that by somehow noting
> > > > > > > > which aspect ratio is preferred and sort based on that, or we try to
> > > > > > > > preserve the order from the EDID until we're ready to sort, and then do
> > > > > > > > the sorting with a stable algorithm.
> > > > > > > AFAIK The mode order and priority is decided and arranged in userspace,
> > > > > > > based on various factors like
> > > > > > > - preferred mode.
> > > > > > > - previously applied mode in previous sessions (like for android tvs)
> > > > > > > - Bigger h/w vs better refresh rate ?
> > > > > > > - Xserver applies its own algorithms to decide which mode should be
> > > > > > > shown first.
> > > > > > Xorg does sort on its own. But since it doesn't know anything about
> > > > > > aspect ratios and whatnot I wouldn't rely on that for anything. I
> > > > > > also wouldn't expect eg. wayland compositors to do their own sorting.
> > > > > > And yeah, looks like weston at least doesn't do any sorting whatsoever.
> > > > > > 
> > > > > > > I dont think kernel needs to bother about it.
> > > > > > So I'm going to say that we in fact do need to bother.
> > > > > > 
> > > > > IMHO, making policies for UI is not a part of kernel design, a UI
> > > > > manager (Hardware composed, X or Wayland) should take care of it, as
> > > > > they have access to much information (Like previously applied mode, user
> > > > > preference etc). When it comes to sorting of modes, the only general rule
> > > > > across drivers like FB, V4L2, I have seen is the first mode in the list
> > > > > should be preferred mode, which we are still keeping. And after that our
> > > > > probed_modes were
> > > > > anyways not sorted now, so it doesn't matter further.
> > > > Having userspace be responsible for sorting the aspect ratios would
> > > > perhaps require that userspace parses the EDID, which is pretty crazy.
> > > Why ?
> > > userspace has to just set cap for aspect ratio, and kernel can read
> > > EDID, parse the CEA block, populate the aspect ratios flags
> > > and add the modes (Just what this patch was doing, except the cap part)
> > > Once userspace has the getResources/getConnector call filled, it can
> > > access all the modes (with and without aspect) and do the sorting
> > > in any way it wants.
> > > > I guess it could try to deduce something from the physical aspect ratio
> > > > of the display, but I'm not sure that's quite what we want either.
> > > > 
> > > > Also we already sort the modes in the kernel anyway, so it's not like
> > > > we'd be doing something new by also considering the aspect ratios.
> > > > I would at the very least want to avoid a totally random order between
> > > > modes that differ only by the aspect ratio.
> > > Path: get_connector -> probe_single_connector_mode -> drm_add_edid_modes
> > > Again, IMHO, we don't sort the modes in kernel, we populate modes in a
> > > particular order, which is:
> > > (From drm_edid.c::drm_add_edid_modes)
> > > ##############################################################
> > > /*
> > >        * EDID spec says modes should be preferred in this order:
> > >        * - preferred detailed mode
> > >        * - other detailed modes from base block
> > >        * - detailed modes from extension blocks
> > >        * - CVT 3-byte code modes
> > >        * - standard timing codes
> > >        * - established timing codes
> > >        * - modes inferred from GTF or CVT range information
> > >        *
> > >        * We get this pretty much right.
> > >        *
> > >        * XXX order for additional mode types in extension blocks?
> > >        */
> > >       num_modes += add_detailed_modes(connector, edid, quirks);
> > >       num_modes += add_cvt_modes(connector, edid);
> > >       num_modes += add_standard_modes(connector, edid);
> > >       num_modes += add_established_modes(connector, edid);
> > >       num_modes += add_cea_modes(connector, edid);
> > >       num_modes += add_alternate_cea_modes(connector, edid);
> > >       num_modes += add_displayid_detailed_modes(connector, edid);
> > > ###############################################################
> > > 
> > > Here the modes are added in the connector, in the same order they are
> > > arranged into their respective blocks in EDID.
> > > But the order to read the block is a preferred order (no sorting).
> > > 
> > > Now, in this patch series, we are adding aspect ratio information in
> > > edid_cea_modes db, which is going to affect only
> > > add_cea/alternate_cea_modes() call, and the modes accordingly.
> > > Please let me know if I misunderstood something here.
> > We explicitly sort the modes after this.
> 
> In any case, I guess addition of a cap for aspect ratio should fix the
> current objections for this implementation.
> 
> And I will keep it 0 by default, so that no aspect ratio information is
> added until userspace sets the cap to 1 on its own.

Note that cap = needs new userspace.
-Daniel

> 
> 
> Regards
> 
> Shashank
> 
> > > Regards
> > > Shashank
> > > > > If X server doesn't know what to do with aspect ratio flags, it can
> > > > > chose not to set the cap, and if HWC knows, it can chose to set. This is
> > > > > the same situation as 2D stereo modes
> > > > > which are existing already.
> > > > > 
> > > > > Regards
> > > > > Shashank
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

Regards

Shashank


On 11/15/2016 2:21 PM, Daniel Vetter wrote:
> On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
>> On 11/14/2016 10:15 PM, Ville Syrjälä wrote:
>>> On Mon, Nov 14, 2016 at 10:12:04PM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 11/14/2016 9:50 PM, Ville Syrjälä wrote:
>>>>> On Mon, Nov 14, 2016 at 09:37:18PM +0530, Sharma, Shashank wrote:
>>>>>> Regards
>>>>>>
>>>>>> Shashank
>>>>>>
>>>>>>
>>>>>> On 11/14/2016 9:19 PM, Ville Syrjälä wrote:
>>>>>>> On Mon, Nov 14, 2016 at 08:14:34PM +0530, Sharma, Shashank wrote:
>>>>>>>> Regards
>>>>>>>> Shashank
>>>>>>>>> the revert:
>>>>>>>>>
>>>>>>>>>       HDMI2 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 700mm x 390mm
>>>>>>>>> -   1920x1080     60.00*+
>>>>>>>>> -   1920x1080i    60.00    50.00
>>>>>>>>> +   1920x1080     60.00*+  50.00    59.94    30.00    25.00    24.00    29.97    23.98
>>>>>>>>> +   1920x1080i    60.00    50.00    59.94
>>>>>>>>>          1600x1200     60.00
>>>>>>>>>          1680x1050     59.88
>>>>>>>>>          1280x1024     75.02    60.02
>>>>>>>>> @@ -13,30 +13,29 @@
>>>>>>>>>          1360x768      60.02
>>>>>>>>>          1280x800      59.91
>>>>>>>>>          1152x864      75.00
>>>>>>>>> -   1280x720      60.00    50.00
>>>>>>>>> +   1280x720      60.00    50.00    59.94
>>>>>>>>>          1024x768      75.03    70.07    60.00
>>>>>>>>>          832x624       74.55
>>>>>>>>>          800x600       72.19    75.00    60.32
>>>>>>>>> -   640x480       75.00    72.81    66.67    59.94
>>>>>>>>> +   720x576       50.00
>>>>>>>>> +   720x480       60.00    59.94
>>>>>>>>> +   640x480       75.00    72.81    66.67    60.00    59.94
>>>>>>>>>          720x400       70.08
>>>>>>>> None of these aspect ratios are new modes / new aspect ratios from HDMI
>>>>>>>> 2.0/CEA-861-F
>>>>>>>> These are the existing modes, and should be independent of reverted
>>>>>>>> patches.
>>>>>>> They're affected because your patches changed them by adding the aspect
>>>>>>> ratio flags to them.
>>>>>> Yes, But they are independent of reverted patch, which adds aspect ratio
>>>>>> for HDMI 2.0 ratios (64:27 and 256:135)
>>>>> The second patch had to be reverted so that the first patch would revert
>>>>> cleanly.
>>>>>
>>>>>>>>> This was with sna, which does this:
>>>>>>>>>       #define KNOWN_MODE_FLAGS ((1<<14)-1)
>>>>>>>>>       if (mode->status == MODE_OK && kmode->flags & ~KNOWN_MODE_FLAGS)
>>>>>>>>>       	mode->status = MODE_BAD; /* unknown flags => unhandled */
>>>>>>>>> so all the modes with an aspect ratio just vanished.
>>>>>>>>>
>>>>>>>>> -modesetting and -ati on the other hand just copy over the unknown
>>>>>>>>> bits into the xrandr mode structure, which sounds dubious at best:
>>>>>>>>>       mode->Flags = kmode->flags; //& FLAG_BITS;
>>>>>>>>> I've not checked what damage it can actually cause.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> It looks like a few modes disappeared from the kernel's mode list
>>>>>>>>> as well, presumably because some cea modes in the list originated from
>>>>>>>>> DTDs and whanot so they don't have an aspect ratio and that causes
>>>>>>>>> add_alternate_cea_modes() to ignore them. So not populating an aspect
>>>>>>>>> ratio for cea modes originating from a source other than
>>>>>>>>> edid_cea_modes[] looks like another bug to me as well.
>>>>>>>> I am writing a patch series to cap the aspect ratio implementation under
>>>>>>>> a drm_cap_hdmi2_aspect_ratios
>>>>>>>> This is how its going to work (inspired from the 2D/stereo series from
>>>>>>>> damien L)
>>>>>>>>
>>>>>>>> - Add a new capability hdmi2_ar
>>>>>>> It should be just a generic "expose aspect ratio flags to userspace?"
>>>>>> Makes sense, in this way we can even revert the aspect_ratio property
>>>>>> for HDMI connector, as discussed during
>>>>>> the code review sessions of this patch series. In this way, when kernel
>>>>>> will expose the aspect ratios, it will either
>>>>>> do the aspect ratios as per EDID, or wont.
>>>>>>>> - by default parsing the new hdmi 2.0 aspect ratio will be disabled
>>>>>>>> under check of this cap
>>>>>>>> - during bootup time, while initializing the display, a userspace can
>>>>>>>> get_cap on the hdmi2_aspect_ratio
>>>>>>>> - If it wants HDMI 2.0 aspect ratio support, it will set the cap, and
>>>>>>>> kernel will expose these aspect ratios
>>>>>>>>> Another bug I think might be the ordering of the modes with aspect ratio
>>>>>>>>> specified. IIRC the spec says that the preferred aspect ratio should be
>>>>>>>>> listed first in the EDID, but I don't think we preserve that ordering
>>>>>>>>> in the final mode list. I guess we could fix that by somehow noting
>>>>>>>>> which aspect ratio is preferred and sort based on that, or we try to
>>>>>>>>> preserve the order from the EDID until we're ready to sort, and then do
>>>>>>>>> the sorting with a stable algorithm.
>>>>>>>> AFAIK The mode order and priority is decided and arranged in userspace,
>>>>>>>> based on various factors like
>>>>>>>> - preferred mode.
>>>>>>>> - previously applied mode in previous sessions (like for android tvs)
>>>>>>>> - Bigger h/w vs better refresh rate ?
>>>>>>>> - Xserver applies its own algorithms to decide which mode should be
>>>>>>>> shown first.
>>>>>>> Xorg does sort on its own. But since it doesn't know anything about
>>>>>>> aspect ratios and whatnot I wouldn't rely on that for anything. I
>>>>>>> also wouldn't expect eg. wayland compositors to do their own sorting.
>>>>>>> And yeah, looks like weston at least doesn't do any sorting whatsoever.
>>>>>>>
>>>>>>>> I dont think kernel needs to bother about it.
>>>>>>> So I'm going to say that we in fact do need to bother.
>>>>>>>
>>>>>> IMHO, making policies for UI is not a part of kernel design, a UI
>>>>>> manager (Hardware composed, X or Wayland) should take care of it, as
>>>>>> they have access to much information (Like previously applied mode, user
>>>>>> preference etc). When it comes to sorting of modes, the only general rule
>>>>>> across drivers like FB, V4L2, I have seen is the first mode in the list
>>>>>> should be preferred mode, which we are still keeping. And after that our
>>>>>> probed_modes were
>>>>>> anyways not sorted now, so it doesn't matter further.
>>>>> Having userspace be responsible for sorting the aspect ratios would
>>>>> perhaps require that userspace parses the EDID, which is pretty crazy.
>>>> Why ?
>>>> userspace has to just set cap for aspect ratio, and kernel can read
>>>> EDID, parse the CEA block, populate the aspect ratios flags
>>>> and add the modes (Just what this patch was doing, except the cap part)
>>>> Once userspace has the getResources/getConnector call filled, it can
>>>> access all the modes (with and without aspect) and do the sorting
>>>> in any way it wants.
>>>>> I guess it could try to deduce something from the physical aspect ratio
>>>>> of the display, but I'm not sure that's quite what we want either.
>>>>>
>>>>> Also we already sort the modes in the kernel anyway, so it's not like
>>>>> we'd be doing something new by also considering the aspect ratios.
>>>>> I would at the very least want to avoid a totally random order between
>>>>> modes that differ only by the aspect ratio.
>>>> Path: get_connector -> probe_single_connector_mode -> drm_add_edid_modes
>>>> Again, IMHO, we don't sort the modes in kernel, we populate modes in a
>>>> particular order, which is:
>>>> (From drm_edid.c::drm_add_edid_modes)
>>>> ##############################################################
>>>> /*
>>>>         * EDID spec says modes should be preferred in this order:
>>>>         * - preferred detailed mode
>>>>         * - other detailed modes from base block
>>>>         * - detailed modes from extension blocks
>>>>         * - CVT 3-byte code modes
>>>>         * - standard timing codes
>>>>         * - established timing codes
>>>>         * - modes inferred from GTF or CVT range information
>>>>         *
>>>>         * We get this pretty much right.
>>>>         *
>>>>         * XXX order for additional mode types in extension blocks?
>>>>         */
>>>>        num_modes += add_detailed_modes(connector, edid, quirks);
>>>>        num_modes += add_cvt_modes(connector, edid);
>>>>        num_modes += add_standard_modes(connector, edid);
>>>>        num_modes += add_established_modes(connector, edid);
>>>>        num_modes += add_cea_modes(connector, edid);
>>>>        num_modes += add_alternate_cea_modes(connector, edid);
>>>>        num_modes += add_displayid_detailed_modes(connector, edid);
>>>> ###############################################################
>>>>
>>>> Here the modes are added in the connector, in the same order they are
>>>> arranged into their respective blocks in EDID.
>>>> But the order to read the block is a preferred order (no sorting).
>>>>
>>>> Now, in this patch series, we are adding aspect ratio information in
>>>> edid_cea_modes db, which is going to affect only
>>>> add_cea/alternate_cea_modes() call, and the modes accordingly.
>>>> Please let me know if I misunderstood something here.
>>> We explicitly sort the modes after this.
>> In any case, I guess addition of a cap for aspect ratio should fix the
>> current objections for this implementation.
>>
>> And I will keep it 0 by default, so that no aspect ratio information is
>> added until userspace sets the cap to 1 on its own.
> Note that cap = needs new userspace.
> -Daniel
I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
Is that what you mean ?

>>
>> Regards
>>
>> Shashank
>>
>>>> Regards
>>>> Shashank
>>>>>> If X server doesn't know what to do with aspect ratio flags, it can
>>>>>> chose not to set the cap, and if HWC knows, it can chose to set. This is
>>>>>> the same situation as 2D stereo modes
>>>>>> which are existing already.
>>>>>>
>>>>>> Regards
>>>>>> Shashank

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Daniel Vetter]

On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
> On 11/15/2016 2:21 PM, Daniel Vetter wrote:
> > On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
> > > In any case, I guess addition of a cap for aspect ratio should fix the
> > > current objections for this implementation.
> > > 
> > > And I will keep it 0 by default, so that no aspect ratio information is
> > > added until userspace sets the cap to 1 on its own.
> > Note that cap = needs new userspace.
> > -Daniel
> I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
> Is that what you mean ?

Full stack solution, including enabling in an Xorg driver (or somewhere
else, we also have drm_hwcomposer as an option).

And because that's probably going to take forever I'm leaning towards
revert again. Ville?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

Regards

Shashank


On 11/15/2016 3:30 PM, Daniel Vetter wrote:
> On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
>> On 11/15/2016 2:21 PM, Daniel Vetter wrote:
>>> On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
>>>> In any case, I guess addition of a cap for aspect ratio should fix the
>>>> current objections for this implementation.
>>>>
>>>> And I will keep it 0 by default, so that no aspect ratio information is
>>>> added until userspace sets the cap to 1 on its own.
>>> Note that cap = needs new userspace.
>>> -Daniel
>> I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
>> Is that what you mean ?
> Full stack solution, including enabling in an Xorg driver (or somewhere
> else, we also have drm_hwcomposer as an option).
>
> And because that's probably going to take forever I'm leaning towards
> revert again. Ville?
Not really, with a kernel cap implementation, the code will be as it was 
before this patch series ( as good as revert )
And then when we want to enable it, we can add the corresponding Xserver 
patch.

I guess the current problem is if is breaks something in some userspace, 
but if I am loading the flags only when the cap is set, we should be good.

Regards
Shashank
> -Daniel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Daniel Vetter]

On Tue, Nov 15, 2016 at 03:36:02PM +0530, Sharma, Shashank wrote:
> On 11/15/2016 3:30 PM, Daniel Vetter wrote:
> > On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
> > > On 11/15/2016 2:21 PM, Daniel Vetter wrote:
> > > > On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
> > > > > In any case, I guess addition of a cap for aspect ratio should fix the
> > > > > current objections for this implementation.
> > > > > 
> > > > > And I will keep it 0 by default, so that no aspect ratio information is
> > > > > added until userspace sets the cap to 1 on its own.
> > > > Note that cap = needs new userspace.
> > > > -Daniel
> > > I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
> > > Is that what you mean ?
> > Full stack solution, including enabling in an Xorg driver (or somewhere
> > else, we also have drm_hwcomposer as an option).
> > 
> > And because that's probably going to take forever I'm leaning towards
> > revert again. Ville?
> Not really, with a kernel cap implementation, the code will be as it was
> before this patch series ( as good as revert )
> And then when we want to enable it, we can add the corresponding Xserver
> patch.
> 
> I guess the current problem is if is breaks something in some userspace, but
> if I am loading the flags only when the cap is set, we should be good.

This is not how new uabi gets merged, see:

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements

Userspace must be ready, or it will not land. The entire point of my
suggestion to just extend the display modes was to avoid the need for
userspace, but since existing userspace tries to be too clever already,
that didn't work out.
Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Jose Abreu]

Hi,



On 15-11-2016 10:52, Daniel Vetter wrote:
> On Tue, Nov 15, 2016 at 03:36:02PM +0530, Sharma, Shashank wrote:
>> On 11/15/2016 3:30 PM, Daniel Vetter wrote:
>>> On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
>>>> On 11/15/2016 2:21 PM, Daniel Vetter wrote:
>>>>> On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
>>>>>> In any case, I guess addition of a cap for aspect ratio should fix the
>>>>>> current objections for this implementation.
>>>>>>
>>>>>> And I will keep it 0 by default, so that no aspect ratio information is
>>>>>> added until userspace sets the cap to 1 on its own.
>>>>> Note that cap = needs new userspace.
>>>>> -Daniel
>>>> I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
>>>> Is that what you mean ?
>>> Full stack solution, including enabling in an Xorg driver (or somewhere
>>> else, we also have drm_hwcomposer as an option).
>>>
>>> And because that's probably going to take forever I'm leaning towards
>>> revert again. Ville?
>> Not really, with a kernel cap implementation, the code will be as it was
>> before this patch series ( as good as revert )
>> And then when we want to enable it, we can add the corresponding Xserver
>> patch.
>>
>> I guess the current problem is if is breaks something in some userspace, but
>> if I am loading the flags only when the cap is set, we should be good.
> This is not how new uabi gets merged, see:
>
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
>
> Userspace must be ready, or it will not land. The entire point of my
> suggestion to just extend the display modes was to avoid the need for
> userspace, but since existing userspace tries to be too clever already,
> that didn't work out.
> Thanks, Daniel

@Ville

I gave my tested by to these patches because I was facing the
same scenario as Shashank: HDMI compliance. I believe we need to
find a better way to handle this instead of just reverting. The
HDMI spec is evolving so we also need to evolve. Of course that
if this is breaking user space then we can revert and wait until
we figure the best way to implement this.

Anyway, this is a wild guess but I think the reason you are
loosing modes may be because of
drm_mode_equal_no_clocks_no_stereo() which is always expecting a
aspect ratio flag to be defined. If there isn't one defined then
it will unconditionally return false, even if the modes are
equal. Can you please test this patch and see if it fixes on your
side? WARNING: Not compile tested

Best regards,
Jose Miguel Abreu

diff --git a/drivers/gpu/drm/drm_modes.c
b/drivers/gpu/drm/drm_modes.c
index ce6eeda..a7973a9 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -971,6 +971,27 @@ bool drm_mode_equal_no_clocks(const struct
drm_display_mode *mode1, const struct
 }
 EXPORT_SYMBOL(drm_mode_equal_no_clocks);
 
+static bool drm_mode_par_valid(const struct drm_display_mode *mode)
+{
+       switch (mode->picture_aspect_ratio) {
+       case HDMI_PICTURE_ASPECT_4_3:
+       case HDMI_PICTURE_ASPECT_16_9:
+       case HDMI_PICTURE_ASPECT_64_27:
+       case HDMI_PICTURE_ASPECT_256_135:
+               return true;
+       default:
+               return false;
+       }
+}
+
+static bool drm_mode_equal_par(const struct drm_display_mode *mode1,
+                              const struct drm_display_mode *mode2)
+{
+       if (!drm_mode_par_valid(mode1) || !drm_mode_par_valid(mode2))
+               return true;
+       return mode1->picture_aspect_ratio ==
mode2->picture_aspect_ratio;
+}
+
 /**
  * drm_mode_equal_no_clocks_no_stereo - test modes for equality
  * @mode1: first mode
@@ -995,7 +1016,7 @@ bool
drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode
*mode1,
            mode1->vsync_end == mode2->vsync_end &&
            mode1->vtotal == mode2->vtotal &&
            mode1->vscan == mode2->vscan &&
-           mode1->picture_aspect_ratio ==
mode2->picture_aspect_ratio &&
+           drm_mode_equal_par(mode1, mode2) &&
            (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
             (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
                return true;

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Tue, Nov 15, 2016 at 11:00:04AM +0100, Daniel Vetter wrote:
> On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
> > On 11/15/2016 2:21 PM, Daniel Vetter wrote:
> > > On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
> > > > In any case, I guess addition of a cap for aspect ratio should fix the
> > > > current objections for this implementation.
> > > > 
> > > > And I will keep it 0 by default, so that no aspect ratio information is
> > > > added until userspace sets the cap to 1 on its own.
> > > Note that cap = needs new userspace.
> > > -Daniel
> > I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
> > Is that what you mean ?
> 
> Full stack solution, including enabling in an Xorg driver (or somewhere
> else, we also have drm_hwcomposer as an option).
> 
> And because that's probably going to take forever I'm leaning towards
> revert again. Ville?

Yeah I guess we'll need to push the revert to avoid the regression.
Trying to put in new client caps and whatnot after -rc5 doesn't seem
like a viable option to me.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Daniel Vetter]

On Tue, Nov 15, 2016 at 03:54:42PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 15, 2016 at 11:00:04AM +0100, Daniel Vetter wrote:
> > On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
> > > On 11/15/2016 2:21 PM, Daniel Vetter wrote:
> > > > On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
> > > > > In any case, I guess addition of a cap for aspect ratio should fix the
> > > > > current objections for this implementation.
> > > > > 
> > > > > And I will keep it 0 by default, so that no aspect ratio information is
> > > > > added until userspace sets the cap to 1 on its own.
> > > > Note that cap = needs new userspace.
> > > > -Daniel
> > > I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
> > > Is that what you mean ?
> > 
> > Full stack solution, including enabling in an Xorg driver (or somewhere
> > else, we also have drm_hwcomposer as an option).
> > 
> > And because that's probably going to take forever I'm leaning towards
> > revert again. Ville?
> 
> Yeah I guess we'll need to push the revert to avoid the regression.
> Trying to put in new client caps and whatnot after -rc5 doesn't seem
> like a viable option to me.

Yeah, a few days left to get userspace in line is just not enough. Agreed
and reverts applied.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Alex Deucher]

On Tue, Nov 15, 2016 at 9:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Nov 15, 2016 at 03:54:42PM +0200, Ville Syrjälä wrote:
>> On Tue, Nov 15, 2016 at 11:00:04AM +0100, Daniel Vetter wrote:
>> > On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
>> > > On 11/15/2016 2:21 PM, Daniel Vetter wrote:
>> > > > On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
>> > > > > In any case, I guess addition of a cap for aspect ratio should fix the
>> > > > > current objections for this implementation.
>> > > > >
>> > > > > And I will keep it 0 by default, so that no aspect ratio information is
>> > > > > added until userspace sets the cap to 1 on its own.
>> > > > Note that cap = needs new userspace.
>> > > > -Daniel
>> > > I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
>> > > Is that what you mean ?
>> >
>> > Full stack solution, including enabling in an Xorg driver (or somewhere
>> > else, we also have drm_hwcomposer as an option).
>> >
>> > And because that's probably going to take forever I'm leaning towards
>> > revert again. Ville?
>>
>> Yeah I guess we'll need to push the revert to avoid the regression.
>> Trying to put in new client caps and whatnot after -rc5 doesn't seem
>> like a viable option to me.
>
> Yeah, a few days left to get userspace in line is just not enough. Agreed
> and reverts applied.
>

Is there any way we can add the new CEA modes and worry about handling
the aspect ratio stuff later?

Alex
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Tue, Nov 15, 2016 at 01:48:04PM +0000, Jose Abreu wrote:
> Hi,
> 
> 
> 
> On 15-11-2016 10:52, Daniel Vetter wrote:
> > On Tue, Nov 15, 2016 at 03:36:02PM +0530, Sharma, Shashank wrote:
> >> On 11/15/2016 3:30 PM, Daniel Vetter wrote:
> >>> On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
> >>>> On 11/15/2016 2:21 PM, Daniel Vetter wrote:
> >>>>> On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
> >>>>>> In any case, I guess addition of a cap for aspect ratio should fix the
> >>>>>> current objections for this implementation.
> >>>>>>
> >>>>>> And I will keep it 0 by default, so that no aspect ratio information is
> >>>>>> added until userspace sets the cap to 1 on its own.
> >>>>> Note that cap = needs new userspace.
> >>>>> -Daniel
> >>>> I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
> >>>> Is that what you mean ?
> >>> Full stack solution, including enabling in an Xorg driver (or somewhere
> >>> else, we also have drm_hwcomposer as an option).
> >>>
> >>> And because that's probably going to take forever I'm leaning towards
> >>> revert again. Ville?
> >> Not really, with a kernel cap implementation, the code will be as it was
> >> before this patch series ( as good as revert )
> >> And then when we want to enable it, we can add the corresponding Xserver
> >> patch.
> >>
> >> I guess the current problem is if is breaks something in some userspace, but
> >> if I am loading the flags only when the cap is set, we should be good.
> > This is not how new uabi gets merged, see:
> >
> > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> >
> > Userspace must be ready, or it will not land. The entire point of my
> > suggestion to just extend the display modes was to avoid the need for
> > userspace, but since existing userspace tries to be too clever already,
> > that didn't work out.
> > Thanks, Daniel
> 
> @Ville
> 
> I gave my tested by to these patches because I was facing the
> same scenario as Shashank: HDMI compliance. I believe we need to
> find a better way to handle this instead of just reverting. The
> HDMI spec is evolving so we also need to evolve. Of course that
> if this is breaking user space then we can revert and wait until
> we figure the best way to implement this.
> 
> Anyway, this is a wild guess but I think the reason you are
> loosing modes may be because of
> drm_mode_equal_no_clocks_no_stereo() which is always expecting a
> aspect ratio flag to be defined. If there isn't one defined then
> it will unconditionally return false, even if the modes are
> equal.

I am well aware why we're losing modes. One reason is the mode equal
checks in the kernel as you suspect, the other is simply userspace
rejecting everything with the aspect ratio defined.

> Can you please test this patch and see if it fixes on your
> side? WARNING: Not compile tested

I already did something like that earlier, except I rewrote the
entire messy mode matching code to use a cleaner flag based approach:

git://github.com/vsyrjala/linux.git cea_f_vics

But that doesn't solve the userspace ABI issue, and there are still a
lot of unanswered questions like:

- Should we define aspect ratios for modes not directly coming from
  edid_cea_modes[]?

  I beleive the answer must be "yes" at least in the cases where the
  EDID lists the VIC even if we then skip adding the mode due to
  already having it on the list via from another source. Perhaps we
  want the aspect ratio even if the VIC wasn't directly specified?

- Should we or should we not specify a VIC in the AVI infoframe
  when the userspace doesn't support aspect ratio flags?

  I would guess the answer is again "yes", and we should just pick the
  preferred aspect ratio for the mode. At least that's closer to what
  we've been doing up to now (except we just picked the first VIC, so
  we might have picked the non-preferred aspect ratio actually). At
  least having the VIC in the infoframe would seem like a safer option
  than not having it, in case there's some cheap ass hardware that
  can't do anything sensible without being hand fed a VIC.

- How we should handle the aspect ratio in mode sorting?

  I think we want some sensible sorting order for these. Preferred
  aspect ratio first would seem like the obvious choice. I do realize
  that we don't sort based on 3D stereo flags either, but I thinking we
  probably should.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Tue, Nov 15, 2016 at 09:18:03AM -0500, Alex Deucher wrote:
> On Tue, Nov 15, 2016 at 9:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Nov 15, 2016 at 03:54:42PM +0200, Ville Syrjälä wrote:
> >> On Tue, Nov 15, 2016 at 11:00:04AM +0100, Daniel Vetter wrote:
> >> > On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
> >> > > On 11/15/2016 2:21 PM, Daniel Vetter wrote:
> >> > > > On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
> >> > > > > In any case, I guess addition of a cap for aspect ratio should fix the
> >> > > > > current objections for this implementation.
> >> > > > >
> >> > > > > And I will keep it 0 by default, so that no aspect ratio information is
> >> > > > > added until userspace sets the cap to 1 on its own.
> >> > > > Note that cap = needs new userspace.
> >> > > > -Daniel
> >> > > I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
> >> > > Is that what you mean ?
> >> >
> >> > Full stack solution, including enabling in an Xorg driver (or somewhere
> >> > else, we also have drm_hwcomposer as an option).
> >> >
> >> > And because that's probably going to take forever I'm leaning towards
> >> > revert again. Ville?
> >>
> >> Yeah I guess we'll need to push the revert to avoid the regression.
> >> Trying to put in new client caps and whatnot after -rc5 doesn't seem
> >> like a viable option to me.
> >
> > Yeah, a few days left to get userspace in line is just not enough. Agreed
> > and reverts applied.
> >
> 
> Is there any way we can add the new CEA modes and worry about handling
> the aspect ratio stuff later?

I don't think there's any dependency between the two. Or am I
overlooking something?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Daniel Vetter]

On Tue, Nov 15, 2016 at 09:18:03AM -0500, Alex Deucher wrote:
> On Tue, Nov 15, 2016 at 9:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Nov 15, 2016 at 03:54:42PM +0200, Ville Syrjälä wrote:
> >> On Tue, Nov 15, 2016 at 11:00:04AM +0100, Daniel Vetter wrote:
> >> > On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
> >> > > On 11/15/2016 2:21 PM, Daniel Vetter wrote:
> >> > > > On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
> >> > > > > In any case, I guess addition of a cap for aspect ratio should fix the
> >> > > > > current objections for this implementation.
> >> > > > >
> >> > > > > And I will keep it 0 by default, so that no aspect ratio information is
> >> > > > > added until userspace sets the cap to 1 on its own.
> >> > > > Note that cap = needs new userspace.
> >> > > > -Daniel
> >> > > I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
> >> > > Is that what you mean ?
> >> >
> >> > Full stack solution, including enabling in an Xorg driver (or somewhere
> >> > else, we also have drm_hwcomposer as an option).
> >> >
> >> > And because that's probably going to take forever I'm leaning towards
> >> > revert again. Ville?
> >>
> >> Yeah I guess we'll need to push the revert to avoid the regression.
> >> Trying to put in new client caps and whatnot after -rc5 doesn't seem
> >> like a viable option to me.
> >
> > Yeah, a few days left to get userspace in line is just not enough. Agreed
> > and reverts applied.
> >
> 
> Is there any way we can add the new CEA modes and worry about handling
> the aspect ratio stuff later?

The two reverts from Ville are just for the aspect ratio pass-thru stuff,
new cea modes should still be around. So exactly what you're asking for.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

Regards
Shashank
On 11/15/2016 7:48 PM, Ville Syrjälä wrote:
> On Tue, Nov 15, 2016 at 01:48:04PM +0000, Jose Abreu wrote:
>> Hi,
>>
>>
>>
>> On 15-11-2016 10:52, Daniel Vetter wrote:
>>> On Tue, Nov 15, 2016 at 03:36:02PM +0530, Sharma, Shashank wrote:
>>>> On 11/15/2016 3:30 PM, Daniel Vetter wrote:
>>>>> On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
>>>>>> On 11/15/2016 2:21 PM, Daniel Vetter wrote:
>>>>>>> On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
>>>>>>>> In any case, I guess addition of a cap for aspect ratio should fix the
>>>>>>>> current objections for this implementation.
>>>>>>>>
>>>>>>>> And I will keep it 0 by default, so that no aspect ratio information is
>>>>>>>> added until userspace sets the cap to 1 on its own.
>>>>>>> Note that cap = needs new userspace.
>>>>>>> -Daniel
>>>>>> I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
>>>>>> Is that what you mean ?
>>>>> Full stack solution, including enabling in an Xorg driver (or somewhere
>>>>> else, we also have drm_hwcomposer as an option).
>>>>>
>>>>> And because that's probably going to take forever I'm leaning towards
>>>>> revert again. Ville?
>>>> Not really, with a kernel cap implementation, the code will be as it was
>>>> before this patch series ( as good as revert )
>>>> And then when we want to enable it, we can add the corresponding Xserver
>>>> patch.
>>>>
>>>> I guess the current problem is if is breaks something in some userspace, but
>>>> if I am loading the flags only when the cap is set, we should be good.
>>> This is not how new uabi gets merged, see:
>>>
>>> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
>>>
>>> Userspace must be ready, or it will not land. The entire point of my
>>> suggestion to just extend the display modes was to avoid the need for
>>> userspace, but since existing userspace tries to be too clever already,
>>> that didn't work out.
>>> Thanks, Daniel
>> @Ville
>>
>> I gave my tested by to these patches because I was facing the
>> same scenario as Shashank: HDMI compliance. I believe we need to
>> find a better way to handle this instead of just reverting. The
>> HDMI spec is evolving so we also need to evolve. Of course that
>> if this is breaking user space then we can revert and wait until
>> we figure the best way to implement this.
>>
>> Anyway, this is a wild guess but I think the reason you are
>> loosing modes may be because of
>> drm_mode_equal_no_clocks_no_stereo() which is always expecting a
>> aspect ratio flag to be defined. If there isn't one defined then
>> it will unconditionally return false, even if the modes are
>> equal.
> I am well aware why we're losing modes. One reason is the mode equal
> checks in the kernel as you suspect, the other is simply userspace
> rejecting everything with the aspect ratio defined.
>
>> Can you please test this patch and see if it fixes on your
>> side? WARNING: Not compile tested
> I already did something like that earlier, except I rewrote the
> entire messy mode matching code to use a cleaner flag based approach:
>
> git://github.com/vsyrjala/linux.git cea_f_vics
>
> But that doesn't solve the userspace ABI issue, and there are still a
> lot of unanswered questions like:
>
> - Should we define aspect ratios for modes not directly coming from
>    edid_cea_modes[]?
>
>    I beleive the answer must be "yes" at least in the cases where the
>    EDID lists the VIC even if we then skip adding the mode due to
>    already having it on the list via from another source. Perhaps we
>    want the aspect ratio even if the VIC wasn't directly specified?
Wouldn't this break the whole concept of VIC, which is supposed to point 
to a unique mode ?
> - Should we or should we not specify a VIC in the AVI infoframe
>    when the userspace doesn't support aspect ratio flags?
This is a requirement of HDMI compliance tests, if userspace 
supports/handles aspect ratio, it should set cap, and it will be set in 
kernel flags
and we should load that in AV IF.  Are you planning to suggest a better 
way, in which this can be done ?
>    I would guess the answer is again "yes", and we should just pick the
>    preferred aspect ratio for the mode. At least that's closer to what
>    we've been doing up to now (except we just picked the first VIC, so
>    we might have picked the non-preferred aspect ratio actually). At
>    least having the VIC in the infoframe would seem like a safer option
>    than not having it, in case there's some cheap ass hardware that
>    can't do anything sensible without being hand fed a VIC.
>
> - How we should handle the aspect ratio in mode sorting?
>
>    I think we want some sensible sorting order for these. Preferred
>    aspect ratio first would seem like the obvious choice. I do realize
>    that we don't sort based on 3D stereo flags either, but I thinking we
>    probably should.
What is the need of this sorting ? None of the standards define/suggest 
this. AFAIK, the requirement is that only the preferred mode should be 
notified
to the userspace, if a system has to take a call, it can create its 
policy, if a user has to take a call, it can pick any from the list.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Sharma, Shashank]

I guess Daniel has already reverted the patches.

Now many I know who is going to define on what should be the right way 
to handle aspect ratios ?

Regards
Shashank
On 11/15/2016 7:48 PM, Ville Syrjälä wrote:
> On Tue, Nov 15, 2016 at 01:48:04PM +0000, Jose Abreu wrote:
>> Hi,
>>
>>
>>
>> On 15-11-2016 10:52, Daniel Vetter wrote:
>>> On Tue, Nov 15, 2016 at 03:36:02PM +0530, Sharma, Shashank wrote:
>>>> On 11/15/2016 3:30 PM, Daniel Vetter wrote:
>>>>> On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
>>>>>> On 11/15/2016 2:21 PM, Daniel Vetter wrote:
>>>>>>> On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
>>>>>>>> In any case, I guess addition of a cap for aspect ratio should fix the
>>>>>>>> current objections for this implementation.
>>>>>>>>
>>>>>>>> And I will keep it 0 by default, so that no aspect ratio information is
>>>>>>>> added until userspace sets the cap to 1 on its own.
>>>>>>> Note that cap = needs new userspace.
>>>>>>> -Daniel
>>>>>> I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
>>>>>> Is that what you mean ?
>>>>> Full stack solution, including enabling in an Xorg driver (or somewhere
>>>>> else, we also have drm_hwcomposer as an option).
>>>>>
>>>>> And because that's probably going to take forever I'm leaning towards
>>>>> revert again. Ville?
>>>> Not really, with a kernel cap implementation, the code will be as it was
>>>> before this patch series ( as good as revert )
>>>> And then when we want to enable it, we can add the corresponding Xserver
>>>> patch.
>>>>
>>>> I guess the current problem is if is breaks something in some userspace, but
>>>> if I am loading the flags only when the cap is set, we should be good.
>>> This is not how new uabi gets merged, see:
>>>
>>> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
>>>
>>> Userspace must be ready, or it will not land. The entire point of my
>>> suggestion to just extend the display modes was to avoid the need for
>>> userspace, but since existing userspace tries to be too clever already,
>>> that didn't work out.
>>> Thanks, Daniel
>> @Ville
>>
>> I gave my tested by to these patches because I was facing the
>> same scenario as Shashank: HDMI compliance. I believe we need to
>> find a better way to handle this instead of just reverting. The
>> HDMI spec is evolving so we also need to evolve. Of course that
>> if this is breaking user space then we can revert and wait until
>> we figure the best way to implement this.
>>
>> Anyway, this is a wild guess but I think the reason you are
>> loosing modes may be because of
>> drm_mode_equal_no_clocks_no_stereo() which is always expecting a
>> aspect ratio flag to be defined. If there isn't one defined then
>> it will unconditionally return false, even if the modes are
>> equal.
> I am well aware why we're losing modes. One reason is the mode equal
> checks in the kernel as you suspect, the other is simply userspace
> rejecting everything with the aspect ratio defined.
>
>> Can you please test this patch and see if it fixes on your
>> side? WARNING: Not compile tested
> I already did something like that earlier, except I rewrote the
> entire messy mode matching code to use a cleaner flag based approach:
>
> git://github.com/vsyrjala/linux.git cea_f_vics
>
> But that doesn't solve the userspace ABI issue, and there are still a
> lot of unanswered questions like:
>
> - Should we define aspect ratios for modes not directly coming from
>    edid_cea_modes[]?
>
>    I beleive the answer must be "yes" at least in the cases where the
>    EDID lists the VIC even if we then skip adding the mode due to
>    already having it on the list via from another source. Perhaps we
>    want the aspect ratio even if the VIC wasn't directly specified?
>
> - Should we or should we not specify a VIC in the AVI infoframe
>    when the userspace doesn't support aspect ratio flags?
>
>    I would guess the answer is again "yes", and we should just pick the
>    preferred aspect ratio for the mode. At least that's closer to what
>    we've been doing up to now (except we just picked the first VIC, so
>    we might have picked the non-preferred aspect ratio actually). At
>    least having the VIC in the infoframe would seem like a safer option
>    than not having it, in case there's some cheap ass hardware that
>    can't do anything sensible without being hand fed a VIC.
>
> - How we should handle the aspect ratio in mode sorting?
>
>    I think we want some sensible sorting order for these. Preferred
>    aspect ratio first would seem like the obvious choice. I do realize
>    that we don't sort based on 3D stereo flags either, but I thinking we
>    probably should.
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Ville Syrjälä]

On Tue, Nov 15, 2016 at 08:40:01PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> On 11/15/2016 7:48 PM, Ville Syrjälä wrote:
> > On Tue, Nov 15, 2016 at 01:48:04PM +0000, Jose Abreu wrote:
> >> Hi,
> >>
> >>
> >>
> >> On 15-11-2016 10:52, Daniel Vetter wrote:
> >>> On Tue, Nov 15, 2016 at 03:36:02PM +0530, Sharma, Shashank wrote:
> >>>> On 11/15/2016 3:30 PM, Daniel Vetter wrote:
> >>>>> On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
> >>>>>> On 11/15/2016 2:21 PM, Daniel Vetter wrote:
> >>>>>>> On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
> >>>>>>>> In any case, I guess addition of a cap for aspect ratio should fix the
> >>>>>>>> current objections for this implementation.
> >>>>>>>>
> >>>>>>>> And I will keep it 0 by default, so that no aspect ratio information is
> >>>>>>>> added until userspace sets the cap to 1 on its own.
> >>>>>>> Note that cap = needs new userspace.
> >>>>>>> -Daniel
> >>>>>> I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
> >>>>>> Is that what you mean ?
> >>>>> Full stack solution, including enabling in an Xorg driver (or somewhere
> >>>>> else, we also have drm_hwcomposer as an option).
> >>>>>
> >>>>> And because that's probably going to take forever I'm leaning towards
> >>>>> revert again. Ville?
> >>>> Not really, with a kernel cap implementation, the code will be as it was
> >>>> before this patch series ( as good as revert )
> >>>> And then when we want to enable it, we can add the corresponding Xserver
> >>>> patch.
> >>>>
> >>>> I guess the current problem is if is breaks something in some userspace, but
> >>>> if I am loading the flags only when the cap is set, we should be good.
> >>> This is not how new uabi gets merged, see:
> >>>
> >>> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> >>>
> >>> Userspace must be ready, or it will not land. The entire point of my
> >>> suggestion to just extend the display modes was to avoid the need for
> >>> userspace, but since existing userspace tries to be too clever already,
> >>> that didn't work out.
> >>> Thanks, Daniel
> >> @Ville
> >>
> >> I gave my tested by to these patches because I was facing the
> >> same scenario as Shashank: HDMI compliance. I believe we need to
> >> find a better way to handle this instead of just reverting. The
> >> HDMI spec is evolving so we also need to evolve. Of course that
> >> if this is breaking user space then we can revert and wait until
> >> we figure the best way to implement this.
> >>
> >> Anyway, this is a wild guess but I think the reason you are
> >> loosing modes may be because of
> >> drm_mode_equal_no_clocks_no_stereo() which is always expecting a
> >> aspect ratio flag to be defined. If there isn't one defined then
> >> it will unconditionally return false, even if the modes are
> >> equal.
> > I am well aware why we're losing modes. One reason is the mode equal
> > checks in the kernel as you suspect, the other is simply userspace
> > rejecting everything with the aspect ratio defined.
> >
> >> Can you please test this patch and see if it fixes on your
> >> side? WARNING: Not compile tested
> > I already did something like that earlier, except I rewrote the
> > entire messy mode matching code to use a cleaner flag based approach:
> >
> > git://github.com/vsyrjala/linux.git cea_f_vics
> >
> > But that doesn't solve the userspace ABI issue, and there are still a
> > lot of unanswered questions like:
> >
> > - Should we define aspect ratios for modes not directly coming from
> >    edid_cea_modes[]?
> >
> >    I beleive the answer must be "yes" at least in the cases where the
> >    EDID lists the VIC even if we then skip adding the mode due to
> >    already having it on the list via from another source. Perhaps we
> >    want the aspect ratio even if the VIC wasn't directly specified?
> Wouldn't this break the whole concept of VIC, which is supposed to point 
> to a unique mode ?

Not sure what you're asking. We're already filtering out duplicates.

> > - Should we or should we not specify a VIC in the AVI infoframe
> >    when the userspace doesn't support aspect ratio flags?
> This is a requirement of HDMI compliance tests, if userspace 
> supports/handles aspect ratio, it should set cap, and it will be set in 
> kernel flags
> and we should load that in AV IF.  Are you planning to suggest a better 
> way, in which this can be done ?

I was asking for the case where userspace doesn't specify the aspect
ratio on account of not understanding what aspect ratios are.

> >    I would guess the answer is again "yes", and we should just pick the
> >    preferred aspect ratio for the mode. At least that's closer to what
> >    we've been doing up to now (except we just picked the first VIC, so
> >    we might have picked the non-preferred aspect ratio actually). At
> >    least having the VIC in the infoframe would seem like a safer option
> >    than not having it, in case there's some cheap ass hardware that
> >    can't do anything sensible without being hand fed a VIC.
> >
> > - How we should handle the aspect ratio in mode sorting?
> >
> >    I think we want some sensible sorting order for these. Preferred
> >    aspect ratio first would seem like the obvious choice. I do realize
> >    that we don't sort based on 3D stereo flags either, but I thinking we
> >    probably should.
> What is the need of this sorting ? None of the standards define/suggest 
> this. AFAIK, the requirement is that only the preferred mode should be 
> notified
> to the userspace, if a system has to take a call, it can create its 
> policy, if a user has to take a call, it can pick any from the list.

Dumb userspace will just pick the first mode it sees that sort of
matches what it wants. So we had better make sure that one has the more
sensible aspect ratio choice.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Alex Deucher]

On Tue, Nov 15, 2016 at 9:26 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Nov 15, 2016 at 09:18:03AM -0500, Alex Deucher wrote:
>> On Tue, Nov 15, 2016 at 9:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Tue, Nov 15, 2016 at 03:54:42PM +0200, Ville Syrjälä wrote:
>> >> On Tue, Nov 15, 2016 at 11:00:04AM +0100, Daniel Vetter wrote:
>> >> > On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
>> >> > > On 11/15/2016 2:21 PM, Daniel Vetter wrote:
>> >> > > > On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
>> >> > > > > In any case, I guess addition of a cap for aspect ratio should fix the
>> >> > > > > current objections for this implementation.
>> >> > > > >
>> >> > > > > And I will keep it 0 by default, so that no aspect ratio information is
>> >> > > > > added until userspace sets the cap to 1 on its own.
>> >> > > > Note that cap = needs new userspace.
>> >> > > > -Daniel
>> >> > > I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
>> >> > > Is that what you mean ?
>> >> >
>> >> > Full stack solution, including enabling in an Xorg driver (or somewhere
>> >> > else, we also have drm_hwcomposer as an option).
>> >> >
>> >> > And because that's probably going to take forever I'm leaning towards
>> >> > revert again. Ville?
>> >>
>> >> Yeah I guess we'll need to push the revert to avoid the regression.
>> >> Trying to put in new client caps and whatnot after -rc5 doesn't seem
>> >> like a viable option to me.
>> >
>> > Yeah, a few days left to get userspace in line is just not enough. Agreed
>> > and reverts applied.
>> >
>>
>> Is there any way we can add the new CEA modes and worry about handling
>> the aspect ratio stuff later?
>
> The two reverts from Ville are just for the aspect ratio pass-thru stuff,
> new cea modes should still be around. So exactly what you're asking for.
> -Daniel

Great.  Thanks!

Alex
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel