Question on 640x480 @ 72fps

Question on 640x480 @ 72fps

[abhinavk]

Hello

During one of our internal tests, we ran into an issue where the 
calculated refresh rate for the mode using the drm_mode_vrefresh() API 
doesnt
match the theoretical value due to rounding.

552    { DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 31500, 640, 664,
553           704,  832, 0, 480, 489, 492, 520, 0,
554           DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC) }, /* 
640x480@72Hz */


int drm_mode_vrefresh(const struct drm_display_mode *mode)
{
     int refresh = 0;

     if (mode->vrefresh > 0)
         refresh = mode->vrefresh;
     else if (mode->htotal > 0 && mode->vtotal > 0) {
         unsigned int num, den;

         num = mode->clock * 1000;
         den = mode->htotal * mode->vtotal;

         if (mode->flags & DRM_MODE_FLAG_INTERLACE)
             num *= 2;
         if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
             den *= 2;
         if (mode->vscan > 1)
             den *= mode->vscan;

         refresh = DIV_ROUND_CLOSEST(num, den);
     }
     return refresh;
}
EXPORT_SYMBOL(drm_mode_vrefresh);

As per the math of this API, the vrefresh comes up to 72.8 fps ( 31500 * 
1000 ) / (832 * 520) .

Hence this gets rounded to 73fps.

However as per the spec, this mode should have the vrefresh as 72fps.

So to satisfy that, we must round-down in this function. That might 
break other modes though.

Do you have any suggestions on how to fix-up this mode ? Shall we just 
directly specify the vrefresh in the edid_est_modes[] and 
drm_dmt_modes[] static array?

I can submit a PATCH based on the approach we agree on here.

Thanks

Abhinav
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Question on 640x480 @ 72fps

[Ville Syrjälä]

On Thu, Aug 30, 2018 at 07:32:58PM -0700, abhinavk@codeaurora.org wrote:
> Hello
> 
> During one of our internal tests, we ran into an issue where the 
> calculated refresh rate for the mode using the drm_mode_vrefresh() API 
> doesnt
> match the theoretical value due to rounding.
> 
> 552    { DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 31500, 640, 664,
> 553           704,  832, 0, 480, 489, 492, 520, 0,
> 554           DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC) }, /* 
> 640x480@72Hz */
> 
> 
> int drm_mode_vrefresh(const struct drm_display_mode *mode)
> {
>      int refresh = 0;
> 
>      if (mode->vrefresh > 0)
>          refresh = mode->vrefresh;
>      else if (mode->htotal > 0 && mode->vtotal > 0) {
>          unsigned int num, den;
> 
>          num = mode->clock * 1000;
>          den = mode->htotal * mode->vtotal;
> 
>          if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>              num *= 2;
>          if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
>              den *= 2;
>          if (mode->vscan > 1)
>              den *= mode->vscan;
> 
>          refresh = DIV_ROUND_CLOSEST(num, den);
>      }
>      return refresh;
> }
> EXPORT_SYMBOL(drm_mode_vrefresh);
> 
> As per the math of this API, the vrefresh comes up to 72.8 fps ( 31500 * 
> 1000 ) / (832 * 520) .
> 
> Hence this gets rounded to 73fps.
> 
> However as per the spec, this mode should have the vrefresh as 72fps.

Why should anyone care if it gets rounded differently?

> 
> So to satisfy that, we must round-down in this function. That might 
> break other modes though.
> 
> Do you have any suggestions on how to fix-up this mode ? Shall we just 
> directly specify the vrefresh in the edid_est_modes[] and 
> drm_dmt_modes[] static array?
> 
> I can submit a PATCH based on the approach we agree on here.
> 
> Thanks
> 
> Abhinav
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: Question on 640x480 @ 72fps

[Sean Paul]

On Thu, Aug 30, 2018 at 07:32:58PM -0700, abhinavk@codeaurora.org wrote:
> Hello
> 
> During one of our internal tests, we ran into an issue where the calculated
> refresh rate for the mode using the drm_mode_vrefresh() API doesnt
> match the theoretical value due to rounding.
> 
> 552    { DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 31500, 640, 664,
> 553           704,  832, 0, 480, 489, 492, 520, 0,
> 554           DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC) }, /*
> 640x480@72Hz */
> 
> 
> int drm_mode_vrefresh(const struct drm_display_mode *mode)
> {
>     int refresh = 0;
> 
>     if (mode->vrefresh > 0)
>         refresh = mode->vrefresh;
>     else if (mode->htotal > 0 && mode->vtotal > 0) {
>         unsigned int num, den;
> 
>         num = mode->clock * 1000;
>         den = mode->htotal * mode->vtotal;
> 
>         if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>             num *= 2;
>         if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
>             den *= 2;
>         if (mode->vscan > 1)
>             den *= mode->vscan;
> 
>         refresh = DIV_ROUND_CLOSEST(num, den);
>     }
>     return refresh;
> }
> EXPORT_SYMBOL(drm_mode_vrefresh);
> 
> As per the math of this API, the vrefresh comes up to 72.8 fps ( 31500 *
> 1000 ) / (832 * 520) .
> 
> Hence this gets rounded to 73fps.
> 
> However as per the spec, this mode should have the vrefresh as 72fps.

