* [PATCH libdrm v2] modetest: Add support for setting mode having floating vertical refresh rate
@ 2019-11-06 15:35 Devarsh Thakkar
2019-11-07 11:13 ` Ville Syrjälä
0 siblings, 1 reply; 3+ messages in thread
From: Devarsh Thakkar @ 2019-11-06 15:35 UTC (permalink / raw)
To: dri-devel; +Cc: rsk, vcu-team, dshah, devarsh.thakkar, varunkum, satishna
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@xilinx.com>
---
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);
+ /* Round off to 2 decimal places to match with user
+ * provided value
+ */
+ temp = (int) (mode_vrefresh * 100 + 0.5);
+ mode_vrefresh = (float) temp / 100;
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH libdrm v2] modetest: Add support for setting mode having floating vertical refresh rate
2019-11-06 15:35 [PATCH libdrm v2] modetest: Add support for setting mode having floating vertical refresh rate Devarsh Thakkar
@ 2019-11-07 11:13 ` Ville Syrjälä
2019-11-11 12:07 ` Devarsh Thakkar
0 siblings, 1 reply; 3+ messages in thread
From: Ville Syrjälä @ 2019-11-07 11:13 UTC (permalink / raw)
To: Devarsh Thakkar; +Cc: rsk, vcu-team, dshah, dri-devel, varunkum, satishna
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@xilinx.com>
> ---
> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread* RE: [PATCH libdrm v2] modetest: Add support for setting mode having floating vertical refresh rate
2019-11-07 11:13 ` Ville Syrjälä
@ 2019-11-11 12:07 ` Devarsh Thakkar
0 siblings, 0 replies; 3+ messages in thread
From: Devarsh Thakkar @ 2019-11-11 12:07 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Ranganathan Sk, vcu-team, Dhaval Rajeshbhai Shah,
dri-devel@lists.freedesktop.org, Varunkumar Allagadapa,
Satish Kumar Nagireddy
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@linux.intel.com>
> Sent: 07 November 2019 03:13
> To: Devarsh Thakkar <DEVARSHT@xilinx.com>
> Cc: dri-devel@lists.freedesktop.org; Ranganathan Sk <rsk@xilinx.com>; vcu-
> team <vcu-team@xilinx.com>; Dhaval Rajeshbhai Shah <dshah@xilinx.com>;
> Varunkumar Allagadapa <VARUNKUM@xilinx.com>; Satish Kumar Nagireddy
> <SATISHNA@xilinx.com>
> 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@xilinx.com>
> > ---
> > 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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-11 12:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-11-06 15:35 [PATCH libdrm v2] modetest: Add support for setting mode having floating vertical refresh rate Devarsh Thakkar
2019-11-07 11:13 ` Ville Syrjälä
2019-11-11 12:07 ` Devarsh Thakkar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.