On Thu, 17 Aug 2023, Dirk Lehmann <deve...@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 <deve...@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) -- Jani Nikula, Intel Open Source Graphics Center