I'm not sure where the official spec is, but this random webpage [1] I found with
Google has the same timing values as you have above. The timing information for
the mode doesn't specify the refresh rate, but rather the pclk. The refresh rate
comes from (pclk / (vtotal * htotal)), which comes out to 72.8Hz. We have to
round one way or the other, so DIV_ROUND_CLOSEST is more correct.

Like Ville said, the rounding doesn't really make a difference. Since we'll be
generating the correct pixel clock with the correct pitch, the hardware will be
operating with a 72.8Hz refresh.

Hopefully that makes sense?

Sean

[1] http://martin.hinner.info/vga/timing.html

> 
> So to satisfy that, we must round-down in this function. That might break
> other modes though.
> 
> Do you have any suggestions on how to fix-up this mode ? Shall we just
> directly specify the vrefresh in the edid_est_modes[] and drm_dmt_modes[]
> static array?
> 
> I can submit a PATCH based on the approach we agree on here.
> 
> Thanks
> 
> Abhinav
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Question on 640x480 @ 72fps

[Abhinav Kumar]

Hi Sean/Ville

Thanks for the comments.

This mode 640x480 @ 72Hz comes directly from the VESA spec ( DMT 
Standards and Guidelines Summary ).

Yes, I understand that the hardware will still be running at 72.8 Hz.

The background behind the test is its actually testing out the EDID 
parser with different EDID blobs to make sure the modes
are populated correctly and this rounding introduces a mismatch between 
the expected and observed mode.

This is not a functionality failure OR any incorrect visual behavior at 
this point but just an error captured from the parsed result.

Hence we wanted to highlight it.

I agree that DIV_ROUND_CLOSEST is the way to go but due to this specific 
mode, the parsed result is incorrect.

Looking at the larger picture, if we should ignore this, we will do 
that.

Thanks

Abhinav

On 2018-08-31 05:23, Sean Paul wrote:
> On Thu, Aug 30, 2018 at 07:32:58PM -0700, abhinavk@codeaurora.org 
> wrote:
>> Hello
>> 
>> During one of our internal tests, we ran into an issue where the 
>> calculated
>> refresh rate for the mode using the drm_mode_vrefresh() API doesnt
>> match the theoretical value due to rounding.
>> 
>> 552    { DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 31500, 640, 664,
>> 553           704,  832, 0, 480, 489, 492, 520, 0,
>> 554           DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC) }, /*
>> 640x480@72Hz */
>> 
>> 
>> int drm_mode_vrefresh(const struct drm_display_mode *mode)
>> {
>>     int refresh = 0;
>> 
>>     if (mode->vrefresh > 0)
>>         refresh = mode->vrefresh;
>>     else if (mode->htotal > 0 && mode->vtotal > 0) {
>>         unsigned int num, den;
>> 
>>         num = mode->clock * 1000;
>>         den = mode->htotal * mode->vtotal;
>> 
>>         if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>>             num *= 2;
>>         if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
>>             den *= 2;
>>         if (mode->vscan > 1)
>>             den *= mode->vscan;
>> 
>>         refresh = DIV_ROUND_CLOSEST(num, den);
>>     }
>>     return refresh;
>> }
>> EXPORT_SYMBOL(drm_mode_vrefresh);
>> 
>> As per the math of this API, the vrefresh comes up to 72.8 fps ( 31500 
>> *
>> 1000 ) / (832 * 520) .
>> 
>> Hence this gets rounded to 73fps.
>> 
>> However as per the spec, this mode should have the vrefresh as 72fps.
> 
> I'm not sure where the official spec is, but this random webpage [1] I
> found with
> Google has the same timing values as you have above. The timing 
> information for
> the mode doesn't specify the refresh rate, but rather the pclk. The 
> refresh rate
> comes from (pclk / (vtotal * htotal)), which comes out to 72.8Hz. We 
> have to
> round one way or the other, so DIV_ROUND_CLOSEST is more correct.
> 
> Like Ville said, the rounding doesn't really make a difference. Since 
> we'll be
> generating the correct pixel clock with the correct pitch, the hardware 
> will be
> operating with a 72.8Hz refresh.
> 
> Hopefully that makes sense?
> 
> Sean
> 
> [1] http://martin.hinner.info/vga/timing.html
> 
>> 
>> So to satisfy that, we must round-down in this function. That might 
>> break
>> other modes though.
>> 
>> Do you have any suggestions on how to fix-up this mode ? Shall we just
>> directly specify the vrefresh in the edid_est_modes[] and 
>> drm_dmt_modes[]
>> static array?
>> 
>> I can submit a PATCH based on the approach we agree on here.
>> 
>> Thanks
>> 
>> Abhinav
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Question on 640x480 @ 72fps

[Daniel Vetter]

On Fri, Aug 31, 2018 at 11:07:12AM -0700, Abhinav Kumar wrote:
> Hi Sean/Ville
> 
> Thanks for the comments.
> 
> This mode 640x480 @ 72Hz comes directly from the VESA spec ( DMT Standards
> and Guidelines Summary ).
> 
> Yes, I understand that the hardware will still be running at 72.8 Hz.
> 
> The background behind the test is its actually testing out the EDID parser
> with different EDID blobs to make sure the modes
> are populated correctly and this rounding introduces a mismatch between the
> expected and observed mode.
> 
> This is not a functionality failure OR any incorrect visual behavior at this
> point but just an error captured from the parsed result.
> 
> Hence we wanted to highlight it.
> 
> I agree that DIV_ROUND_CLOSEST is the way to go but due to this specific
> mode, the parsed result is incorrect.
> 
> Looking at the larger picture, if we should ignore this, we will do that.

Do not use the compute vrefresh in code. It's only meant for human
consumption, not anything else. The mode->clock is the important part.
-Daniel

> 
> Thanks
> 
> Abhinav
> 
> On 2018-08-31 05:23, Sean Paul wrote:
> > On Thu, Aug 30, 2018 at 07:32:58PM -0700, abhinavk@codeaurora.org wrote:
> > > Hello
> > > 
> > > During one of our internal tests, we ran into an issue where the
> > > calculated
> > > refresh rate for the mode using the drm_mode_vrefresh() API doesnt
> > > match the theoretical value due to rounding.
> > > 
> > > 552    { DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 31500, 640, 664,
> > > 553           704,  832, 0, 480, 489, 492, 520, 0,
> > > 554           DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC) }, /*
> > > 640x480@72Hz */
> > > 
> > > 
> > > int drm_mode_vrefresh(const struct drm_display_mode *mode)
> > > {
> > >     int refresh = 0;
> > > 
> > >     if (mode->vrefresh > 0)
> > >         refresh = mode->vrefresh;
> > >     else if (mode->htotal > 0 && mode->vtotal > 0) {
> > >         unsigned int num, den;
> > > 
> > >         num = mode->clock * 1000;
> > >         den = mode->htotal * mode->vtotal;
> > > 
> > >         if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> > >             num *= 2;
> > >         if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > >             den *= 2;
> > >         if (mode->vscan > 1)
> > >             den *= mode->vscan;
> > > 
> > >         refresh = DIV_ROUND_CLOSEST(num, den);
> > >     }
> > >     return refresh;
> > > }
> > > EXPORT_SYMBOL(drm_mode_vrefresh);
> > > 
> > > As per the math of this API, the vrefresh comes up to 72.8 fps (
> > > 31500 *
> > > 1000 ) / (832 * 520) .
> > > 
> > > Hence this gets rounded to 73fps.
> > > 
> > > However as per the spec, this mode should have the vrefresh as 72fps.
> > 
> > I'm not sure where the official spec is, but this random webpage [1] I
> > found with
> > Google has the same timing values as you have above. The timing
> > information for
> > the mode doesn't specify the refresh rate, but rather the pclk. The
> > refresh rate
> > comes from (pclk / (vtotal * htotal)), which comes out to 72.8Hz. We
> > have to
> > round one way or the other, so DIV_ROUND_CLOSEST is more correct.
> > 
> > Like Ville said, the rounding doesn't really make a difference. Since
> > we'll be
> > generating the correct pixel clock with the correct pitch, the hardware
> > will be
> > operating with a 72.8Hz refresh.
> > 
> > Hopefully that makes sense?
> > 
> > Sean
> > 
> > [1] http://martin.hinner.info/vga/timing.html
> > 
> > > 
> > > So to satisfy that, we must round-down in this function. That might
> > > break
> > > other modes though.
> > > 
> > > Do you have any suggestions on how to fix-up this mode ? Shall we just
> > > directly specify the vrefresh in the edid_est_modes[] and
> > > drm_dmt_modes[]
> > > static array?
> > > 
> > > I can submit a PATCH based on the approach we agree on here.
> > > 
> > > Thanks
> > > 
> > > Abhinav
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel