[PATCH 1/4] drm/probe-helper: Add drm_connector_helper_get_modes_static()
Thomas Zimmermann
tzimmermann at suse.de
Thu Aug 11 18:22:22 UTC 2022
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 at suse.de>
> Reviewed-by: Sam Ravnborg <sam at ravnborg.org>
>
> 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 connector
> The 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's
> their?
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220811/322ead89/attachment-0001.sig>
More information about the dri-devel
mailing list