| Message ID | 20171208004257.3625-1-keithp@keithp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 07, 2017 at 04:42:57PM -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. > > Signed-off-by: Keith Packard <keithp@keithp.com> Yeah looks like a good way to reset the desktop-mode in all cases. > --- > 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. > + */ Looks like the most reasonable fix for -rc. For the future, maybe capture a FIXME/TODO in this comment that we should look into merging drm_add_edid_modes() and drm_mode_connector_update_edid_property(). I really can't think of a good reason for that split. > + 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); Do we really need these 2 EXPORT_SYMBOL here? From a quick look the users are only internal to drm.ko, so not needed. And that would gives us a clearer driver api (plus I won't nag you about lack of kerneldoc for said driver api). If that's correct pls also move the decls to drm_crtc_internal.h. There's already another function exported like that from drm_edid.c. With the nits addressed: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Please also cc: intel-gfx for v2 so our CI can double-check I didn't miss anything. Dave's out, but there's some other drm-misc fixes, I'll probably forward a -fixes pull to Linus directly. Thanks, Daniel > + > +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);
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. Signed-off-by: Keith Packard <keithp@keithp.com> --- 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(-)