Hi Eric, Ville, Thanks for the review, please see inline. > -----Original Message----- > From: Eric Engestrom <eric.engestrom@xxxxxxxxx> > Sent: 05 December 2019 05:17 > To: Devarsh Thakkar <DEVARSHT@xxxxxxxxxx> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH libdrm v4] modetest: Use floating vrefresh while dumping > mode > > EXTERNAL EMAIL > > On Tuesday, 2019-12-03 06:37:36 -0800, Devarsh Thakkar wrote: > > Add function to derive floating value of vertical refresh rate from > > drm mode using 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. > > > > Also use this function for printing floating vrefresh while dumping > > all available modes. > > > > 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. > > Any chance you could use the merge requests infra instead of patches on the > mailing list? :) https://gitlab.freedesktop.org/mesa/drm/merge_requests > > This will allow much easier reviews of the patch's revisions and build-test your > changes automatically. Yes I can do, but I'd prefer not to since as per https://cgit.freedesktop.org/mesa/drm/tree/CONTRIBUTING.rst the process is to send the patches to dri-devel mailing list directly for the review, so I guess unless the guidelines are changed, it would be really confusing for the larger audience to review somewhere else since it contradicts the current contribution guidelines mentioned in the repo. > > Can I also suggest you split the unrelated changes into individual commits? The major part of functionality change has already been done in previous patch (i.e https://cgit.freedesktop.org/mesa/drm/commit/tests/modetest?id=a2d588fe12dd55b222bcd31d683bd17a41326df7), this patch is mainly to print floating value of vrefresh while dumping mode details, so all changes done here s are related in the sense that their goal is to print floating value of vrefresh and some abstraction done (new func that is described in the commit message) to achieve that. So maybe, I can modify the commit message if it doesn't make sense clearly, but code-wise it looks ok to me to have it in a single patch unless I am missing something. Kindly let me know your opinion. @Ville Syrjälä, The v4 patch also has updates (%s/float *vrefresh/ float vrefresh) that you suggested, Kindly let me know if it looks ok now, or any further changes are required. Regrards, Devarsh > > > > > V4: > > 1) While setting mode, print mode name and vrefresh using struct > > drmModeModeInfo instead of struct pipe_args. > > 2) Revert back to using a float value instead of float * > > for vrefresh arg in connector_find_mode(). > > > > V3: > > 1) Change name of function used to derive refresh rate. > > > > V2: > > 1) Don't use inline function for deriving refresh rate from mode. > > 2) If requested mode not found, print refresh rate only > > if user had provided it in args. > > > > Signed-off-by: Devarsh Thakkar <devarsh.thakkar@xxxxxxxxxx> > > Reviewed-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx> > > --- > > tests/modetest/modetest.c | 35 +++++++++++++++++++++++------------ > > 1 file changed, 23 insertions(+), 12 deletions(-) > > > > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c > > index b4edfcb..e998e8e 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 float mode_vrefresh(drmModeModeInfo *mode) { > > + 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, > > + mode_vrefresh(mode), > > mode->hdisplay, > > mode->hsync_start, > > mode->hsync_end, > > @@ -828,7 +834,6 @@ connector_find_mode(struct device *dev, uint32_t > con_id, const char *mode_str, > > drmModeConnector *connector; > > drmModeModeInfo *mode; > > int i; > > - float mode_vrefresh; > > > > connector = get_connector_by_id(dev, con_id); > > if (!connector || !connector->count_modes) @@ -837,15 +842,14 @@ > > 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. > > + * 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) > > return mode; > > - else if (fabs(mode_vrefresh - vrefresh) < 0.005) > > + else if (fabs(mode_vrefresh(mode) - vrefresh) < > > + 0.005) > > return mode; > > } > > } > > @@ -911,7 +915,13 @@ static int pipe_find_crtc_and_mode(struct device > *dev, struct pipe_arg *pipe) > > mode = connector_find_mode(dev, pipe->con_ids[i], > > pipe->mode_str, pipe->vrefresh); > > if (mode == NULL) { > > - fprintf(stderr, > > + if (pipe->vrefresh) > > + fprintf(stderr, > > + "failed to find mode " > > + "\"%s-%.2fHz\" for connector %s\n", > > + pipe->mode_str, pipe->vrefresh, pipe->cons[i]); > > + else > > + fprintf(stderr, > > "failed to find mode \"%s\" for connector %s\n", > > pipe->mode_str, pipe->cons[i]); > > return -EINVAL; > > @@ -1398,7 +1408,7 @@ static void atomic_set_mode(struct device *dev, > struct pipe_arg *pipes, unsigned > > continue; > > > > printf("setting mode %s-%.2fHz on connectors ", > > - pipe->mode_str, pipe->vrefresh); > > + pipe->mode->name, mode_vrefresh(pipe->mode)); > > for (j = 0; j < pipe->num_cons; ++j) { > > printf("%s, ", pipe->cons[j]); > > add_property(dev, pipe->con_ids[j], "CRTC_ID", > > pipe->crtc->crtc->crtc_id); @@ -1481,7 +1491,8 @@ static void > set_mode(struct device *dev, struct pipe_arg *pipes, unsigned int co > > continue; > > > > printf("setting mode %s-%.2fHz@%s on connectors ", > > - pipe->mode_str, pipe->vrefresh, pipe->format_str); > > + pipe->mode->name, mode_vrefresh(pipe->mode), > > + pipe->format_str); > > for (j = 0; j < pipe->num_cons; ++j) > > printf("%s, ", pipe->cons[j]); > > printf("crtc %d\n", pipe->crtc->crtc->crtc_id); > > -- > > 2.7.4 > > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
