Hi Sam, thanks for the reviews. Am 10.08.22 um 21:21 schrieb Sam Ravnborg:
Hi Thomas, On Wed, Aug 10, 2022 at 01:20:50PM +0200, Thomas Zimmermann wrote:Add drm_connector_helper_get_modes_static(), which duplicates a single display mode for a connector. Convert drivers.I like this helper! There are a lot of panels that can benefit from the same helper. The current users that are replaced do not do so, but some panels also set: connector->display_info.bpc = 8; connector->display_info.bus_flags = DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE; drm_display_info_set_bus_formats(&connector->display_info, &bus_format, 1); I looked at a similar helper for panels once, but for panels I stopped there as we then had to pass bpc, bus_format and bus_mode as arguments. Maybe that is over-engineering things. Someone that knows when we must pass bpc, bus_mode, bus_flags and when not can maybe help here. The current helper is fine as is, but I wonder if we can cover more use cases with an extra helper. It would also be nice to convert the panels that can use the new helper, but that should be in a new patch and can be done later.
Yeah, I saw other drivers that do something very similar to what the new helper does, but with a 'twist.' So I hesitated to change them. Maybe some rearchitecturing of the drivers or helpers can change that.
Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx> but I have a few comments in the following.--- drivers/gpu/drm/drm_mipi_dbi.c | 20 +-------------- drivers/gpu/drm/drm_probe_helper.c | 40 ++++++++++++++++++++++++++++++ drivers/gpu/drm/tiny/repaper.c | 16 +----------- drivers/gpu/drm/tiny/simpledrm.c | 18 +------------- include/drm/drm_probe_helper.h | 3 +++ 5 files changed, 46 insertions(+), 51 deletions(-) diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c index 84abc3920b57..b67ec9a5cda9 100644 --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -415,26 +415,8 @@ EXPORT_SYMBOL(mipi_dbi_pipe_disable); static int mipi_dbi_connector_get_modes(struct drm_connector *connector) { struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(connector->dev); - struct drm_display_mode *mode;- mode = drm_mode_duplicate(connector->dev, &dbidev->mode);- if (!mode) { - DRM_ERROR("Failed to duplicate mode\n"); - return 0; - } - - if (mode->name[0] == '\0') - drm_mode_set_name(mode); - - mode->type |= DRM_MODE_TYPE_PREFERRED; - drm_mode_probed_add(connector, mode); - - if (mode->width_mm) { - connector->display_info.width_mm = mode->width_mm; - connector->display_info.height_mm = mode->height_mm; - } - - return 1; + return drm_connector_helper_get_modes_static(connector, &dbidev->mode); }static const struct drm_connector_helper_funcs mipi_dbi_connector_hfuncs = {diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index bb427c5a4f1f..809187377e4e 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -1050,6 +1050,46 @@ int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector) } EXPORT_SYMBOL(drm_connector_helper_get_modes_from_ddc);+/**+ * drm_connector_helper_get_modes_static - Duplicates a display mode for a connectorThe hw mode is duplicated so maybe name it ..._modes_hw()? But I dunno.. Naming is hard.
I considered 'hw', 'static' and 'fixed', all of wich I didn't really like. I'll change all names to 'fixed'. It seems closest to what the helper is for (think of 'fixed resolution') and also close to DRM's common terminology.
+ * @connector: the connector + * @hw_mode: the display hardware's mode + * + * This function duplicates a display modes for a connector. Drivers for hardware + * that only supports a single mode can use this function in there connector'stheir?
Oh, indeed.
+ * get_modes helper. + * + * Returns: + * The number of created modes. + */ +int drm_connector_helper_get_modes_static(struct drm_connector *connector, + const struct drm_display_mode *hw_mode) +{ + struct drm_device *dev = connector->dev; + struct drm_display_mode *mode; + + mode = drm_mode_duplicate(dev, hw_mode); + if (!mode) { + drm_err(dev, "Failed to duplicate mode " DRM_MODE_FMT "\n", + DRM_MODE_ARG(hw_mode)); + return 0; + } + + if (mode->name[0] == '\0') + drm_mode_set_name(mode);Hmm, so we rely that it was set to something relevant before. I guess that's OK.
Name is an array within the structure. According to DRM's rules, it will be initialized to zero. If the caller set a name in hw_mode, we keep that instead, otherwise create a standard name with drm_mode_set_name(). That seems appropriate to me.
Best regards Thomas
+ + mode->type |= DRM_MODE_TYPE_PREFERRED; + drm_mode_probed_add(connector, mode); + + if (mode->width_mm) + connector->display_info.width_mm = mode->width_mm; + if (mode->height_mm) + connector->display_info.height_mm = mode->height_mm; + + return 1; +} +EXPORT_SYMBOL(drm_connector_helper_get_modes_static); + /** * drm_connector_helper_get_modes - Read EDID and update connector. * @connector: The connector diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c index c4c1be3ac0b8..855968fd46af 100644 --- a/drivers/gpu/drm/tiny/repaper.c +++ b/drivers/gpu/drm/tiny/repaper.c @@ -839,22 +839,8 @@ static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = { static int repaper_connector_get_modes(struct drm_connector *connector) { struct repaper_epd *epd = drm_to_epd(connector->dev); - struct drm_display_mode *mode;- mode = drm_mode_duplicate(connector->dev, epd->mode);- if (!mode) { - DRM_ERROR("Failed to duplicate mode\n"); - return 0; - } - - drm_mode_set_name(mode); - mode->type |= DRM_MODE_TYPE_PREFERRED; - drm_mode_probed_add(connector, mode); - - connector->display_info.width_mm = mode->width_mm; - connector->display_info.height_mm = mode->height_mm; - - return 1; + return drm_connector_helper_get_modes_static(connector, epd->mode); }static const struct drm_connector_helper_funcs repaper_connector_hfuncs = {diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c index a81f91814595..2d5b56c4a77d 100644 --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -620,24 +620,8 @@ static const struct drm_encoder_funcs simpledrm_encoder_funcs = { static int simpledrm_connector_helper_get_modes(struct drm_connector *connector) { struct simpledrm_device *sdev = simpledrm_device_of_dev(connector->dev); - struct drm_display_mode *mode;- mode = drm_mode_duplicate(connector->dev, &sdev->mode);- if (!mode) - return 0; - - if (mode->name[0] == '\0') - drm_mode_set_name(mode); - - mode->type |= DRM_MODE_TYPE_PREFERRED; - drm_mode_probed_add(connector, mode); - - if (mode->width_mm) - connector->display_info.width_mm = mode->width_mm; - if (mode->height_mm) - connector->display_info.height_mm = mode->height_mm; - - return 1; + return drm_connector_helper_get_modes_static(connector, &sdev->mode); }static const struct drm_connector_helper_funcs simpledrm_connector_helper_funcs = {diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h index 8075e02aa865..5a883ee9fc32 100644 --- a/include/drm/drm_probe_helper.h +++ b/include/drm/drm_probe_helper.h @@ -7,6 +7,7 @@struct drm_connector;struct drm_device; +struct drm_display_mode; struct drm_modeset_acquire_ctx;int drm_helper_probe_single_connector_modes(struct drm_connector@@ -27,6 +28,8 @@ void drm_kms_helper_poll_enable(struct drm_device *dev); bool drm_kms_helper_is_poll_worker(void);int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector);+int drm_connector_helper_get_modes_static(struct drm_connector *connector, + const struct drm_display_mode *hw_mode); int drm_connector_helper_get_modes(struct drm_connector *connector);#endif-- 2.37.1
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
