* [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" @ 2016-11-03 12:31 ville.syrjala 2016-11-03 12:31 ` [PATCH 2/2] Revert "drm: Add aspect ratio parsing " ville.syrjala ` (2 more replies) 0 siblings, 3 replies; 44+ messages in thread From: ville.syrjala @ 2016-11-03 12:31 UTC (permalink / raw) To: dri-devel Cc: Jose Abreu, Jia, Akashdeep Sharma, Lin, Emil Velikov, Daniel Vetter, Jim Bride 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 2/2] Revert "drm: Add aspect ratio parsing in DRM layer" 2016-11-03 12:31 [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" ville.syrjala @ 2016-11-03 12:31 ` ville.syrjala 2016-11-03 12:47 ` Sharma, Shashank [not found] ` <FF3DDC77922A8A4BB08A3BC48A1EA8CB411AE158@BGSMSX101.gar.corp.intel.com> 2016-11-11 17:00 ` Ville Syrjälä 2 siblings, 1 reply; 44+ messages in thread From: ville.syrjala @ 2016-11-03 12:31 UTC (permalink / raw) To: dri-devel Cc: Jose Abreu, Jia, Akashdeep Sharma, Lin, Emil Velikov, Daniel Vetter, Jim Bride 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* RE: [PATCH 2/2] Revert "drm: Add aspect ratio parsing in DRM layer" 2016-11-03 12:31 ` [PATCH 2/2] Revert "drm: Add aspect ratio parsing " ville.syrjala @ 2016-11-03 12:47 ` Sharma, Shashank 0 siblings, 0 replies; 44+ messages in thread From: Sharma, Shashank @ 2016-11-03 12:47 UTC (permalink / raw) To: ville.syrjala, dri-devel Cc: Abdallah, Lina S, Jose Abreu, Jia, Lin A, Sharma, Akashdeep, Emil Velikov, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
[parent not found: <FF3DDC77922A8A4BB08A3BC48A1EA8CB411AE158@BGSMSX101.gar.corp.intel.com>]
[parent not found: <20161103130253.GM4617@intel.com>]
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" [not found] ` <20161103130253.GM4617@intel.com> @ 2016-11-03 13:04 ` Ville Syrjälä 2016-11-03 13:09 ` Ville Syrjälä [not found] ` <2ededbc1-b923-200e-8443-f1ee522402bf@intel.com> 1 sibling, 1 reply; 44+ messages in thread From: Ville Syrjälä @ 2016-11-03 13:04 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Sharma, Akashdeep, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-03 13:04 ` [PATCH 1/2] Revert "drm: Add and handle new aspect ratios " Ville Syrjälä @ 2016-11-03 13:09 ` Ville Syrjälä 2016-11-03 13:22 ` Sharma, Shashank 0 siblings, 1 reply; 44+ messages in thread From: Ville Syrjälä @ 2016-11-03 13:09 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Sharma, Akashdeep, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-03 13:09 ` Ville Syrjälä @ 2016-11-03 13:22 ` Sharma, Shashank 0 siblings, 0 replies; 44+ messages in thread From: Sharma, Shashank @ 2016-11-03 13:22 UTC (permalink / raw) To: Ville Syrjälä Cc: Jose Abreu, Jia, Lin A, Sharma, Akashdeep, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
[parent not found: <2ededbc1-b923-200e-8443-f1ee522402bf@intel.com>]
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" [not found] ` <2ededbc1-b923-200e-8443-f1ee522402bf@intel.com> @ 2016-11-03 13:26 ` Ville Syrjälä 2016-11-03 13:34 ` Sharma, Shashank 0 siblings, 1 reply; 44+ messages in thread From: Ville Syrjälä @ 2016-11-03 13:26 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Sharma, Akashdeep, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-03 13:26 ` Ville Syrjälä @ 2016-11-03 13:34 ` Sharma, Shashank 2016-11-03 13:49 ` Ville Syrjälä 0 siblings, 1 reply; 44+ messages in thread From: Sharma, Shashank @ 2016-11-03 13:34 UTC (permalink / raw) To: Ville Syrjälä Cc: Jose Abreu, Jia, Lin A, Sharma, Akashdeep, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-03 13:34 ` Sharma, Shashank @ 2016-11-03 13:49 ` Ville Syrjälä 2016-11-03 16:03 ` Daniel Vetter 0 siblings, 1 reply; 44+ messages in thread From: Ville Syrjälä @ 2016-11-03 13:49 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Sharma, Akashdeep, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-03 13:49 ` Ville Syrjälä @ 2016-11-03 16:03 ` Daniel Vetter 2016-11-03 16:19 ` Sharma, Shashank 0 siblings, 1 reply; 44+ messages in thread From: Daniel Vetter @ 2016-11-03 16:03 UTC (permalink / raw) To: Ville Syrjälä Cc: Jose Abreu, Jia, Lin A, Sharma, Akashdeep, Emil Velikov, dri-devel, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-03 16:03 ` Daniel Vetter @ 2016-11-03 16:19 ` Sharma, Shashank 2016-11-07 7:43 ` Sharma, Shashank 0 siblings, 1 reply; 44+ messages in thread From: Sharma, Shashank @ 2016-11-03 16:19 UTC (permalink / raw) To: Daniel Vetter, Ville Syrjälä Cc: Jose Abreu, Jia, Lin A, Sharma, Akashdeep, Emil Velikov, dri-devel, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-03 16:19 ` Sharma, Shashank @ 2016-11-07 7:43 ` Sharma, Shashank 2016-11-07 15:26 ` Emil Velikov 0 siblings, 1 reply; 44+ messages in thread From: Sharma, Shashank @ 2016-11-07 7:43 UTC (permalink / raw) To: Daniel Vetter, Ville Syrjälä Cc: Jose Abreu, Jia, Lin A, Sharma, Akashdeep, Emil Velikov, dri-devel, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-07 7:43 ` Sharma, Shashank @ 2016-11-07 15:26 ` Emil Velikov 2016-11-07 15:48 ` Sharma, Shashank 0 siblings, 1 reply; 44+ messages in thread From: Emil Velikov @ 2016-11-07 15:26 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Daniel Vetter, dri-devel, Sharma, Akashdeep, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-07 15:26 ` Emil Velikov @ 2016-11-07 15:48 ` Sharma, Shashank 2016-11-07 16:57 ` Emil Velikov 0 siblings, 1 reply; 44+ messages in thread From: Sharma, Shashank @ 2016-11-07 15:48 UTC (permalink / raw) To: Emil Velikov Cc: Jose Abreu, Jia, Lin A, Daniel Vetter, dri-devel, Sharma, Akashdeep, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-07 15:48 ` Sharma, Shashank @ 2016-11-07 16:57 ` Emil Velikov 0 siblings, 0 replies; 44+ messages in thread From: Emil Velikov @ 2016-11-07 16:57 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Daniel Vetter, dri-devel, Sharma, Akashdeep, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" [not found] ` <FF3DDC77922A8A4BB08A3BC48A1EA8CB411AE158@BGSMSX101.gar.corp.intel.com> [not found] ` <20161103130253.GM4617@intel.com> @ 2016-11-03 16:02 ` Daniel Vetter 1 sibling, 0 replies; 44+ messages in thread From: Daniel Vetter @ 2016-11-03 16:02 UTC (permalink / raw) To: Sharma, Shashank, intel-gfx, dri-devel Cc: Jose Abreu, Jia, Lin A, Sharma, Akashdeep, Emil Velikov, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-03 12:31 [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" ville.syrjala 2016-11-03 12:31 ` [PATCH 2/2] Revert "drm: Add aspect ratio parsing " ville.syrjala [not found] ` <FF3DDC77922A8A4BB08A3BC48A1EA8CB411AE158@BGSMSX101.gar.corp.intel.com> @ 2016-11-11 17:00 ` Ville Syrjälä 2016-11-11 17:03 ` Ville Syrjälä ` (2 more replies) 2 siblings, 3 replies; 44+ messages in thread From: Ville Syrjälä @ 2016-11-11 17:00 UTC (permalink / raw) To: dri-devel Cc: Jose Abreu, Jia, Akashdeep Sharma, Lin, Emil Velikov, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-11 17:00 ` Ville Syrjälä @ 2016-11-11 17:03 ` Ville Syrjälä 2016-11-11 17:07 ` Alex Deucher 2016-11-14 14:44 ` Sharma, Shashank 2 siblings, 0 replies; 44+ messages in thread From: Ville Syrjälä @ 2016-11-11 17:03 UTC (permalink / raw) To: dri-devel Cc: Jose Abreu, Jia, Akashdeep Sharma, Emil Velikov, Daniel Vetter, Lin, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-11 17:00 ` Ville Syrjälä 2016-11-11 17:03 ` Ville Syrjälä @ 2016-11-11 17:07 ` Alex Deucher 2016-11-11 17:08 ` Daniel Vetter 2016-11-11 17:22 ` Ville Syrjälä 2016-11-14 14:44 ` Sharma, Shashank 2 siblings, 2 replies; 44+ messages in thread From: Alex Deucher @ 2016-11-11 17:07 UTC (permalink / raw) To: Ville Syrjälä Cc: Jose Abreu, Jia, Akashdeep Sharma, Emil Velikov, Maling list - DRI developers, Daniel Vetter, Lin, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-11 17:07 ` Alex Deucher @ 2016-11-11 17:08 ` Daniel Vetter 2016-11-11 17:22 ` Ville Syrjälä 1 sibling, 0 replies; 44+ messages in thread From: Daniel Vetter @ 2016-11-11 17:08 UTC (permalink / raw) To: Alex Deucher Cc: Jose Abreu, Jia, Akashdeep Sharma, Emil Velikov, Maling list - DRI developers, Jim Bride, Lin 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-11 17:07 ` Alex Deucher 2016-11-11 17:08 ` Daniel Vetter @ 2016-11-11 17:22 ` Ville Syrjälä 1 sibling, 0 replies; 44+ messages in thread From: Ville Syrjälä @ 2016-11-11 17:22 UTC (permalink / raw) To: Alex Deucher Cc: Jose Abreu, Jia, Akashdeep Sharma, Emil Velikov, Maling list - DRI developers, Daniel Vetter, Lin, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-11 17:00 ` Ville Syrjälä 2016-11-11 17:03 ` Ville Syrjälä 2016-11-11 17:07 ` Alex Deucher @ 2016-11-14 14:44 ` Sharma, Shashank 2016-11-14 15:49 ` Ville Syrjälä 2 siblings, 1 reply; 44+ messages in thread From: Sharma, Shashank @ 2016-11-14 14:44 UTC (permalink / raw) To: Ville Syrjälä, dri-devel Cc: Jose Abreu, Jia, Akashdeep Sharma, Emil Velikov, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-14 14:44 ` Sharma, Shashank @ 2016-11-14 15:49 ` Ville Syrjälä 2016-11-14 16:07 ` Sharma, Shashank 0 siblings, 1 reply; 44+ messages in thread From: Ville Syrjälä @ 2016-11-14 15:49 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-14 15:49 ` Ville Syrjälä @ 2016-11-14 16:07 ` Sharma, Shashank 2016-11-14 16:20 ` Ville Syrjälä 0 siblings, 1 reply; 44+ messages in thread From: Sharma, Shashank @ 2016-11-14 16:07 UTC (permalink / raw) To: Ville Syrjälä Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-14 16:07 ` Sharma, Shashank @ 2016-11-14 16:20 ` Ville Syrjälä 2016-11-14 16:42 ` Sharma, Shashank 0 siblings, 1 reply; 44+ messages in thread From: Ville Syrjälä @ 2016-11-14 16:20 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-14 16:20 ` Ville Syrjälä @ 2016-11-14 16:42 ` Sharma, Shashank 2016-11-14 16:45 ` Ville Syrjälä 0 siblings, 1 reply; 44+ messages in thread From: Sharma, Shashank @ 2016-11-14 16:42 UTC (permalink / raw) To: Ville Syrjälä Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-14 16:42 ` Sharma, Shashank @ 2016-11-14 16:45 ` Ville Syrjälä 2016-11-14 16:56 ` Sharma, Shashank 0 siblings, 1 reply; 44+ messages in thread From: Ville Syrjälä @ 2016-11-14 16:45 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-14 16:45 ` Ville Syrjälä @ 2016-11-14 16:56 ` Sharma, Shashank 2016-11-15 8:51 ` Daniel Vetter 0 siblings, 1 reply; 44+ messages in thread From: Sharma, Shashank @ 2016-11-14 16:56 UTC (permalink / raw) To: Ville Syrjälä Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-14 16:56 ` Sharma, Shashank @ 2016-11-15 8:51 ` Daniel Vetter 2016-11-15 9:00 ` Sharma, Shashank 0 siblings, 1 reply; 44+ messages in thread From: Daniel Vetter @ 2016-11-15 8:51 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 8:51 ` Daniel Vetter @ 2016-11-15 9:00 ` Sharma, Shashank 2016-11-15 10:00 ` Daniel Vetter 0 siblings, 1 reply; 44+ messages in thread From: Sharma, Shashank @ 2016-11-15 9:00 UTC (permalink / raw) To: Daniel Vetter Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 9:00 ` Sharma, Shashank @ 2016-11-15 10:00 ` Daniel Vetter 2016-11-15 10:06 ` Sharma, Shashank 2016-11-15 13:54 ` Ville Syrjälä 0 siblings, 2 replies; 44+ messages in thread From: Daniel Vetter @ 2016-11-15 10:00 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 10:00 ` Daniel Vetter @ 2016-11-15 10:06 ` Sharma, Shashank 2016-11-15 10:52 ` Daniel Vetter 2016-11-15 13:54 ` Ville Syrjälä 1 sibling, 1 reply; 44+ messages in thread From: Sharma, Shashank @ 2016-11-15 10:06 UTC (permalink / raw) To: Daniel Vetter Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 10:06 ` Sharma, Shashank @ 2016-11-15 10:52 ` Daniel Vetter 2016-11-15 13:48 ` Jose Abreu 0 siblings, 1 reply; 44+ messages in thread From: Daniel Vetter @ 2016-11-15 10:52 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 10:52 ` Daniel Vetter @ 2016-11-15 13:48 ` Jose Abreu 2016-11-15 14:18 ` Ville Syrjälä 0 siblings, 1 reply; 44+ messages in thread From: Jose Abreu @ 2016-11-15 13:48 UTC (permalink / raw) To: Daniel Vetter, Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 13:48 ` Jose Abreu @ 2016-11-15 14:18 ` Ville Syrjälä 2016-11-15 15:10 ` Sharma, Shashank 2016-11-15 15:13 ` Sharma, Shashank 0 siblings, 2 replies; 44+ messages in thread From: Ville Syrjälä @ 2016-11-15 14:18 UTC (permalink / raw) To: Jose Abreu Cc: Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 14:18 ` Ville Syrjälä @ 2016-11-15 15:10 ` Sharma, Shashank 2016-11-15 15:18 ` Ville Syrjälä 2016-11-15 15:13 ` Sharma, Shashank 1 sibling, 1 reply; 44+ messages in thread From: Sharma, Shashank @ 2016-11-15 15:10 UTC (permalink / raw) To: Ville Syrjälä, Jose Abreu Cc: Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 15:10 ` Sharma, Shashank @ 2016-11-15 15:18 ` Ville Syrjälä 0 siblings, 0 replies; 44+ messages in thread From: Ville Syrjälä @ 2016-11-15 15:18 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 14:18 ` Ville Syrjälä 2016-11-15 15:10 ` Sharma, Shashank @ 2016-11-15 15:13 ` Sharma, Shashank 1 sibling, 0 replies; 44+ messages in thread From: Sharma, Shashank @ 2016-11-15 15:13 UTC (permalink / raw) To: Ville Syrjälä, Jose Abreu Cc: Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 10:00 ` Daniel Vetter 2016-11-15 10:06 ` Sharma, Shashank @ 2016-11-15 13:54 ` Ville Syrjälä 2016-11-15 14:03 ` Daniel Vetter 1 sibling, 1 reply; 44+ messages in thread From: Ville Syrjälä @ 2016-11-15 13:54 UTC (permalink / raw) To: Daniel Vetter Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 13:54 ` Ville Syrjälä @ 2016-11-15 14:03 ` Daniel Vetter 2016-11-15 14:18 ` Alex Deucher 0 siblings, 1 reply; 44+ messages in thread From: Daniel Vetter @ 2016-11-15 14:03 UTC (permalink / raw) To: Ville Syrjälä Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 14:03 ` Daniel Vetter @ 2016-11-15 14:18 ` Alex Deucher 2016-11-15 14:24 ` Ville Syrjälä 2016-11-15 14:26 ` Daniel Vetter 0 siblings, 2 replies; 44+ messages in thread From: Alex Deucher @ 2016-11-15 14:18 UTC (permalink / raw) To: Daniel Vetter Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, Maling list - DRI developers, Jim Bride, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 14:18 ` Alex Deucher @ 2016-11-15 14:24 ` Ville Syrjälä 2016-11-15 14:26 ` Daniel Vetter 1 sibling, 0 replies; 44+ messages in thread From: Ville Syrjälä @ 2016-11-15 14:24 UTC (permalink / raw) To: Alex Deucher Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, Maling list - DRI developers, Daniel Vetter, Jim Bride 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 14:18 ` Alex Deucher 2016-11-15 14:24 ` Ville Syrjälä @ 2016-11-15 14:26 ` Daniel Vetter 2016-11-15 15:26 ` Alex Deucher 1 sibling, 1 reply; 44+ messages in thread From: Daniel Vetter @ 2016-11-15 14:26 UTC (permalink / raw) To: Alex Deucher Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, Maling list - DRI developers, Jim Bride, 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" 2016-11-15 14:26 ` Daniel Vetter @ 2016-11-15 15:26 ` Alex Deucher 0 siblings, 0 replies; 44+ messages in thread From: Alex Deucher @ 2016-11-15 15:26 UTC (permalink / raw) To: Daniel Vetter Cc: Jose Abreu, Jia, Lin A, Akashdeep Sharma, Emil Velikov, Maling list - DRI developers, Jim Bride, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2016-11-15 15:26 UTC | newest] Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-03 12:31 [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer" ville.syrjala 2016-11-03 12:31 ` [PATCH 2/2] Revert "drm: Add aspect ratio parsing " ville.syrjala 2016-11-03 12:47 ` Sharma, Shashank [not found] ` <FF3DDC77922A8A4BB08A3BC48A1EA8CB411AE158@BGSMSX101.gar.corp.intel.com> [not found] ` <20161103130253.GM4617@intel.com> 2016-11-03 13:04 ` [PATCH 1/2] Revert "drm: Add and handle new aspect ratios " Ville Syrjälä 2016-11-03 13:09 ` Ville Syrjälä 2016-11-03 13:22 ` Sharma, Shashank [not found] ` <2ededbc1-b923-200e-8443-f1ee522402bf@intel.com> 2016-11-03 13:26 ` Ville Syrjälä 2016-11-03 13:34 ` Sharma, Shashank 2016-11-03 13:49 ` Ville Syrjälä 2016-11-03 16:03 ` Daniel Vetter 2016-11-03 16:19 ` Sharma, Shashank 2016-11-07 7:43 ` Sharma, Shashank 2016-11-07 15:26 ` Emil Velikov 2016-11-07 15:48 ` Sharma, Shashank 2016-11-07 16:57 ` Emil Velikov 2016-11-03 16:02 ` Daniel Vetter 2016-11-11 17:00 ` Ville Syrjälä 2016-11-11 17:03 ` Ville Syrjälä 2016-11-11 17:07 ` Alex Deucher 2016-11-11 17:08 ` Daniel Vetter 2016-11-11 17:22 ` Ville Syrjälä 2016-11-14 14:44 ` Sharma, Shashank 2016-11-14 15:49 ` Ville Syrjälä 2016-11-14 16:07 ` Sharma, Shashank 2016-11-14 16:20 ` Ville Syrjälä 2016-11-14 16:42 ` Sharma, Shashank 2016-11-14 16:45 ` Ville Syrjälä 2016-11-14 16:56 ` Sharma, Shashank 2016-11-15 8:51 ` Daniel Vetter 2016-11-15 9:00 ` Sharma, Shashank 2016-11-15 10:00 ` Daniel Vetter 2016-11-15 10:06 ` Sharma, Shashank 2016-11-15 10:52 ` Daniel Vetter 2016-11-15 13:48 ` Jose Abreu 2016-11-15 14:18 ` Ville Syrjälä 2016-11-15 15:10 ` Sharma, Shashank 2016-11-15 15:18 ` Ville Syrjälä 2016-11-15 15:13 ` Sharma, Shashank 2016-11-15 13:54 ` Ville Syrjälä 2016-11-15 14:03 ` Daniel Vetter 2016-11-15 14:18 ` Alex Deucher 2016-11-15 14:24 ` Ville Syrjälä 2016-11-15 14:26 ` Daniel Vetter 2016-11-15 15:26 ` Alex Deucher
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.