________________________________________ Von: Aaron Ma <aaron...@canonical.com> Gesendet: Dienstag, 14. April 2020 10:30 An: Walter Harms; xorg-devel@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-boun...@lists.x.org> im Auftrag von Aaron Ma > <aaron...@canonical.com> > Gesendet: Samstag, 11. April 2020 12:14 > An: xorg-devel@lists.x.org; aaron...@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...@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@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@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@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
