[PATCH libdrm 2/2] modetest: Add a new "-r" option to set a default mode

[Emil Velikov]

Hi Ezequiel,

The first patch looks spot on. I'll commit it in a moment.

On Mon, 22 Jul 2019 at 17:08, Ezequiel Garcia <ezequiel at collabora.com> wrote:
>
> This option finds the first connected connector and then
> sets its preferred mode on it.
>
> Set this option to be set when no mode or plane is set
> explicitily. This allows to quickly test, in cases where
> one just needs something displayed.
>
> Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
> ---
>  tests/modetest/modetest.c | 81 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 75 insertions(+), 6 deletions(-)
>
> diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> index 5e628127a130..6042aaae7cca 100644
> --- a/tests/modetest/modetest.c
> +++ b/tests/modetest/modetest.c

> @@ -1874,6 +1878,9 @@ static int pipe_resolve_connectors(struct device *dev, struct pipe_arg *pipe)
>         char *endp;
>
>         for (i = 0; i < pipe->num_cons; i++) {
> +               /* If set_preferred is used, the connector is already resolved. */
> +               if (pipe->con_ids[i])
> +                       continue;
This seems rather non-intuitive. How about we guard invocation of this
function altogether?
For example (minimal whitespace changes), in main() we can use:

if (!set_preferred && pipe_resolve_connectors()....)


> +static int pipe_find_preferred(struct device *dev, struct pipe_arg *pipe)
> +{
> +       drmModeRes *res = dev->resources->res;
> +       drmModeConnector *con = NULL;
> +       char *con_str;
> +       int i;
> +
> +       for (i = 0; i < res->count_connectors; i++) {
> +               con = drmModeGetConnector(dev->fd, res->connectors[i]);
> +               if (con->connection == DRM_MODE_CONNECTED)
> +                       break;
> +               drmModeFreeConnector(con);
> +               con = NULL;
> +       }
> +

> +
> +       /* A CRTC possible will be chosen by pipe_find_crtc_and_mode. */
> +       pipe->crtc_id = (uint32_t)-1;
> +
As-is this will pick the first connector, which may not work with all crtcs.

How about we tweak the loop above to pick a connector for the given crtc_id?
Feel free to do that as follow-up.


>  int main(int argc, char **argv)

>         int test_cursor = 0;
> +       int set_preferred = 0;
>         int use_atomic = 0;
>         char *device = NULL;
>         char *module = NULL;
> @@ -1987,6 +2043,9 @@ int main(int argc, char **argv)
>                 case 'v':
>                         test_vsync = 1;
>                         break;
> +               case 'r':
> +                       set_preferred = 1;
> +                       break;
>                 case 'w':
>                         prop_args = realloc(prop_args,
>                                            (prop_count + 1) * sizeof *prop_args);
> @@ -2008,7 +2067,7 @@ int main(int argc, char **argv)
>         }
>
>         if (!args || (args == 1 && use_atomic))
> -               encoders = connectors = crtcs = planes = framebuffers = 1;
> +               set_preferred = encoders = connectors = crtcs = planes = framebuffers = 1;
>
Did you mean to git add this? The variable set_preferred is already
set as needed.

Any reason why using atomic modeset (modetest -r -a), clears the
screen while legacy (modetest -r) sets the usual pattern. What am I
missing?

Thanks
Emil