Thanks for the review Ville, please see inline: > -----Original Message----- > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Sent: 26 November 2019 07:15 > To: Devarsh Thakkar <DEVARSHT@xxxxxxxxxx> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Hyun Kwon <hyunk@xxxxxxxxxx>; vcu- > team <vcu-team@xxxxxxxxxx>; Ranganathan Sk <rsk@xxxxxxxxxx>; Dhaval > Rajeshbhai Shah <dshah@xxxxxxxxxx>; Satish Kumar Nagireddy > <SATISHNA@xxxxxxxxxx>; Varunkumar Allagadapa <VARUNKUM@xxxxxxxxxx>; > Rohit Visavalia <RVISAVAL@xxxxxxxxxx> > Subject: Re: [PATCH libdrm] modetest: Use floating vrefresh while dumping > mode > > EXTERNAL EMAIL > > On Tue, Nov 26, 2019 at 07:03:58AM -0800, Devarsh Thakkar wrote: > > Add inline function to derive floating value of vertical refresh rate > > from pixel clock, horizontal total size and vertical total size. > > > > Use this function to find suitable mode having vrefresh value which is > > matching with user provided vrefresh value. > > > > If user doesn't provide any vrefresh value in args then update > > vertical refresh rate value in pipe args using this function so that > > it will be used for printing floating vrefresh while dumping mode. > > > > This will give more accurate picture to user for available modes > > differentiated by floating vertical refresh rate and help user select > > more appropriate mode using suitable refresh rate value. > > > > Signed-off-by: Devarsh Thakkar <devarsh.thakkar@xxxxxxxxxx> > > --- > > tests/modetest/modetest.c | 36 ++++++++++++++++++++++-------------- > > 1 file changed, 22 insertions(+), 14 deletions(-) > > > > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c > > index b4edfcb..80cf076 100644 > > --- a/tests/modetest/modetest.c > > +++ b/tests/modetest/modetest.c > > @@ -133,6 +133,12 @@ static inline int64_t U642I64(uint64_t val) > > return (int64_t)*((int64_t *)&val); } > > > > +static inline float get_floating_vrefresh(drmModeModeInfo *mode) > > Drop the 'inline'. I'd call it just mode_vrefresh() or somehting like that. > Yes I do agree that most compilers will substitute function body if needed (and I do see that in generated assembly even without inline) but I still thought better to add inline keyword just to give the hint to compiler in case if it doesn't. Any specific reason or issues if we use inline ? > > +{ > > + return mode->clock * 1000.00 > > + / (mode->htotal * mode->vtotal); } > > + > > #define bit_name_fn(res) \ > > const char * res##_str(int type) { \ > > unsigned int i; \ > > @@ -210,9 +216,9 @@ static void dump_encoders(struct device *dev) > > > > static void dump_mode(drmModeModeInfo *mode) { > > - printf(" %s %d %d %d %d %d %d %d %d %d %d", > > + printf(" %s %.2f %d %d %d %d %d %d %d %d %d", > > mode->name, > > - mode->vrefresh, > > + get_floating_vrefresh(mode), > > mode->hdisplay, > > mode->hsync_start, > > mode->hsync_end, > > @@ -823,12 +829,11 @@ struct plane_arg { > > > > static drmModeModeInfo * > > connector_find_mode(struct device *dev, uint32_t con_id, const char > *mode_str, > > - const float vrefresh) > > + float *vrefresh) > > { > > drmModeConnector *connector; > > drmModeModeInfo *mode; > > int i; > > - float mode_vrefresh; > > > > connector = get_connector_by_id(dev, con_id); > > if (!connector || !connector->count_modes) @@ -837,16 +842,19 @@ > > connector_find_mode(struct device *dev, uint32_t con_id, const char > *mode_str, > > for (i = 0; i < connector->count_modes; i++) { > > mode = &connector->modes[i]; > > if (!strcmp(mode->name, mode_str)) { > > - /* If the vertical refresh frequency is not specified then return > the > > - * first mode that match with the name. Else, return the mode > that match > > - * the name and the specified vertical refresh frequency. > > + /* If the vertical refresh frequency is not specified > > + * then return the first mode that match with the name > > + * and update corresponding vrefresh in pipe_arg. > > + * Else, return the mode that match the name and > > + * the specified vertical refresh frequency. > > */ > > - mode_vrefresh = mode->clock * 1000.00 > > - / (mode->htotal * mode->vtotal); > > - if (vrefresh == 0) > > + if (*vrefresh == 0) { > > + *vrefresh = get_floating_vrefresh(mode); > > return mode; > > - else if (fabs(mode_vrefresh - vrefresh) < 0.005) > > + } else if (fabs(get_floating_vrefresh(mode) > > + - *vrefresh) < 0.005) { > > return mode; > > + } > > } > > } > > > > @@ -909,11 +917,11 @@ static int pipe_find_crtc_and_mode(struct > device > > *dev, struct pipe_arg *pipe) > > > > for (i = 0; i < (int)pipe->num_cons; i++) { > > mode = connector_find_mode(dev, pipe->con_ids[i], > > - pipe->mode_str, pipe->vrefresh); > > + pipe->mode_str, > > + &pipe->vrefresh); > > if (mode == NULL) { > > fprintf(stderr, > > - "failed to find mode \"%s\" for connector %s\n", > > - pipe->mode_str, pipe->cons[i]); > > + "failed to find mode \"%s-%.2fHz\" for connector %s\n", > > + pipe->mode_str, pipe->vrefresh, pipe->cons[i]); > > We didn't find any mode so having connector_find_mode() update vrefresh > for us is nonsense. Yes that's correct, but we need it in a scenario where user provided a vrefresh for the mode and mode is not available with that exact vrefresh but available with a different refresh rate. So for e.g if 1920x1080-60 is only available and user set 1920x1080-59.92 (which is not available). Then it should display that mode 1920x1080-59.92 is not available and not that 1920x1080 is not found (since 1920x1080-60 is available). So I guess best way to handle not found mode is to print vrefresh too only in scenario where user had provided a specific refresh rate as arg. What's your opinion ? > > > return -EINVAL; > > } > > } > > -- > > 2.7.4 > > -- > Ville Syrjälä > Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel