| Message ID | 20171213084427.31199-1-keithp@keithp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 13, 2017 at 12:44:26AM -0800, Keith Packard wrote: > There are a set of values in the drm_display_info structure for each > connector which hold information derived from EDID. These are computed > in drm_add_display_info. Before this patch, that was only called in > drm_add_edid_modes. This meant that they were only set when EDID was > present and never reset when EDID was not, as happened when the > display was disconnected. > > One of these fields, non_desktop, is used from > drm_mode_connector_update_edid_property, the function responsible for > assigning the new edid value to the application-visible property. > > Various drivers call these two functions (drm_add_edid_modes and > drm_mode_connector_update_edid_property) in different orders. This > means that even when EDID is present, the drm_display_info fields may > not have been computed at the time that > drm_mode_connector_update_edid_property used the non_desktop value to > set the non_desktop property. > > I've added a public function (drm_reset_display_info) that resets the > drm_display_info field values to default values and then made the > drm_add_display_info function public. These two functions are now > called directly from drm_mode_connector_update_edid_property so that > the drm_display_info fields are always computed from the current EDID > information before being used in that function. > > This means that the drm_display_info values are often computed twice, > once when the EDID property it set and a second time when EDID is used > to compute modes for the device. The alternative would be to uniformly > ensure that the values were computed once before being used, which > would require that all drivers reliably invoke the two paths in the > same order. The computation is inexpensive enough that it seems more > maintainable in the long term to simply compute them in both paths. > > The API to drm_add_display_info has been changed so that it no longer > takes the set of edid-based quirks as a parameter. Rather, it now > computes those quirks itself and returns them for further use by > drm_add_edid_modes. > > This patch also includes a number of 'const' additions caused by > drm_mode_connector_update_edid_property taking a 'const struct edid *' > parameter and wanting to pass that along to drm_add_display_info. > > v2: after review by Daniel Vetter <daniel.vetter@ffwll.ch> > > Removed EXPORT_SYMBOL_GPL for drm_reset_display_info and > drm_add_display_info. > > Added FIXME in drm_mode_connector_update_edid_property about > potentially merging that with drm_add_edid_modes to avoid > the need for two driver calls. > > Signed-off-by: Keith Packard <keithp@keithp.com> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Ok there's a functional conflict when I tried to apply this to -fixes, due to some rework already in drm-misc-next. Hence I applied your original patch here to drm-misc-next, and then cherry-picked it over to drm-misc-fixes and fixed it up. That way we'll not loose the bits needed in -next. Please double-check I didn't wreck stuff before I send the pull request out. Thanks, Daniel > --- > drivers/gpu/drm/drm_connector.c | 13 ++++++++++ > drivers/gpu/drm/drm_edid.c | 53 ++++++++++++++++++++++++++++++----------- > include/drm/drm_edid.h | 2 ++ > 3 files changed, 54 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 25f4b2e9a44f..83c01f359d55 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1207,6 +1207,19 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector, > if (edid) > size = EDID_LENGTH * (1 + edid->extensions); > > + /* Set the display info, using edid if available, otherwise > + * reseting the values to defaults. This duplicates the work > + * done in drm_add_edid_modes, but that function is not > + * consistently called before this one in all drivers and the > + * computation is cheap enough that it seems better to > + * duplicate it rather than attempt to ensure some arbitrary > + * ordering of calls. > + */ > + if (edid) > + drm_add_display_info(connector, edid); > + else > + drm_reset_display_info(connector); > + > drm_object_property_set_value(&connector->base, > dev->mode_config.non_desktop_property, > connector->display_info.non_desktop); > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 524eace3d460..365901c1c33c 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -1731,7 +1731,7 @@ EXPORT_SYMBOL(drm_edid_duplicate); > * > * Returns true if @vendor is in @edid, false otherwise > */ > -static bool edid_vendor(struct edid *edid, const char *vendor) > +static bool edid_vendor(const struct edid *edid, const char *vendor) > { > char edid_vendor[3]; > > @@ -1749,7 +1749,7 @@ static bool edid_vendor(struct edid *edid, const char *vendor) > * > * This tells subsequent routines what fixes they need to apply. > */ > -static u32 edid_get_quirks(struct edid *edid) > +static u32 edid_get_quirks(const struct edid *edid) > { > const struct edid_quirk *quirk; > int i; > @@ -2813,7 +2813,7 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid, > /* > * Search EDID for CEA extension block. > */ > -static u8 *drm_find_edid_extension(struct edid *edid, int ext_id) > +static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id) > { > u8 *edid_ext = NULL; > int i; > @@ -2835,12 +2835,12 @@ static u8 *drm_find_edid_extension(struct edid *edid, int ext_id) > return edid_ext; > } > > -static u8 *drm_find_cea_extension(struct edid *edid) > +static u8 *drm_find_cea_extension(const struct edid *edid) > { > return drm_find_edid_extension(edid, CEA_EXT); > } > > -static u8 *drm_find_displayid_extension(struct edid *edid) > +static u8 *drm_find_displayid_extension(const struct edid *edid) > { > return drm_find_edid_extension(edid, DISPLAYID_EXT); > } > @@ -4378,7 +4378,7 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db) > } > > static void drm_parse_cea_ext(struct drm_connector *connector, > - struct edid *edid) > + const struct edid *edid) > { > struct drm_display_info *info = &connector->display_info; > const u8 *edid_ext; > @@ -4412,11 +4412,34 @@ static void drm_parse_cea_ext(struct drm_connector *connector, > } > } > > -static void drm_add_display_info(struct drm_connector *connector, > - struct edid *edid, u32 quirks) > +/* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset > + * all of the values which would have been set from EDID > + */ > +void > +drm_reset_display_info(struct drm_connector *connector) > +{ > + struct drm_display_info *info = &connector->display_info; > + > + info->width_mm = 0; > + info->height_mm = 0; > + > + info->bpc = 0; > + info->color_formats = 0; > + info->cea_rev = 0; > + info->max_tmds_clock = 0; > + info->dvi_dual = false; > + info->has_hdmi_infoframe = false; > + > + info->non_desktop = 0; > +} > +EXPORT_SYMBOL_GPL(drm_reset_display_info); > + > +u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid) > { > struct drm_display_info *info = &connector->display_info; > > + u32 quirks = edid_get_quirks(edid); > + > info->width_mm = edid->width_cm * 10; > info->height_mm = edid->height_cm * 10; > > @@ -4430,11 +4453,13 @@ static void drm_add_display_info(struct drm_connector *connector, > > info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP); > > + DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop); > + > if (edid->revision < 3) > - return; > + return quirks; > > if (!(edid->input & DRM_EDID_INPUT_DIGITAL)) > - return; > + return quirks; > > drm_parse_cea_ext(connector, edid); > > @@ -4454,7 +4479,7 @@ static void drm_add_display_info(struct drm_connector *connector, > > /* Only defined for 1.4 with digital displays */ > if (edid->revision < 4) > - return; > + return quirks; > > switch (edid->input & DRM_EDID_DIGITAL_DEPTH_MASK) { > case DRM_EDID_DIGITAL_DEPTH_6: > @@ -4489,7 +4514,9 @@ static void drm_add_display_info(struct drm_connector *connector, > info->color_formats |= DRM_COLOR_FORMAT_YCRCB444; > if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422) > info->color_formats |= DRM_COLOR_FORMAT_YCRCB422; > + return quirks; > } > +EXPORT_SYMBOL_GPL(drm_add_display_info); > > static int validate_displayid(u8 *displayid, int length, int idx) > { > @@ -4645,8 +4672,6 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) > return 0; > } > > - quirks = edid_get_quirks(edid); > - > drm_edid_to_eld(connector, edid); > > /* > @@ -4654,7 +4679,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) > * To avoid multiple parsing of same block, lets parse that map > * from sink info, before parsing CEA modes. > */ > - drm_add_display_info(connector, edid, quirks); > + quirks = drm_add_display_info(connector, edid); > > /* > * EDID spec says modes should be preferred in this order: > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > index b25d12ef120a..8d89a9c3748d 100644 > --- a/include/drm/drm_edid.h > +++ b/include/drm/drm_edid.h > @@ -465,6 +465,8 @@ struct edid *drm_get_edid(struct drm_connector *connector, > struct edid *drm_get_edid_switcheroo(struct drm_connector *connector, > struct i2c_adapter *adapter); > struct edid *drm_edid_duplicate(const struct edid *edid); > +void drm_reset_display_info(struct drm_connector *connector); > +u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid); > int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid); > > u8 drm_match_cea_mode(const struct drm_display_mode *to_match); > -- > 2.15.0.rc0 >
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 25f4b2e9a44f..83c01f359d55 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1207,6 +1207,19 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector, if (edid) size = EDID_LENGTH * (1 + edid->extensions); + /* Set the display info, using edid if available, otherwise + * reseting the values to defaults. This duplicates the work + * done in drm_add_edid_modes, but that function is not + * consistently called before this one in all drivers and the + * computation is cheap enough that it seems better to + * duplicate it rather than attempt to ensure some arbitrary + * ordering of calls. + */ + if (edid) + drm_add_display_info(connector, edid); + else + drm_reset_display_info(connector); + drm_object_property_set_value(&connector->base, dev->mode_config.non_desktop_property, connector->display_info.non_desktop); diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 524eace3d460..365901c1c33c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1731,7 +1731,7 @@ EXPORT_SYMBOL(drm_edid_duplicate); * * Returns true if @vendor is in @edid, false otherwise */ -static bool edid_vendor(struct edid *edid, const char *vendor) +static bool edid_vendor(const struct edid *edid, const char *vendor) { char edid_vendor[3]; @@ -1749,7 +1749,7 @@ static bool edid_vendor(struct edid *edid, const char *vendor) * * This tells subsequent routines what fixes they need to apply. */ -static u32 edid_get_quirks(struct edid *edid) +static u32 edid_get_quirks(const struct edid *edid) { const struct edid_quirk *quirk; int i; @@ -2813,7 +2813,7 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid, /* * Search EDID for CEA extension block. */ -static u8 *drm_find_edid_extension(struct edid *edid, int ext_id) +static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id) { u8 *edid_ext = NULL; int i; @@ -2835,12 +2835,12 @@ static u8 *drm_find_edid_extension(struct edid *edid, int ext_id) return edid_ext; } -static u8 *drm_find_cea_extension(struct edid *edid) +static u8 *drm_find_cea_extension(const struct edid *edid) { return drm_find_edid_extension(edid, CEA_EXT); } -static u8 *drm_find_displayid_extension(struct edid *edid) +static u8 *drm_find_displayid_extension(const struct edid *edid) { return drm_find_edid_extension(edid, DISPLAYID_EXT); } @@ -4378,7 +4378,7 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db) } static void drm_parse_cea_ext(struct drm_connector *connector, - struct edid *edid) + const struct edid *edid) { struct drm_display_info *info = &connector->display_info; const u8 *edid_ext; @@ -4412,11 +4412,34 @@ static void drm_parse_cea_ext(struct drm_connector *connector, } } -static void drm_add_display_info(struct drm_connector *connector, - struct edid *edid, u32 quirks) +/* A connector has no EDID information, so we've got no EDID to compute quirks from. Reset + * all of the values which would have been set from EDID + */ +void +drm_reset_display_info(struct drm_connector *connector) +{ + struct drm_display_info *info = &connector->display_info; + + info->width_mm = 0; + info->height_mm = 0; + + info->bpc = 0; + info->color_formats = 0; + info->cea_rev = 0; + info->max_tmds_clock = 0; + info->dvi_dual = false; + info->has_hdmi_infoframe = false; + + info->non_desktop = 0; +} +EXPORT_SYMBOL_GPL(drm_reset_display_info); + +u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid) { struct drm_display_info *info = &connector->display_info; + u32 quirks = edid_get_quirks(edid); + info->width_mm = edid->width_cm * 10; info->height_mm = edid->height_cm * 10; @@ -4430,11 +4453,13 @@ static void drm_add_display_info(struct drm_connector *connector, info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP); + DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop); + if (edid->revision < 3) - return; + return quirks; if (!(edid->input & DRM_EDID_INPUT_DIGITAL)) - return; + return quirks; drm_parse_cea_ext(connector, edid); @@ -4454,7 +4479,7 @@ static void drm_add_display_info(struct drm_connector *connector, /* Only defined for 1.4 with digital displays */ if (edid->revision < 4) - return; + return quirks; switch (edid->input & DRM_EDID_DIGITAL_DEPTH_MASK) { case DRM_EDID_DIGITAL_DEPTH_6: @@ -4489,7 +4514,9 @@ static void drm_add_display_info(struct drm_connector *connector, info->color_formats |= DRM_COLOR_FORMAT_YCRCB444; if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422) info->color_formats |= DRM_COLOR_FORMAT_YCRCB422; + return quirks; } +EXPORT_SYMBOL_GPL(drm_add_display_info); static int validate_displayid(u8 *displayid, int length, int idx) { @@ -4645,8 +4672,6 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) return 0; } - quirks = edid_get_quirks(edid); - drm_edid_to_eld(connector, edid); /* @@ -4654,7 +4679,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) * To avoid multiple parsing of same block, lets parse that map * from sink info, before parsing CEA modes. */ - drm_add_display_info(connector, edid, quirks); + quirks = drm_add_display_info(connector, edid); /* * EDID spec says modes should be preferred in this order: diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index b25d12ef120a..8d89a9c3748d 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -465,6 +465,8 @@ struct edid *drm_get_edid(struct drm_connector *connector, struct edid *drm_get_edid_switcheroo(struct drm_connector *connector, struct i2c_adapter *adapter); struct edid *drm_edid_duplicate(const struct edid *edid); +void drm_reset_display_info(struct drm_connector *connector); +u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid); int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid); u8 drm_match_cea_mode(const struct drm_display_mode *to_match);