| Message ID | 20230815101907.2900768-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "drm/edid: Fix csync detailed mode parsing" | expand |
On Tue, Aug 15, 2023 at 01:19:07PM +0300, Jani Nikula wrote: > This reverts commit ca62297b2085b5b3168bd891ca24862242c635a1. > > Commit ca62297b2085 ("drm/edid: Fix csync detailed mode parsing") fixed > EDID detailed mode sync parsing. Unfortunately, there are quite a few > displays out there that have bogus (zero) sync field that are broken by > the change. Zero means analog composite sync, which is not right for > digital displays, and the modes get rejected. Regardless, it used to > work, and it needs to continue to work. Revert the change. Bah. I guess one option would be to quirk the bogus EDIDs, or maybe just ignore bogus sync flags for the eDP preferred mode. But maybe needs a bit more thinking, so Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Rejecting modes with analog composite sync was the part that fixed the > gitlab issue 8146 [1]. We'll need to get back to the drawing board with > that. > > [1] https://gitlab.freedesktop.org/drm/intel/-/issues/8146 > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8789 > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8930 > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9044 > Fixes: ca62297b2085 ("drm/edid: Fix csync detailed mode parsing") > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: dri-devel@lists.freedesktop.org > Cc: <stable@vger.kernel.org> # v6.4+ > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/drm_edid.c | 29 ++++++++--------------------- > include/drm/drm_edid.h | 12 +++--------- > 2 files changed, 11 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index f95152fac427..340da8257b51 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -3457,6 +3457,10 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto > connector->base.id, connector->name); > return NULL; > } > + if (!(pt->misc & DRM_EDID_PT_SEPARATE_SYNC)) { > + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Composite sync not supported\n", > + connector->base.id, connector->name); > + } > > /* it is incorrect if hsync/vsync width is zero */ > if (!hsync_pulse_width || !vsync_pulse_width) { > @@ -3503,27 +3507,10 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto > if (info->quirks & EDID_QUIRK_DETAILED_SYNC_PP) { > mode->flags |= DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC; > } else { > - switch (pt->misc & DRM_EDID_PT_SYNC_MASK) { > - case DRM_EDID_PT_ANALOG_CSYNC: > - case DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC: > - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Analog composite sync!\n", > - connector->base.id, connector->name); > - mode->flags |= DRM_MODE_FLAG_CSYNC | DRM_MODE_FLAG_NCSYNC; > - break; > - case DRM_EDID_PT_DIGITAL_CSYNC: > - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Digital composite sync!\n", > - connector->base.id, connector->name); > - mode->flags |= DRM_MODE_FLAG_CSYNC; > - mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? > - DRM_MODE_FLAG_PCSYNC : DRM_MODE_FLAG_NCSYNC; > - break; > - case DRM_EDID_PT_DIGITAL_SEPARATE_SYNC: > - mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? > - DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC; > - mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ? > - DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC; > - break; > - } > + mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? > + DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC; > + mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ? > + DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC; > } > > set_size: > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > index 169755d3de19..48e93f909ef6 100644 > --- a/include/drm/drm_edid.h > +++ b/include/drm/drm_edid.h > @@ -61,15 +61,9 @@ struct std_timing { > u8 vfreq_aspect; > } __attribute__((packed)); > > -#define DRM_EDID_PT_SYNC_MASK (3 << 3) > -# define DRM_EDID_PT_ANALOG_CSYNC (0 << 3) > -# define DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC (1 << 3) > -# define DRM_EDID_PT_DIGITAL_CSYNC (2 << 3) > -# define DRM_EDID_PT_CSYNC_ON_RGB (1 << 1) /* analog csync only */ > -# define DRM_EDID_PT_CSYNC_SERRATE (1 << 2) > -# define DRM_EDID_PT_DIGITAL_SEPARATE_SYNC (3 << 3) > -# define DRM_EDID_PT_HSYNC_POSITIVE (1 << 1) /* also digital csync */ > -# define DRM_EDID_PT_VSYNC_POSITIVE (1 << 2) > +#define DRM_EDID_PT_HSYNC_POSITIVE (1 << 1) > +#define DRM_EDID_PT_VSYNC_POSITIVE (1 << 2) > +#define DRM_EDID_PT_SEPARATE_SYNC (3 << 3) > #define DRM_EDID_PT_STEREO (1 << 5) > #define DRM_EDID_PT_INTERLACED (1 << 7) > > -- > 2.39.2
On Thu, 17 Aug 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Aug 15, 2023 at 01:19:07PM +0300, Jani Nikula wrote: >> This reverts commit ca62297b2085b5b3168bd891ca24862242c635a1. >> >> Commit ca62297b2085 ("drm/edid: Fix csync detailed mode parsing") fixed >> EDID detailed mode sync parsing. Unfortunately, there are quite a few >> displays out there that have bogus (zero) sync field that are broken by >> the change. Zero means analog composite sync, which is not right for >> digital displays, and the modes get rejected. Regardless, it used to >> work, and it needs to continue to work. Revert the change. > > Bah. I guess one option would be to quirk the bogus EDIDs, or maybe just > ignore bogus sync flags for the eDP preferred mode. But maybe needs a > bit more thinking, so > > Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks, pushed to drm-misc-fixes. BR, Jani. > >> >> Rejecting modes with analog composite sync was the part that fixed the >> gitlab issue 8146 [1]. We'll need to get back to the drawing board with >> that. >> >> [1] https://gitlab.freedesktop.org/drm/intel/-/issues/8146 >> >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8789 >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8930 >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9044 >> Fixes: ca62297b2085 ("drm/edid: Fix csync detailed mode parsing") >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: dri-devel@lists.freedesktop.org >> Cc: <stable@vger.kernel.org> # v6.4+ >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/drm_edid.c | 29 ++++++++--------------------- >> include/drm/drm_edid.h | 12 +++--------- >> 2 files changed, 11 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> index f95152fac427..340da8257b51 100644 >> --- a/drivers/gpu/drm/drm_edid.c >> +++ b/drivers/gpu/drm/drm_edid.c >> @@ -3457,6 +3457,10 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto >> connector->base.id, connector->name); >> return NULL; >> } >> + if (!(pt->misc & DRM_EDID_PT_SEPARATE_SYNC)) { >> + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Composite sync not supported\n", >> + connector->base.id, connector->name); >> + } >> >> /* it is incorrect if hsync/vsync width is zero */ >> if (!hsync_pulse_width || !vsync_pulse_width) { >> @@ -3503,27 +3507,10 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto >> if (info->quirks & EDID_QUIRK_DETAILED_SYNC_PP) { >> mode->flags |= DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC; >> } else { >> - switch (pt->misc & DRM_EDID_PT_SYNC_MASK) { >> - case DRM_EDID_PT_ANALOG_CSYNC: >> - case DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC: >> - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Analog composite sync!\n", >> - connector->base.id, connector->name); >> - mode->flags |= DRM_MODE_FLAG_CSYNC | DRM_MODE_FLAG_NCSYNC; >> - break; >> - case DRM_EDID_PT_DIGITAL_CSYNC: >> - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Digital composite sync!\n", >> - connector->base.id, connector->name); >> - mode->flags |= DRM_MODE_FLAG_CSYNC; >> - mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? >> - DRM_MODE_FLAG_PCSYNC : DRM_MODE_FLAG_NCSYNC; >> - break; >> - case DRM_EDID_PT_DIGITAL_SEPARATE_SYNC: >> - mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? >> - DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC; >> - mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ? >> - DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC; >> - break; >> - } >> + mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? >> + DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC; >> + mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ? >> + DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC; >> } >> >> set_size: >> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h >> index 169755d3de19..48e93f909ef6 100644 >> --- a/include/drm/drm_edid.h >> +++ b/include/drm/drm_edid.h >> @@ -61,15 +61,9 @@ struct std_timing { >> u8 vfreq_aspect; >> } __attribute__((packed)); >> >> -#define DRM_EDID_PT_SYNC_MASK (3 << 3) >> -# define DRM_EDID_PT_ANALOG_CSYNC (0 << 3) >> -# define DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC (1 << 3) >> -# define DRM_EDID_PT_DIGITAL_CSYNC (2 << 3) >> -# define DRM_EDID_PT_CSYNC_ON_RGB (1 << 1) /* analog csync only */ >> -# define DRM_EDID_PT_CSYNC_SERRATE (1 << 2) >> -# define DRM_EDID_PT_DIGITAL_SEPARATE_SYNC (3 << 3) >> -# define DRM_EDID_PT_HSYNC_POSITIVE (1 << 1) /* also digital csync */ >> -# define DRM_EDID_PT_VSYNC_POSITIVE (1 << 2) >> +#define DRM_EDID_PT_HSYNC_POSITIVE (1 << 1) >> +#define DRM_EDID_PT_VSYNC_POSITIVE (1 << 2) >> +#define DRM_EDID_PT_SEPARATE_SYNC (3 << 3) >> #define DRM_EDID_PT_STEREO (1 << 5) >> #define DRM_EDID_PT_INTERLACED (1 << 7) >> >> -- >> 2.39.2
On Thu, 17 Aug 2023, Dirk Lehmann <develop@dj-l.de> wrote: > VESA Enhanced EDID Standard does not clearly describe how display > panel vendors should setup the Sync Signal Defintions (bit 4 & 3) in > the Detailed Timing Definition (relative offset 17, absolute offset > 47h[+18]) for Digital Video Signal Interfaces (bit 7 at offset 14h). > > In practice many eDP panels which using a Digital Video Signal > Interfaces (bit 7 at offset 14h == 1) are leaving the Sync Signal > Defintions at offset 47h[+18] blank 0x00, which would mean concerned > with the VESA Standard [1] that they are using "Analog Composite > Sync". > > Fix: Just detect Analog Sync Signal if an Analog Video Signal > Interface (bit 7 at offset 14h == 0) is in use. Just detect > Digital Sync Signal if an Digital Video Signal Interface is in > use. > > Reference: [1] VESA Enhanced EDID Standard, Release A, Rev.2, Page 35 Please don't reply with patches in-reply-to other people's patches. > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8789 This is now fixed by the revert that I just pushed. > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8146 I don't think this patch fixes that one; see below. > Signed-off-by: Dirk Lehmann <develop@dj-l.de> > --- > drivers/gpu/drm/drm_edid.c | 74 ++++++++++++++++++++++++++++++++------ > include/drm/drm_edid.h | 12 +++++-- > 2 files changed, 73 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 1f470968ed14..6afdc34e55ce 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -3437,6 +3437,7 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto > const struct drm_display_info *info = &connector->display_info; > struct drm_device *dev = connector->dev; > struct drm_display_mode *mode; > + const struct edid *edid = drm_edid->edid; > const struct detailed_pixel_timing *pt = &timing->data.pixel_data; > unsigned hactive = (pt->hactive_hblank_hi & 0xf0) << 4 | pt->hactive_lo; > unsigned vactive = (pt->vactive_vblank_hi & 0xf0) << 4 | pt->vactive_lo; > @@ -3456,10 +3457,6 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto > connector->base.id, connector->name); > return NULL; > } > - if (!(pt->misc & DRM_EDID_PT_SEPARATE_SYNC)) { > - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Composite sync not supported\n", > - connector->base.id, connector->name); > - } > > /* it is incorrect if hsync/vsync width is zero */ > if (!hsync_pulse_width || !vsync_pulse_width) { > @@ -3505,11 +3502,68 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto > > if (info->quirks & EDID_QUIRK_DETAILED_SYNC_PP) { > mode->flags |= DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC; > + } else if (edid->input & DRM_EDID_INPUT_DIGITAL) { > + /* !info->quirks && edid->input == DIGITAL */ > + switch (pt->misc & DRM_EDID_PT_SYNC_MASK) { > + /* VESA Enhanced EDID Standard, Release A, Rev.2, Page 35 > + * > + * CASE DRM_EDID_PT_ANALOG_CSYNC: > + * > + * (pt->misc & DRM_EDID_PT_SYNC_MASK == 0x00) means > + * "Analog Composite Sync" as described in VESA > + * Standard. But many digital display panels without > + * composite sync are also using 0x00 here. > + * > + * Therefore use DEFAULT: as we are currently on an > + * digital video signal interface. > + */ > + case DRM_EDID_PT_DIGITAL_CSYNC: > + drm_dbg_kms(dev, > + "[CONNECTOR:%d:%s] Digital composite sync!\n", > + connector->base.id, connector->name); > + mode->flags |= DRM_MODE_FLAG_CSYNC; > + mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? > + DRM_MODE_FLAG_PCSYNC : DRM_MODE_FLAG_NCSYNC; > + break; > + case DRM_EDID_PT_DIGITAL_SEPARATE_SYNC: > + drm_dbg_kms(dev, > + "[CONNECTOR:%d:%s] Digital seperate sync!\n", > + connector->base.id, connector->name); > + goto digital_default; > + break; /* Missing BREAK throws a compiler warning */ fallthrough; will do the trick. > + default: > +digital_default: > + mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? > + DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC; > + mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ? > + DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC; The failing mode in bug [1] has 0x0a for flags, so it ends up here, and for that display this patch is a no-op. As I explained in [2], all the problematic cases have invalid data, but the ones fixed by the revert need to ignore the analog sync *flags* and accept the mode, and the display in [1] needs to have the whole *mode* rejected. [1] https://gitlab.freedesktop.org/drm/intel/-/issues/8146 [2] https://gitlab.freedesktop.org/drm/intel/-/issues/8789#note_2047902 > + break; > + } > } else { > - mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? > - DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC; > - mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ? > - DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC; > + /* !info->quirks && edid->input == ANALOG */ > + switch (pt->misc & DRM_EDID_PT_SYNC_MASK) { > + /* VESA Enhanced EDID Standard, Release A, Rev.2, Page 35 > + * > + * CASE DRM_EDID_PT_ANALOG_CSYNC: > + * > + * (pt->misc & DRM_EDID_PT_SYNC_MASK == 0x00) for > + * "Analog Composite Sync" is possible here, as we are > + * currently on an analog video signal interface. > + */ > + case DRM_EDID_PT_ANALOG_CSYNC: > + case DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC: > + drm_dbg_kms(dev, > + "[CONNECTOR:%d:%s] Analog composite sync!\n", > + connector->base.id, connector->name); > + mode->flags |= DRM_MODE_FLAG_CSYNC | DRM_MODE_FLAG_NCSYNC; > + break; > + default: > + mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? > + DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC; > + mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ? > + DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC; > + break; > + } Another angle is that for analog displays, EDID offset 0x14 has info about the supported sync modes. If we're separating digital/analog sync handling, we should probably filter the analog sync with the info from 0x14. BR, Jani. > } > > set_size: > @@ -3522,8 +3576,8 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto > } > > if (info->quirks & EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE) { > - mode->width_mm = drm_edid->edid->width_cm * 10; > - mode->height_mm = drm_edid->edid->height_cm * 10; > + mode->width_mm = edid->width_cm * 10; > + mode->height_mm = edid->height_cm * 10; > } > > mode->type = DRM_MODE_TYPE_DRIVER; > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > index 48e93f909ef6..169755d3de19 100644 > --- a/include/drm/drm_edid.h > +++ b/include/drm/drm_edid.h > @@ -61,9 +61,15 @@ struct std_timing { > u8 vfreq_aspect; > } __attribute__((packed)); > > -#define DRM_EDID_PT_HSYNC_POSITIVE (1 << 1) > -#define DRM_EDID_PT_VSYNC_POSITIVE (1 << 2) > -#define DRM_EDID_PT_SEPARATE_SYNC (3 << 3) > +#define DRM_EDID_PT_SYNC_MASK (3 << 3) > +# define DRM_EDID_PT_ANALOG_CSYNC (0 << 3) > +# define DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC (1 << 3) > +# define DRM_EDID_PT_DIGITAL_CSYNC (2 << 3) > +# define DRM_EDID_PT_CSYNC_ON_RGB (1 << 1) /* analog csync only */ > +# define DRM_EDID_PT_CSYNC_SERRATE (1 << 2) > +# define DRM_EDID_PT_DIGITAL_SEPARATE_SYNC (3 << 3) > +# define DRM_EDID_PT_HSYNC_POSITIVE (1 << 1) /* also digital csync */ > +# define DRM_EDID_PT_VSYNC_POSITIVE (1 << 2) > #define DRM_EDID_PT_STEREO (1 << 5) > #define DRM_EDID_PT_INTERLACED (1 << 7)
Hey Jani, On 8/17/23 15:05, Jani Nikula wrote: > On Thu, 17 Aug 2023, Dirk Lehmann <develop@dj-l.de> wrote: >> VESA Enhanced EDID Standard does not clearly describe how display >> panel vendors should setup the Sync Signal Defintions (bit 4 & 3) in >> the Detailed Timing Definition (relative offset 17, absolute offset >> 47h[+18]) for Digital Video Signal Interfaces (bit 7 at offset 14h). >> >> In practice many eDP panels which using a Digital Video Signal >> Interfaces (bit 7 at offset 14h == 1) are leaving the Sync Signal >> Defintions at offset 47h[+18] blank 0x00, which would mean concerned >> with the VESA Standard [1] that they are using "Analog Composite >> Sync". >> >> Fix: Just detect Analog Sync Signal if an Analog Video Signal >> Interface (bit 7 at offset 14h == 0) is in use. Just detect >> Digital Sync Signal if an Digital Video Signal Interface is in >> use. >> >> Reference: [1] VESA Enhanced EDID Standard, Release A, Rev.2, Page 35 > > Please don't reply with patches in-reply-to other people's patches. Sorry, I am new with such patch lists :/ ... > >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8789 > > This is now fixed by the revert that I just pushed. > >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8146 > > I don't think this patch fixes that one; see below. > >> Signed-off-by: Dirk Lehmann <develop@dj-l.de> >> --- >> drivers/gpu/drm/drm_edid.c | 74 ++++++++++++++++++++++++++++++++------ >> include/drm/drm_edid.h | 12 +++++-- >> 2 files changed, 73 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> index 1f470968ed14..6afdc34e55ce 100644 >> --- a/drivers/gpu/drm/drm_edid.c >> +++ b/drivers/gpu/drm/drm_edid.c >> @@ -3437,6 +3437,7 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto >> const struct drm_display_info *info = &connector->display_info; >> struct drm_device *dev = connector->dev; >> struct drm_display_mode *mode; >> + const struct edid *edid = drm_edid->edid; >> const struct detailed_pixel_timing *pt = &timing->data.pixel_data; >> unsigned hactive = (pt->hactive_hblank_hi & 0xf0) << 4 | pt->hactive_lo; >> unsigned vactive = (pt->vactive_vblank_hi & 0xf0) << 4 | pt->vactive_lo; >> @@ -3456,10 +3457,6 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto >> connector->base.id, connector->name); >> return NULL; >> } >> - if (!(pt->misc & DRM_EDID_PT_SEPARATE_SYNC)) { >> - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Composite sync not supported\n", >> - connector->base.id, connector->name); >> - } >> >> /* it is incorrect if hsync/vsync width is zero */ >> if (!hsync_pulse_width || !vsync_pulse_width) { >> @@ -3505,11 +3502,68 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto >> >> if (info->quirks & EDID_QUIRK_DETAILED_SYNC_PP) { >> mode->flags |= DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC; >> + } else if (edid->input & DRM_EDID_INPUT_DIGITAL) { >> + /* !info->quirks && edid->input == DIGITAL */ >> + switch (pt->misc & DRM_EDID_PT_SYNC_MASK) { >> + /* VESA Enhanced EDID Standard, Release A, Rev.2, Page 35 >> + * >> + * CASE DRM_EDID_PT_ANALOG_CSYNC: >> + * >> + * (pt->misc & DRM_EDID_PT_SYNC_MASK == 0x00) means >> + * "Analog Composite Sync" as described in VESA >> + * Standard. But many digital display panels without >> + * composite sync are also using 0x00 here. >> + * >> + * Therefore use DEFAULT: as we are currently on an >> + * digital video signal interface. >> + */ >> + case DRM_EDID_PT_DIGITAL_CSYNC: >> + drm_dbg_kms(dev, >> + "[CONNECTOR:%d:%s] Digital composite sync!\n", >> + connector->base.id, connector->name); >> + mode->flags |= DRM_MODE_FLAG_CSYNC; >> + mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? >> + DRM_MODE_FLAG_PCSYNC : DRM_MODE_FLAG_NCSYNC; >> + break; >> + case DRM_EDID_PT_DIGITAL_SEPARATE_SYNC: >> + drm_dbg_kms(dev, >> + "[CONNECTOR:%d:%s] Digital seperate sync!\n", >> + connector->base.id, connector->name); >> + goto digital_default; >> + break; /* Missing BREAK throws a compiler warning */ > > fallthrough; will do the trick. Cool, that's new for me. FALLTHROUGH really works, great :) > >> + default: >> +digital_default: >> + mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? >> + DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC; >> + mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ? >> + DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC; > > The failing mode in bug [1] has 0x0a for flags, so it ends up here, and > for that display this patch is a no-op. Aaaeem yes, something is wrong here. If the EDID data are correct, then since 0001-*_5.15.patch in [1] should broke the implementation. (0x1a & (3 << 3)) == (3 << 3) == DRM_EDID_PT_DIGITAL_SEPARATE_SYNC But this implementation here I made and we are discussing should be have the same behavior as before in that case of [1]. The no-op should also be before. But then the fix ca62297b2085b before is useless. Yes, you are right, then reverting is the better idea xD > > As I explained in [2], all the problematic cases have invalid data, but > the ones fixed by the revert need to ignore the analog sync *flags* and > accept the mode, and the display in [1] needs to have the whole *mode* > rejected. > I don't know about the other problematic cases. I don't have the data of the use cases for that. > [1] https://gitlab.freedesktop.org/drm/intel/-/issues/8146 > [2] https://gitlab.freedesktop.org/drm/intel/-/issues/8789#note_2047902 > >> + break; >> + } >> } else { >> - mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? >> - DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC; >> - mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ? >> - DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC; >> + /* !info->quirks && edid->input == ANALOG */ >> + switch (pt->misc & DRM_EDID_PT_SYNC_MASK) { >> + /* VESA Enhanced EDID Standard, Release A, Rev.2, Page 35 >> + * >> + * CASE DRM_EDID_PT_ANALOG_CSYNC: >> + * >> + * (pt->misc & DRM_EDID_PT_SYNC_MASK == 0x00) for >> + * "Analog Composite Sync" is possible here, as we are >> + * currently on an analog video signal interface. >> + */ >> + case DRM_EDID_PT_ANALOG_CSYNC: >> + case DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC: >> + drm_dbg_kms(dev, >> + "[CONNECTOR:%d:%s] Analog composite sync!\n", >> + connector->base.id, connector->name); >> + mode->flags |= DRM_MODE_FLAG_CSYNC | DRM_MODE_FLAG_NCSYNC; >> + break; >> + default: >> + mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? >> + DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC; >> + mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ? >> + DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC; >> + break; >> + } > > Another angle is that for analog displays, EDID offset 0x14 has info > about the supported sync modes. If we're separating digital/analog sync > handling, we should probably filter the analog sync with the info from > 0x14. Yeah I also saw that in the VESA Standard, but I reflected that the analog display implementation should be more stable, as they are longer exist. For current digital eDP panels which have 0x00 at 47h are these composite bits at 14h not reachable --- because bit 7 of 14h is 1. It may just have effects on older analog displays. > > BR, > Jani. > Okay, summary: You may be right, as the fix commit ca62297b2085b makes less sense. Yeah, greets and thanks for the fast fix. Really great =D Dirk > >> } >> >> set_size: >> @@ -3522,8 +3576,8 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto >> } >> >> if (info->quirks & EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE) { >> - mode->width_mm = drm_edid->edid->width_cm * 10; >> - mode->height_mm = drm_edid->edid->height_cm * 10; >> + mode->width_mm = edid->width_cm * 10; >> + mode->height_mm = edid->height_cm * 10; >> } >> >> mode->type = DRM_MODE_TYPE_DRIVER; >> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h >> index 48e93f909ef6..169755d3de19 100644 >> --- a/include/drm/drm_edid.h >> +++ b/include/drm/drm_edid.h >> @@ -61,9 +61,15 @@ struct std_timing { >> u8 vfreq_aspect; >> } __attribute__((packed)); >> >> -#define DRM_EDID_PT_HSYNC_POSITIVE (1 << 1) >> -#define DRM_EDID_PT_VSYNC_POSITIVE (1 << 2) >> -#define DRM_EDID_PT_SEPARATE_SYNC (3 << 3) >> +#define DRM_EDID_PT_SYNC_MASK (3 << 3) >> +# define DRM_EDID_PT_ANALOG_CSYNC (0 << 3) >> +# define DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC (1 << 3) >> +# define DRM_EDID_PT_DIGITAL_CSYNC (2 << 3) >> +# define DRM_EDID_PT_CSYNC_ON_RGB (1 << 1) /* analog csync only */ >> +# define DRM_EDID_PT_CSYNC_SERRATE (1 << 2) >> +# define DRM_EDID_PT_DIGITAL_SEPARATE_SYNC (3 << 3) >> +# define DRM_EDID_PT_HSYNC_POSITIVE (1 << 1) /* also digital csync */ >> +# define DRM_EDID_PT_VSYNC_POSITIVE (1 << 2) >> #define DRM_EDID_PT_STEREO (1 << 5) >> #define DRM_EDID_PT_INTERLACED (1 << 7) >
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index f95152fac427..340da8257b51 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3457,6 +3457,10 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto connector->base.id, connector->name); return NULL; } + if (!(pt->misc & DRM_EDID_PT_SEPARATE_SYNC)) { + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Composite sync not supported\n", + connector->base.id, connector->name); + } /* it is incorrect if hsync/vsync width is zero */ if (!hsync_pulse_width || !vsync_pulse_width) { @@ -3503,27 +3507,10 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connecto if (info->quirks & EDID_QUIRK_DETAILED_SYNC_PP) { mode->flags |= DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC; } else { - switch (pt->misc & DRM_EDID_PT_SYNC_MASK) { - case DRM_EDID_PT_ANALOG_CSYNC: - case DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC: - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Analog composite sync!\n", - connector->base.id, connector->name); - mode->flags |= DRM_MODE_FLAG_CSYNC | DRM_MODE_FLAG_NCSYNC; - break; - case DRM_EDID_PT_DIGITAL_CSYNC: - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Digital composite sync!\n", - connector->base.id, connector->name); - mode->flags |= DRM_MODE_FLAG_CSYNC; - mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? - DRM_MODE_FLAG_PCSYNC : DRM_MODE_FLAG_NCSYNC; - break; - case DRM_EDID_PT_DIGITAL_SEPARATE_SYNC: - mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? - DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC; - mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ? - DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC; - break; - } + mode->flags |= (pt->misc & DRM_EDID_PT_HSYNC_POSITIVE) ? + DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC; + mode->flags |= (pt->misc & DRM_EDID_PT_VSYNC_POSITIVE) ? + DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC; } set_size: diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 169755d3de19..48e93f909ef6 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -61,15 +61,9 @@ struct std_timing { u8 vfreq_aspect; } __attribute__((packed)); -#define DRM_EDID_PT_SYNC_MASK (3 << 3) -# define DRM_EDID_PT_ANALOG_CSYNC (0 << 3) -# define DRM_EDID_PT_BIPOLAR_ANALOG_CSYNC (1 << 3) -# define DRM_EDID_PT_DIGITAL_CSYNC (2 << 3) -# define DRM_EDID_PT_CSYNC_ON_RGB (1 << 1) /* analog csync only */ -# define DRM_EDID_PT_CSYNC_SERRATE (1 << 2) -# define DRM_EDID_PT_DIGITAL_SEPARATE_SYNC (3 << 3) -# define DRM_EDID_PT_HSYNC_POSITIVE (1 << 1) /* also digital csync */ -# define DRM_EDID_PT_VSYNC_POSITIVE (1 << 2) +#define DRM_EDID_PT_HSYNC_POSITIVE (1 << 1) +#define DRM_EDID_PT_VSYNC_POSITIVE (1 << 2) +#define DRM_EDID_PT_SEPARATE_SYNC (3 << 3) #define DRM_EDID_PT_STEREO (1 << 5) #define DRM_EDID_PT_INTERLACED (1 << 7)
This reverts commit ca62297b2085b5b3168bd891ca24862242c635a1. Commit ca62297b2085 ("drm/edid: Fix csync detailed mode parsing") fixed EDID detailed mode sync parsing. Unfortunately, there are quite a few displays out there that have bogus (zero) sync field that are broken by the change. Zero means analog composite sync, which is not right for digital displays, and the modes get rejected. Regardless, it used to work, and it needs to continue to work. Revert the change. Rejecting modes with analog composite sync was the part that fixed the gitlab issue 8146 [1]. We'll need to get back to the drawing board with that. [1] https://gitlab.freedesktop.org/drm/intel/-/issues/8146 Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8789 Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8930 Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9044 Fixes: ca62297b2085 ("drm/edid: Fix csync detailed mode parsing") Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: dri-devel@lists.freedesktop.org Cc: <stable@vger.kernel.org> # v6.4+ Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/drm_edid.c | 29 ++++++++--------------------- include/drm/drm_edid.h | 12 +++--------- 2 files changed, 11 insertions(+), 30 deletions(-)