Re: [PATCH v2 2/2] drm/edid: Fix "Analog composite sync!" for current eDP display panels
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;}
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;}
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)
linux-stable-mirror@lists.linaro.org
-
Dirk Lehmann -
Jani Nikula