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

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

On Tue, Dec 03, 2019 at 06:49:31AM +0000, Devarsh Thakkar wrote:
> Thanks for the review Ville and Neil, response inline.
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: 02 December 2019 09:43
> > To: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Devarsh Thakkar <DEVARSHT@xilinx.com>; Ranganathan Sk
> > <rsk@xilinx.com>; vcu-team <vcu-team@xilinx.com>; Dhaval Rajeshbhai Shah
> > <dshah@xilinx.com>; dri-devel@lists.freedesktop.org; Varunkumar
> > Allagadapa <VARUNKUM@xilinx.com>
> > Subject: Re: [PATCH libdrm v3] modetest: Use floating vrefresh while dumping
> > mode
> > 
> > EXTERNAL EMAIL
> > 
> > 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.
> > >
> 
> The only small advantage it offers is that it backs up the vrefresh from mode in pipe_args in connector_find_mode and I don't have to call mode_vrefresh again while printing below :
> printf("setting mode %s-%.2fHz on connectors ",
>                        pipe->mode_str, pipe->vrefresh);
> 
> If this is not preferable then it's not mandatory either, then instead of doing this I can call mode_vrefresh again while printing the mode as below :
> printf("setting mode %s-%.2fHz on connectors ",
>                        pipe->mode_str, mode_vrefresh(pipe->mode))

I would change that to to use mode_vrefres(), and also IMO better to
replace the pipe->mode_str with mode->name as well. That way the code
doesn't have to care at all how the mode was chosen.

> 
> Kindly let me know your opinion.
> 
> > > 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>
> > >
> 
> Thanks for the review Neil.
> 
> > > >
> > > >>  {
> > > >>    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

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