Re: [PATCH 1/2] drm/probe-helper: Add helper for drm_helper_probe_single_connector_modes()

[Doug Anderson]

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