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 <[email protected]>
---
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,
+ 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;
+ else
+ return true;
+ }
+
+ if (mode->status != MODE_OK)
+ continue;
+ mode->status = drm_mode_validate_ycbcr420(mode, connector);
+ }
+
+ return false;
+}
+
/**
* drm_helper_probe_single_connector_modes - get complete set of display modes
* @connector: connector to probe
@@ -421,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
const struct drm_connector_helper_funcs *connector_funcs =
connector->helper_private;
int count = 0, ret;
- int mode_flags = 0;
bool verbose_prune = true;
enum drm_connector_status old_status;
struct drm_modeset_acquire_ctx ctx;
@@ -519,52 +573,11 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
connector->status == connector_status_unknown))
count = drm_add_modes_noedid(connector, 1024, 768);
count += drm_helper_probe_add_cmdline_mode(connector);
- if (count == 0)
- goto prune;
-
- 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;
- } else {
- drm_modeset_backoff(&ctx);
- goto retry;
- }
+ if (count != 0) {
+ if (_drm_helper_update_and_validate(connector, maxX, maxY, &ctx)) {
+ drm_modeset_backoff(&ctx);
+ goto retry;
}
-
- if (mode->status != MODE_OK)
- continue;
- mode->status = drm_mode_validate_ycbcr420(mode, connector);
}
prune:
--
2.36.0.rc2.479.g8af0fa9b8e-goog
On 4/26/2022 11:46 AM, Douglas Anderson wrote:
> 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 <[email protected]>
I think this cleanup looks reasonable to me.
Hence,
Reviewed-by: Abhinav Kumar <[email protected]>
> ---
>
> 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,
> + 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;
> + else
> + return true;
> + }
> +
> + if (mode->status != MODE_OK)
> + continue;
> + mode->status = drm_mode_validate_ycbcr420(mode, connector);
> + }
> +
> + return false;
> +}
> +
> /**
> * drm_helper_probe_single_connector_modes - get complete set of display modes
> * @connector: connector to probe
> @@ -421,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> const struct drm_connector_helper_funcs *connector_funcs =
> connector->helper_private;
> int count = 0, ret;
> - int mode_flags = 0;
> bool verbose_prune = true;
> enum drm_connector_status old_status;
> struct drm_modeset_acquire_ctx ctx;
> @@ -519,52 +573,11 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> connector->status == connector_status_unknown))
> count = drm_add_modes_noedid(connector, 1024, 768);
> count += drm_helper_probe_add_cmdline_mode(connector);
> - if (count == 0)
> - goto prune;
> -
> - 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;
> - } else {
> - drm_modeset_backoff(&ctx);
> - goto retry;
> - }
> + if (count != 0) {
> + if (_drm_helper_update_and_validate(connector, maxX, maxY, &ctx)) {
> + drm_modeset_backoff(&ctx);
> + goto retry;
> }
> -
> - if (mode->status != MODE_OK)
> - continue;
> - mode->status = drm_mode_validate_ycbcr420(mode, connector);
> }
>
> prune:
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 <[email protected]>
> ---
>
> 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.
> + 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;
> + else
> + return 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.
Best regards
Thomas
> + }
> +
> + if (mode->status != MODE_OK)
> + continue;
> + mode->status = drm_mode_validate_ycbcr420(mode, connector);
> + }
> +
> + return false;
> +}
> +
> /**
> * drm_helper_probe_single_connector_modes - get complete set of display modes
> * @connector: connector to probe
> @@ -421,7 +476,6 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> const struct drm_connector_helper_funcs *connector_funcs =
> connector->helper_private;
> int count = 0, ret;
> - int mode_flags = 0;
> bool verbose_prune = true;
> enum drm_connector_status old_status;
> struct drm_modeset_acquire_ctx ctx;
> @@ -519,52 +573,11 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> connector->status == connector_status_unknown))
> count = drm_add_modes_noedid(connector, 1024, 768);
> count += drm_helper_probe_add_cmdline_mode(connector);
> - if (count == 0)
> - goto prune;
> -
> - 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;
> - } else {
> - drm_modeset_backoff(&ctx);
> - goto retry;
> - }
> + if (count != 0) {
> + if (_drm_helper_update_and_validate(connector, maxX, maxY, &ctx)) {
> + drm_modeset_backoff(&ctx);
> + goto retry;
> }
> -
> - if (mode->status != MODE_OK)
> - continue;
> - mode->status = drm_mode_validate_ycbcr420(mode, connector);
> }
>
> prune:
--
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
Hi,
On Thu, May 5, 2022 at 11:34 AM Thomas Zimmermann <[email protected]> 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 <[email protected]>
>
>
> > ---
> >
> > 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;
> > + else
> > + return 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