All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH libdrm v4] modetest: Use floating vrefresh while dumping mode
@ 2019-12-03 14:37 Devarsh Thakkar
  2019-12-05 13:16 ` Eric Engestrom
  2019-12-11 15:49 ` Ville Syrjälä
  0 siblings, 2 replies; 5+ messages in thread
From: Devarsh Thakkar @ 2019-12-03 14:37 UTC (permalink / raw)
  To: dri-devel, ville.syrjala, narmstrong
  Cc: rsk, vcu-team, dshah, devarsh.thakkar, varunkum, rvisaval

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.

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@xilinx.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH libdrm v4] modetest: Use floating vrefresh while dumping mode
  2019-12-03 14:37 [PATCH libdrm v4] modetest: Use floating vrefresh while dumping mode Devarsh Thakkar
@ 2019-12-05 13:16 ` Eric Engestrom
  2019-12-05 14:18   ` Ville Syrjälä
  2019-12-09 12:56   ` Devarsh Thakkar
  2019-12-11 15:49 ` Ville Syrjälä
  1 sibling, 2 replies; 5+ messages in thread
From: Eric Engestrom @ 2019-12-05 13:16 UTC (permalink / raw)
  To: Devarsh Thakkar; +Cc: dri-devel

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.

Can I also suggest you split the unrelated changes into individual
commits?

> 
> 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@xilinx.com>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH libdrm v4] modetest: Use floating vrefresh while dumping mode
  2019-12-05 13:16 ` Eric Engestrom
@ 2019-12-05 14:18   ` Ville Syrjälä
  2019-12-09 12:56   ` Devarsh Thakkar
  1 sibling, 0 replies; 5+ messages in thread
From: Ville Syrjälä @ 2019-12-05 14:18 UTC (permalink / raw)
  To: Eric Engestrom; +Cc: Devarsh Thakkar, dri-devel

On Thu, Dec 05, 2019 at 01:16:32PM +0000, Eric Engestrom wrote:
> 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

Yuck. I'll not use that at least.

-- 
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] 5+ messages in thread

* RE: [PATCH libdrm v4] modetest: Use floating vrefresh while dumping mode
  2019-12-05 13:16 ` Eric Engestrom
  2019-12-05 14:18   ` Ville Syrjälä
@ 2019-12-09 12:56   ` Devarsh Thakkar
  1 sibling, 0 replies; 5+ messages in thread
From: Devarsh Thakkar @ 2019-12-09 12:56 UTC (permalink / raw)
  To: Eric Engestrom, Ville Syrjälä
  Cc: Ranganathan Sk, Dhaval Rajeshbhai Shah, Jeegar Patel, dri-devel,
	Varunkumar Allagadapa, Rohit Visavalia

Hi Eric, Ville,

Thanks for the review, please see inline.

> -----Original Message-----
> From: Eric Engestrom <eric.engestrom@intel.com>
> Sent: 05 December 2019 05:17
> To: Devarsh Thakkar <DEVARSHT@xilinx.com>
> Cc: dri-devel@lists.freedesktop.org
> 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@xilinx.com>
> > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> > ---
> >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH libdrm v4] modetest: Use floating vrefresh while dumping mode
  2019-12-03 14:37 [PATCH libdrm v4] modetest: Use floating vrefresh while dumping mode Devarsh Thakkar
  2019-12-05 13:16 ` Eric Engestrom
@ 2019-12-11 15:49 ` Ville Syrjälä
  1 sibling, 0 replies; 5+ messages in thread
From: Ville Syrjälä @ 2019-12-11 15:49 UTC (permalink / raw)
  To: Devarsh Thakkar
  Cc: rsk, vcu-team, dshah, narmstrong, dri-devel, varunkum, rvisaval

On Tue, Dec 03, 2019 at 06:37:36AM -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.
> 
> 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@xilinx.com>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Thanks. lgtm -> pushed to master.

> ---
>  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

-- 
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] 5+ messages in thread

end of thread, other threads:[~2019-12-11 15:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-03 14:37 [PATCH libdrm v4] modetest: Use floating vrefresh while dumping mode Devarsh Thakkar
2019-12-05 13:16 ` Eric Engestrom
2019-12-05 14:18   ` Ville Syrjälä
2019-12-09 12:56   ` Devarsh Thakkar
2019-12-11 15:49 ` Ville Syrjälä

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.