Hi,
On Thu, May 5, 2022 at 11:34 AM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxxxxx>
>
>
> > ---
> >
> > 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
