[PATCH libdrm] modetest: Use floating vrefresh while dumping mode
Devarsh Thakkar
DEVARSHT at xilinx.com
Wed Nov 27 07:28:31 UTC 2019
Thanks for the review Ville, please see inline:
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: 26 November 2019 07:15
> To: Devarsh Thakkar <DEVARSHT at xilinx.com>
> Cc: dri-devel at lists.freedesktop.org; Hyun Kwon <hyunk at xilinx.com>; vcu-
> team <vcu-team at xilinx.com>; Ranganathan Sk <rsk at xilinx.com>; Dhaval
> Rajeshbhai Shah <dshah at xilinx.com>; Satish Kumar Nagireddy
> <SATISHNA at xilinx.com>; Varunkumar Allagadapa <VARUNKUM at xilinx.com>;
> Rohit Visavalia <RVISAVAL at 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 at 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 ?
> > +{
> > + 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 ?
>
> > return -EINVAL;
> > }
> > }
> > --
> > 2.7.4
>
> --
> Ville Syrjälä
> Intel
More information about the dri-devel
mailing list