Hi Ville,
Thanks for the review. I have sent v3 patch addressing the review comments.
Could you please review and let me know if any further changes required ?
Regards,
Devarsh
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Sent: 07 November 2019 03:13
> To: Devarsh Thakkar <DEVARSHT@xxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Ranganathan Sk <rsk@xxxxxxxxxx>; vcu-
> team <vcu-team@xxxxxxxxxx>; Dhaval Rajeshbhai Shah <dshah@xxxxxxxxxx>;
> Varunkumar Allagadapa <VARUNKUM@xxxxxxxxxx>; Satish Kumar Nagireddy
> <SATISHNA@xxxxxxxxxx>
> Subject: Re: [PATCH libdrm v2] modetest: Add support for setting mode having
> floating vertical refresh rate
>
> EXTERNAL EMAIL
>
> On Wed, Nov 06, 2019 at 07:35:36AM -0800, Devarsh Thakkar wrote:
> > For the scenario where user may require to modeset with a mode
> > supporting a fractional value for vertical refresh-rate, appropriate
> > mode can be selected by searching for mode having matching fractional
> > vertical refresh rate using below equation.
> >
> > vrefresh = (1000 * pixel clock) / (htotal * vtotal) Hz.
> >
> > We do this way since driver doesn't return float value of vrefresh as
> > it use int for vrefresh in struct drm_mode_info, but we can derive the
> > actual value using pixel clock, horizontal total size and vertical
> > total size values.
> >
> > So for e.g. if user want to select mode having 59.94 Hz as refresh
> > rate then with this patch it be can done as shown in below command,
> > given there is an appropriate mode is available :
> >
> > modetest -M xlnx -s 39:1920x1080-59.94@BG24 -v
> >
> > NOTE: Above command was tested on xilinx DRM driver with DP monitor
> > which was supporting mode having 59.94 Hz refresh rate.
> >
> > Signed-off-by: Devarsh Thakkar <devarsh.thakkar@xxxxxxxxxx>
> > ---
> > V2: Update commit message
> > ---
> > tests/modetest/modetest.c | 32 ++++++++++++++++++++++++--------
> > 1 file changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > index e66be66..9b3e410 100644
> > --- a/tests/modetest/modetest.c
> > +++ b/tests/modetest/modetest.c
> > @@ -795,7 +795,7 @@ struct pipe_arg {
> > uint32_t crtc_id;
> > char mode_str[64];
> > char format_str[5];
> > - unsigned int vrefresh;
> > + float vrefresh;
> > unsigned int fourcc;
> > drmModeModeInfo *mode;
> > struct crtc *crtc;
> > @@ -822,11 +822,12 @@ struct plane_arg {
> >
> > static drmModeModeInfo *
> > connector_find_mode(struct device *dev, uint32_t con_id, const char
> *mode_str,
> > - const unsigned int vrefresh)
> > + const float vrefresh)
> > {
> > drmModeConnector *connector;
> > drmModeModeInfo *mode;
> > int i;
> > + float mode_vrefresh;
> >
> > connector = get_connector_by_id(dev, con_id);
> > if (!connector || !connector->count_modes) @@ -839,9 +840,19
> > @@ connector_find_mode(struct device *dev, uint32_t con_id, const char
> *mode_str,
> > * first mode that match with the name. Else, return the mode
> that match
> > * the name and the specified vertical refresh frequency.
> > */
> > + float temp;
> > +
> > + mode_vrefresh = ((float)(mode->clock) * 1000.00)
> > + / ((float)(mode->htotal)
> > + * (float)mode->vtotal);
>
> 1000.00 is a double, ditto for all the 0.5 etc. later.
>
> All the casts are pointless here. Lost of unnecessary parens too.
>
> > + /* Round off to 2 decimal places to match with user
> > + * provided value
> > + */
> > + temp = (int) (mode_vrefresh * 100 + 0.5);
> > + mode_vrefresh = (float) temp / 100;
>
> This *100/100 business feels a bit convoluted. How about just leave the floats
> alone and replace the == with a some epsilon check?
>
> Or if you want to do this rounding stuff I would at least put it into some function
> so it doesn't hurt one's eyes all the time.
> The timings->vrefresh calculation could also be a neat function.
>
> > if (vrefresh == 0)
> > return mode;
> > - else if (mode->vrefresh == vrefresh)
> > + else if (mode_vrefresh == vrefresh)
> > return mode;
> > }
> > }
> > @@ -1393,8 +1404,8 @@ static void atomic_set_mode(struct device *dev,
> struct pipe_arg *pipes, unsigned
> > if (pipe->mode == NULL)
> > continue;
> >
> > - printf("setting mode %s-%dHz on connectors ",
> > - pipe->mode_str, pipe->mode->vrefresh);
> > + printf("setting mode %s-%.2fHz on connectors ",
> > + pipe->mode_str, pipe->vrefresh);
> > 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); @@ -1476,8 +1487,8 @@ static void
> set_mode(struct device *dev, struct pipe_arg *pipes, unsigned int co
> > if (pipe->mode == NULL)
> > continue;
> >
> > - printf("setting mode %s-%dHz@%s on connectors ",
> > - pipe->mode_str, pipe->mode->vrefresh, pipe->format_str);
> > + printf("setting mode %s-%.2fHz@%s on connectors ",
> > + pipe->mode_str, pipe->vrefresh,
> > + 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); @@
> > -1713,8 +1724,13 @@ static int parse_connector(struct pipe_arg *pipe,
> const char *arg)
> > pipe->mode_str[len] = '\0';
> >
> > if (*p == '-') {
> > - pipe->vrefresh = strtoul(p + 1, &endp, 10);
> > + float temp;
> > +
> > + pipe->vrefresh = strtof(p + 1, &endp);
> > p = endp;
> > + /* Round off to 2 decimal places */
> > + temp = (int) (pipe->vrefresh * 100 + 0.5);
> > + pipe->vrefresh = (float) temp / 100;
> > }
> >
> > if (*p == '@') {
> > --
> > 2.7.4
> >
> > This email and any attachments are intended for the sole use of the named
> recipient(s) and contain(s) confidential information that may be proprietary,
> privileged or copyrighted under applicable law. If you are not the intended
> recipient, do not read, copy, or forward this email message or any attachments.
> Delete this email message and any attachments immediately.
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
