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 <[email protected]>
> ---
> 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
