| Message ID | 20190227171456.21543-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/edid: If no preferred mode is found assume the first mode is preferred | expand |
On Wed, 2019-02-27 at 19:14 +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Some monitors apparently forget to mark any mode as preferred in the > EDID. In this particular case we have a very generic looking ID > "PNP Model 0 Serial Number 4" / "LVDS 800x600" so a specific quirk > doesn't seem particularly wise. Also the quirk we have > (EDID_QUIRK_FIRST_DETAILED_PREFERRED) is actually defunct so we'd > have to fix it first. > > As a generic fallback let's just mark the first probed mode (which > should be the first detailed mode, assuming there are any) as > preferred. What problem is this trying to fix? Userspace (and drm for that matter) is typically going to pick the first mode in the list anyway if there's none marked as preferred. Not having any preferred modes was pretty common on CRTs IIRC. The other major case I've seen of a monitor with no preferred mode are the early dual-link DVI displays without internal scalers (Apple Cinema, Dell 3007WFP, etc). You end up with 1280x800 first in the list since 2560x1600 doesn't fit in a single DVI link. It might be nice if such monitors decided their preferred mode based on the link capabilities; if you know it's a dual-link capable port, you'd probably prefer 2560x1600. Does it make more sense to run the "infer a preferred mode" logic after we've done mode validation for the output? - ajax
On Wed, Feb 27, 2019 at 03:23:01PM -0500, Adam Jackson wrote: > On Wed, 2019-02-27 at 19:14 +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Some monitors apparently forget to mark any mode as preferred in the > > EDID. In this particular case we have a very generic looking ID > > "PNP Model 0 Serial Number 4" / "LVDS 800x600" so a specific quirk > > doesn't seem particularly wise. Also the quirk we have > > (EDID_QUIRK_FIRST_DETAILED_PREFERRED) is actually defunct so we'd > > have to fix it first. > > > > As a generic fallback let's just mark the first probed mode (which > > should be the first detailed mode, assuming there are any) as > > preferred. > > What problem is this trying to fix? Userspace (and drm for that matter) > is typically going to pick the first mode in the list anyway if there's > none marked as preferred. Not having any preferred modes was pretty > common on CRTs IIRC. Ah sorry, I forgot to mention the original symptoms. The problem is that with no preferred mode i915 decided to get the fixed mode for the panel from the VBT instead. That one turned out to be 1024x768 which made the 800x600 panel somewhat unhappy. So instead of putting this logic into the EDID parser I guess we could just put it into the i915 fixed mode code. But then I suppose we should also fix EDID_QUIRK_FIRST_DETAILED_PREFERRED (assuming it exists for a good reason). > > The other major case I've seen of a monitor with no preferred mode are > the early dual-link DVI displays without internal scalers (Apple > Cinema, Dell 3007WFP, etc). You end up with 1280x800 first in the list > since 2560x1600 doesn't fit in a single DVI link. It might be nice if > such monitors decided their preferred mode based on the link > capabilities; if you know it's a dual-link capable port, you'd probably > prefer 2560x1600. > > Does it make more sense to run the "infer a preferred mode" logic after > we've done mode validation for the output? For this particular case that wouldn't help since then we'd end up picking the 800x600 75Hz EST mode. Well, I'm not sure whether the panel would like that or not. The VBIOS at least selects the 800x600 60Hz EST mode for whatever reason.
On Wed, 2019-02-27 at 23:03 +0200, Ville Syrjälä wrote: > So instead of putting this logic into the EDID parser I guess we > could just put it into the i915 fixed mode code. But then I suppose > we should also fix EDID_QUIRK_FIRST_DETAILED_PREFERRED (assuming it > exists for a good reason). I don't think it has any good reason to exist, tbh. The commit adding it to xserver was for the Philips 107P5, which - being a CRT - would be entirely expected to not have the preferred bit set. We should probably have handled that instead by letting the "target a DPI near 96" logic handle things. - ajax
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 5f142530532a..6c6a93647686 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1828,7 +1828,7 @@ static void edid_fixup_preferred(struct drm_connector *connector, list_for_each_entry_safe(cur_mode, t, &connector->probed_modes, head) { cur_mode->type &= ~DRM_MODE_TYPE_PREFERRED; - if (cur_mode == preferred_mode) + if (cur_mode == preferred_mode || target_refresh == 0) continue; /* Largest mode is preferred */ @@ -1850,6 +1850,18 @@ static void edid_fixup_preferred(struct drm_connector *connector, preferred_mode->type |= DRM_MODE_TYPE_PREFERRED; } +static bool preferred_mode_exists(struct drm_connector *connector) +{ + struct drm_display_mode *mode; + + list_for_each_entry(mode, &connector->probed_modes, head) { + if (mode->type & DRM_MODE_TYPE_PREFERRED) + return true; + } + + return false; +} + static bool mode_is_rb(const struct drm_display_mode *mode) { @@ -4733,7 +4745,8 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF) num_modes += add_inferred_modes(connector, edid); - if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75)) + if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75) || + !preferred_mode_exists(connector)) edid_fixup_preferred(connector, quirks); if (quirks & EDID_QUIRK_FORCE_6BPC)