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

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

On Wed, Nov 27, 2019 at 07:28:31AM +0000, Devarsh Thakkar wrote:
> Thanks for the review Ville, please see inline:
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: 26 November 2019 07:15
> > To: Devarsh Thakkar <DEVARSHT@xilinx.com>
> > Cc: dri-devel@lists.freedesktop.org; Hyun Kwon <hyunk@xilinx.com>; vcu-
> > team <vcu-team@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>; Dhaval
> > Rajeshbhai Shah <dshah@xilinx.com>; Satish Kumar Nagireddy
> > <SATISHNA@xilinx.com>; Varunkumar Allagadapa <VARUNKUM@xilinx.com>;
> > Rohit Visavalia <RVISAVAL@xilinx.com>
> > Subject: Re: [PATCH libdrm] modetest: Use floating vrefresh while dumping
> > mode
> > 
> > EXTERNAL EMAIL
> > 
> > On Tue, Nov 26, 2019 at 07:03:58AM -0800, Devarsh Thakkar wrote:
> > > Add inline function to derive floating value of vertical refresh rate
> > > from 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 so that
> > > it will be used for printing floating vrefresh while dumping mode.
> > >
> > > 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.
> > >
> > > Signed-off-by: Devarsh Thakkar <devarsh.thakkar@xilinx.com>
> > > ---
> > >  tests/modetest/modetest.c | 36 ++++++++++++++++++++++--------------
> > >  1 file changed, 22 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > > index b4edfcb..80cf076 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 inline float get_floating_vrefresh(drmModeModeInfo *mode)
> > 
> > Drop the 'inline'. I'd call it just mode_vrefresh() or somehting like that.
> > 
> 
> Yes I do agree that most compilers will substitute function body if needed (and I do see that in generated assembly even without inline) but I still thought better to add inline keyword just to give the hint to compiler in case if it doesn't.
> Any specific reason or issues if we use inline ?

My usual rule is to use static inline only in header files,
otherwise I never use it unless it shows a tangible performance
benefit.

> 
> 
> > > +{
> > > +     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,
> > > +            get_floating_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)
> > >  {
> > >       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 = get_floating_vrefresh(mode);
> > >                               return mode;
> > > -                     else if (fabs(mode_vrefresh - vrefresh) < 0.005)
> > > +                     } else if (fabs(get_floating_vrefresh(mode)
> > > +                                - *vrefresh) < 0.005) {
> > >                               return mode;
> > > +                     }
> > >               }
> > >       }
> > >
> > > @@ -909,11 +917,11 @@ 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,
> > > -                             "failed to find mode \"%s\" for connector %s\n",
> > > -                             pipe->mode_str, pipe->cons[i]);
> > > +                     "failed to find mode \"%s-%.2fHz\" for connector %s\n",
> > > +                      pipe->mode_str, pipe->vrefresh, pipe->cons[i]);
> > 
> > We didn't find any mode so having connector_find_mode() update vrefresh
> > for us is nonsense.
> 
> Yes that's correct, but we need it in a scenario where user provided a vrefresh for the mode and mode is not available with that exact vrefresh but available with a different refresh rate. So for e.g if 1920x1080-60 is only available and user set 1920x1080-59.92 (which is not available). Then it should display that mode 1920x1080-59.92 is not available and not that 1920x1080 is not found (since 1920x1080-60 is available). So I guess best way to handle not found mode is to print vrefresh too only in scenario where user had provided a specific refresh rate as arg. What's your opinion ?

Yeah, only printing it if the user asked for it seems like the sanest
option.

> 
> 
> > 
> > >                       return -EINVAL;
> > >               }
> > >       }
> > > --
> > > 2.7.4
> > 
> > --
> > Ville Syrjälä
> > Intel

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