Hi,
On Thu, May 5, 2022 at 11:34 AM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 26.04.22 um 20:46 schrieb Douglas Anderson:
The drm_helper_probe_single_connector_modes() is a bit long. Let's break a chunk off to update and validate modes. This helps avoid one goto and also will allow us to more easily call the helper a second time in a future patch without adding looping or another goto.
This change is intended to be a no-op change--just code movement.
Signed-off-by: Douglas Anderson dianders@chromium.org
drivers/gpu/drm/drm_probe_helper.c | 105 ++++++++++++++++------------- 1 file changed, 59 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 682359512996..819225629010 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -354,6 +354,61 @@ drm_helper_probe_detect(struct drm_connector *connector, } EXPORT_SYMBOL(drm_helper_probe_detect);
+static bool _drm_helper_update_and_validate(struct drm_connector *connector,
AFAIK convention is to use two underscores for internal names.
Sure! I'll spin with this.
uint32_t maxX, uint32_t maxY,struct drm_modeset_acquire_ctx *ctx)+{
struct drm_device *dev = connector->dev;struct drm_display_mode *mode;int mode_flags = 0;int ret;drm_connector_list_update(connector);if (connector->interlace_allowed)mode_flags |= DRM_MODE_FLAG_INTERLACE;if (connector->doublescan_allowed)mode_flags |= DRM_MODE_FLAG_DBLSCAN;if (connector->stereo_allowed)mode_flags |= DRM_MODE_FLAG_3D_MASK;list_for_each_entry(mode, &connector->modes, head) {if (mode->status != MODE_OK)continue;mode->status = drm_mode_validate_driver(dev, mode);if (mode->status != MODE_OK)continue;mode->status = drm_mode_validate_size(mode, maxX, maxY);if (mode->status != MODE_OK)continue;mode->status = drm_mode_validate_flag(mode, mode_flags);if (mode->status != MODE_OK)continue;ret = drm_mode_validate_pipeline(mode, connector, ctx,&mode->status);if (ret) {drm_dbg_kms(dev,"drm_mode_validate_pipeline failed: %d\n",ret);if (drm_WARN_ON_ONCE(dev, ret != -EDEADLK))mode->status = MODE_ERROR;elsereturn true;Returning true is non-intuitive. It looks as if we report success when it actually signals a retry.
I suggest to return 'ret' here and let the caller decide. On success at the end of the function, it would return 0 as usual.
There's a madness to my method. I originally had it returning "ret" but then I felt like the callers now needed to handle three cases:
a) ret = -EDEADLK b) ret = 0 c) ret = some other error
In reality _drm_helper_update_and_validate() never returned anything other than a) or b), so adding the extra error handling for c) seemed like a waste. ...but it also felt like if it violated the abstraction of _drm_helper_update_and_validate() returning an error code if I didn't handle c).
In any case, I'll change it back to an error code. Maybe a compromise would be:
ret = _drm_helper_update_and_validate(...) if (ret == -EDEADLK) { drm_modeset_backoff(...) goto retry; } WARN_ON(ret);
...so we at least document that ret can only be one of the two values and we'll get a warning splat if it ever happens, but we don't need to add complex error handling for a case that the code can't hit. Ah, looking above I guess that matches what the function does earlier too.
OK, I'll give a few more days for feedback on this series and I'll spin with the two changes you've requested.
-Doug
-Doug