Re: [PATCH libdrm v3] modetest: Use floating vrefresh while dumping mode

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Dec 02, 2019 at 06:22:56PM +0100, Neil Armstrong wrote:
> On 02/12/2019 18:12, Ville Syrjälä wrote:
> > On Mon, Dec 02, 2019 at 03:27:51AM -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.
> >>
> >> V3: 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>
> >> ---
> >>  tests/modetest/modetest.c | 40 +++++++++++++++++++++++++++-------------
> >>  1 file changed, 27 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> >> index b4edfcb..19ce20f 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,
> >> @@ -823,12 +829,11 @@ struct plane_arg {
> >>  
> >>  static drmModeModeInfo *
> >>  connector_find_mode(struct device *dev, uint32_t con_id, const char *mode_str,
> >> -	const float vrefresh)
> >> +	float *vrefresh)
> > 
> > This change still looks pointless.
> 
> Without this, you cannot set 1000/1001 CEA alternate clock modes, so, no this is not pointless.

I'm talking about this specific s/float/float*/ change. That
is pointless. Are you talking about the whole patch or what?

> 
> This is actual something I always wanted to implement !
> 
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> > 
> >>  {
> >>  	drmModeConnector *connector;
> >>  	drmModeModeInfo *mode;
> >>  	int i;
> >> -	float mode_vrefresh;
> >>  
> >>  	connector = get_connector_by_id(dev, con_id);
> >>  	if (!connector || !connector->count_modes)
> >> @@ -837,16 +842,19 @@ 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
> >> +			 * and update corresponding vrefresh in pipe_arg.
> >> +			 * 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)
> >> +			if (*vrefresh == 0) {
> >> +				*vrefresh = mode_vrefresh(mode);
> >>  				return mode;
> >> -			else if (fabs(mode_vrefresh - vrefresh) < 0.005)
> >> +			} else if (fabs(mode_vrefresh(mode)
> >> +				   - *vrefresh) < 0.005) {
> >>  				return mode;
> >> +			}
> >>  		}
> >>  	}
> >>  
> >> @@ -909,9 +917,15 @@ static int pipe_find_crtc_and_mode(struct device *dev, struct pipe_arg *pipe)
> >>  
> >>  	for (i = 0; i < (int)pipe->num_cons; i++) {
> >>  		mode = connector_find_mode(dev, pipe->con_ids[i],
> >> -					   pipe->mode_str, pipe->vrefresh);
> >> +					   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;
> >> -- 
> >> 2.7.4
> > 

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel