AW: AW V2: [PATCH xserver] xfree86: Refine drm modes on continuous freq panel

Walter Harms wharms at bfs.de
Tue Apr 14 08:51:35 UTC 2020


________________________________________
Von: Aaron Ma <aaron.ma at canonical.com>
Gesendet: Dienstag, 14. April 2020 10:30
An: Walter Harms; xorg-devel at lists.x.org
Betreff: Re: AW V2: [PATCH xserver] xfree86: Refine drm modes on continuous freq panel

On 4/13/20 5:11 PM, Walter Harms wrote:
>
>
> ________________________________________
> Von: xorg-devel <xorg-devel-bounces at lists.x.org> im Auftrag von Aaron Ma <aaron.ma at canonical.com>
> Gesendet: Samstag, 11. April 2020 12:14
> An: xorg-devel at lists.x.org; aaron.ma at canonical.com
> Betreff: [PATCH xserver] xfree86: Refine drm modes on continuous freq panel
>
> EDID1.4 replaced GTF Bit with Continuous or Non-Continuous Frequency Display.
>
> Check the "Display Range Limits Descriptor" for GTF support.
> If panel doesn't support GTF, then add gtf modes.
>
> Otherwise X will only show the modes in "Detailed Timing Descriptor".
>
> BugLink: https://gitlab.freedesktop.org/drm/intel/issues/313
> Signed-off-by: Aaron Ma <aaron.ma at canonical.com>
> ---
>  hw/xfree86/ddc/edid.h                         |  6 ++++
>  hw/xfree86/ddc/interpret_edid.c               | 32 +++++++++++++++++++
>  hw/xfree86/ddc/xf86DDC.h                      |  3 ++
>  .../drivers/modesetting/drmmode_display.c     |  2 +-
>  4 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/hw/xfree86/ddc/edid.h b/hw/xfree86/ddc/edid.h
> index 750e4270b..dffcb3749 100644
> --- a/hw/xfree86/ddc/edid.h
> +++ b/hw/xfree86/ddc/edid.h
> @@ -262,6 +262,10 @@
>  #define MAX_H (_MAX_H(c) + _MAX_H_OFFSET(c))
>  #define _MAX_CLOCK(x) x[9]
>  #define MAX_CLOCK _MAX_CLOCK(c)
> +#define _DEFAULT_GTF(x) (x[10] == 0x00)
> +#define DEFAULT_GTF _DEFAULT_GTF(c)
> +#define _RANGE_LIMITS_ONLY(x) (x[10] == 0x01)
> +#define RANGE_LIMITS_ONLY _RANGE_LIMITS_ONLY(c)
>  #define _HAVE_2ND_GTF(x) (x[10] == 0x02)
>  #define HAVE_2ND_GTF _HAVE_2ND_GTF(c)
>  #define _F_2ND_GTF(x) (x[12] * 2)
> @@ -490,6 +494,8 @@ struct monitor_ranges {
>      int gtf_2nd_j;
>      int max_clock_khz;
>      int maxwidth;               /* in pixels */
> +    BOOL supported_default_gtf;
> +    BOOL range_limits_only;
>      char supported_aspect;
>      char preferred_aspect;
>      char supported_blanking;
> diff --git a/hw/xfree86/ddc/interpret_edid.c b/hw/xfree86/ddc/interpret_edid.c
> index 17a8f81c0..4f3cbc587 100644
> --- a/hw/xfree86/ddc/interpret_edid.c
> +++ b/hw/xfree86/ddc/interpret_edid.c
> @@ -672,7 +672,19 @@ get_monitor_ranges(Uchar * c, struct monitor_ranges *r)
>      r->max_clock = 0;
>      if (MAX_CLOCK != 0xff)      /* is specified? */
>          r->max_clock = MAX_CLOCK * 10 + 5;
> +
> +    if (DEFAULT_GTF) {
> +       r->supported_default_gtf = TRUE;
> +    } else
> +       r->supported_default_gtf = FALSE;
> +
> +    if (RANGE_LIMITS_ONLY) {
> +       r->range_limits_only = TRUE;
> +    } else
> +       r->range_limits_only = FALSE;
> +
>      if (HAVE_2ND_GTF) {
> +        r->supported_default_gtf = TRUE;
>          r->gtf_2nd_f = F_2ND_GTF;
>          r->gtf_2nd_c = C_2ND_GTF;
>          r->gtf_2nd_m = M_2ND_GTF;
> @@ -751,6 +763,26 @@ validate_version(int scrnIndex, struct edid_version *r)
>      return TRUE;
>  }
>
> +Bool
> +gtf_supported(int scrnIndex, xf86MonPtr mon)
> +{
>
> +    struct detailed_monitor_section *det_timing_des = mon->det_mon;
> +    int i;
> +
> +    if (mon && (mon->ver.revision < 4)) {
>
> can mon be NULL ? if yes det_timing_des = mon->det_mon; will crash.
> a simple
>            if (!mon)
>                  return FALSE;
>
> clears the fog.

Will change in V2.


>
> +        if (GTF_SUPPORTED(mon->features.msc))
> +           return TRUE;
> +    } else {
>
> maybe you can improve readability this way:
>
>  if ( mon->ver.revision < 4 ) {
>          if (GTF_SUPPORTED(mon->features.msc))
>            return TRUE;
>           else
>             return FALSE;
>    }

Only return TRUE in if condition.
Default return FALSE at the function end.


so far i understand there is no need to loop if you have Version < 4
so you can return early.

happy hacking
re,
 wh

>
>
>
>
>
> +        for (i = 0; i < DET_TIMINGS; det_timing_des = &(mon->det_mon[++i])) {
> +           if ((det_timing_des) && (det_timing_des->type == DS_RANGES))
>
> using a simple loop makes it more readable, and avoids the init at start
> (case mon==NULL). The scope of det_timing_des is small you could call it dms or so.
>
>          for(i=0;i < DET_TIMINGS;i++)
>           {
>               struct detailed_monitor_section *det_timing_des = &(mon->det_mon[i]))

Change in V2.

>                if (! det_timing_des)                                    // is this possible ?
>                      return FALSE;

No this will change the behavior, all det_timing_des shoulde be checked,
in case there are NULL detailed timing descriptor.

Thanks,
Aaron

>                 if ( det_timing_des->type == DS_RANGES &&
>                        det_timing_des->section.ranges.supported_default_gtf )
>                         return TRUE;
>
>          }
>
> hope that helps,
> re,
>  wh
> +               if (det_timing_des->section.ranges.supported_default_gtf)
> +                   return TRUE;
> +       }
> +    }
> +
> +    return FALSE;
> +}
> +
>  /*
>   * Returns true if HDMI, false if definitely not or unknown.
>   */
> diff --git a/hw/xfree86/ddc/xf86DDC.h b/hw/xfree86/ddc/xf86DDC.h
> index 7d81ab911..2fa4ba55a 100644
> --- a/hw/xfree86/ddc/xf86DDC.h
> +++ b/hw/xfree86/ddc/xf86DDC.h
> @@ -48,6 +48,9 @@ extern _X_EXPORT Bool xf86SetDDCproperties(ScrnInfoPtr pScreen, xf86MonPtr DDC);
>  extern _X_EXPORT Bool
>   xf86MonitorIsHDMI(xf86MonPtr mon);
>
> +extern _X_EXPORT Bool
> +gtf_supported(int scrnIndex, xf86MonPtr mon);
> +
>  extern _X_EXPORT DisplayModePtr
>  FindDMTMode(int hsize, int vsize, int refresh, Bool rb);
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 8e6b697c4..031e5efea 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -2459,7 +2459,7 @@ drmmode_output_add_gtf_modes(xf86OutputPtr output, DisplayModePtr Modes)
>      int max_x = 0, max_y = 0;
>      float max_vrefresh = 0.0;
>
> -    if (mon && GTF_SUPPORTED(mon->features.msc))
> +    if (mon && gtf_supported(output->scrn->scrnIndex, mon))
>          return Modes;
>
>      if (!has_panel_fitter(output))
> --
> 2.26.0
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
>


More information about the xorg-devel mailing list