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

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@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@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