Re: [Intel-gfx] [PATCH 1/2] drm/edid: adjust double-clocked cea modes
On Mon, May 14, 2012 at 04:55:36PM -0300, Paulo Zanoni wrote: > 2012/5/12 Daniel Vetter : > > The CEA spec has a bunch of very peculiar modes. For backwards > > compatibility it specifies a bunch of modes that are suitable to > > display old SD TV content. But these modes have such low pixel clocks > > that pixels need to be doubled to reach the minimal clock of the HDMI > > interface. > > > > I just tested the 2 patches. They don't work for me... My monitor > complains the timings are not supported, and now I get a black screen > instead of a screen with half of the vertical pixel columns. > > HTOTAL_B: 0x035f02cf (720 active, 864 total) > HBLANK_B: 0x035f02cf (720 start, 864 end) >HSYNC_B: 0x031a02db (732 start, 795 end) > VTOTAL_B: 0x026f023f (576 active, 624 total) > VBLANK_B: 0x026f023f (576 start, 624 end) >VSYNC_B: 0x02490243 (580 start, 586 end) > > I believe the timings sent to the hardware must be the ones we were > already sending before the patches... Well, that is what actually _should_ happen. The crtc doubles every pixel, so all horizontal timings should be twice what we program into the registers. Looks like something is still amiss :( -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/edid: adjust double-clocked cea modes
On Tue, May 15, 2012 at 11:33:43AM -0400, Adam Jackson wrote: > On 5/14/12 3:43 PM, Paulo Zanoni wrote: > > >Also, I think flag DRM_MODE_FLAG_DBLCLK does not sound correct for > >them, so we would need to create flags: > >- DRM_MODE_FLAG_PR_1_to_10 > >- DRM_MODE_FLAG_PR_1_or_2 > >- DRM_MODE_FLAG_PR_1_or_2_or_4 > > > >Or any other more creative names. And then we should cross our fingers > >in the hope that they don't start creating modes with other possible > >variations of PR :) > >Anyway, how will the user be able to choose the wanted PR? What about > >the aspect ratio? Yet Another Connector Property? > > Well, one thing at a time. > > Right now we don't have the concept of a mode property. Therefore, > modes exposed to userspace should all be unique. Exposing the pixel > repeat to userspace as a connector property is wrong because it > doesn't apply to all modes for a connector. > > So my initial inclination would be to do something like this in the > flags field: > > --- a/include/drm/drm_mode.h > +++ b/include/drm/drm_mode.h > @@ -58,6 +58,9 @@ > #define DRM_MODE_FLAG_PIXMUX (1<<11) > #define DRM_MODE_FLAG_DBLCLK (1<<12) > #define DRM_MODE_FLAG_CLKDIV2 (1<<13) > +/* begin non-xorg definitions */ > +#define DRM_PIXREP_MASK(15 << 14) > +#define DRM_MODE_FLAG_PIXREP(x) (((x) << 14) & DRM_PIXMULT_MASK) > > /* DPMS flags */ > /* bit compatible with the xorg definitions. */ > > The timings for the variably-repeated modes won't change (right?) so > this is the only way to get the desired pixel repeat passed into the > modesettinng path. And then the "one to ten" and "one two or four" > modes simply need to be added to the list multiple times, once for > each possible pixel replication, and with varying names depending on > the effective framebuffer size. > > Problems with this approach are that it means rewriting the CEA mode > list or the VIC table walk or both, and that it means creating a > class of modes that userspace can set but not create (which means, > if your X driver is parsing EDID itself instead of just using the > damn kernel mode list, that many of the modes will be inaccessible > to X). Neither is insoluble, just nngh. For the variable pixel-repeat modes I'd propose that we mark them up with a flag and flat-out reject them int the cea edid parser. All these modes are super-low-res interlaced things likely only good to upscale sdtv material. At least on Intel we might as well use the hw scaler for them. The double-clocked ones look slightly more sane, so I guess we could keep these. If I manage to get them to work anywhere else than on my machine here :( > The dual-aspect-ratio modes are, well, ugly. Ideally we'd have > scaler hardware on all digital output routes so we could just make > that a connector property, and then hide the choice of mechanism > inside the drm (either in the monitor if the infoframe says so / if > it's controllable with DDC/CI or something, or in the GPU if not). > I don't know if all the digital outputs in the world live up to that > lofty goal. Failing that we could encode the widescreen-ness in the > mode name? All quite horrible. TVs really are the worst thing. tbh our current avi infoframe support is ridiculously lacking - we don't even properly work the overscan properties, set the VIC number and all kinds of stuff. I think we can defer worrying about these multi-aspect ratio modes until everything else works somewhat. Cheers, Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/edid: adjust double-clocked cea modes
On 5/14/12 3:43 PM, Paulo Zanoni wrote: Also, I think flag DRM_MODE_FLAG_DBLCLK does not sound correct for them, so we would need to create flags: - DRM_MODE_FLAG_PR_1_to_10 - DRM_MODE_FLAG_PR_1_or_2 - DRM_MODE_FLAG_PR_1_or_2_or_4 Or any other more creative names. And then we should cross our fingers in the hope that they don't start creating modes with other possible variations of PR :) Anyway, how will the user be able to choose the wanted PR? What about the aspect ratio? Yet Another Connector Property? Well, one thing at a time. Right now we don't have the concept of a mode property. Therefore, modes exposed to userspace should all be unique. Exposing the pixel repeat to userspace as a connector property is wrong because it doesn't apply to all modes for a connector. So my initial inclination would be to do something like this in the flags field: --- a/include/drm/drm_mode.h +++ b/include/drm/drm_mode.h @@ -58,6 +58,9 @@ #define DRM_MODE_FLAG_PIXMUX (1<<11) #define DRM_MODE_FLAG_DBLCLK (1<<12) #define DRM_MODE_FLAG_CLKDIV2 (1<<13) +/* begin non-xorg definitions */ +#define DRM_PIXREP_MASK(15 << 14) +#define DRM_MODE_FLAG_PIXREP(x) (((x) << 14) & DRM_PIXMULT_MASK) /* DPMS flags */ /* bit compatible with the xorg definitions. */ The timings for the variably-repeated modes won't change (right?) so this is the only way to get the desired pixel repeat passed into the modesettinng path. And then the "one to ten" and "one two or four" modes simply need to be added to the list multiple times, once for each possible pixel replication, and with varying names depending on the effective framebuffer size. Problems with this approach are that it means rewriting the CEA mode list or the VIC table walk or both, and that it means creating a class of modes that userspace can set but not create (which means, if your X driver is parsing EDID itself instead of just using the damn kernel mode list, that many of the modes will be inaccessible to X). Neither is insoluble, just nngh. The dual-aspect-ratio modes are, well, ugly. Ideally we'd have scaler hardware on all digital output routes so we could just make that a connector property, and then hide the choice of mechanism inside the drm (either in the monitor if the infoframe says so / if it's controllable with DDC/CI or something, or in the GPU if not). I don't know if all the digital outputs in the world live up to that lofty goal. Failing that we could encode the widescreen-ness in the mode name? All quite horrible. TVs really are the worst thing. I think at some point we're going to need to reconsider using xserver's data type for the mode so literally. - ajax ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/edid: adjust double-clocked cea modes
2012/5/12 Daniel Vetter : > The CEA spec has a bunch of very peculiar modes. For backwards > compatibility it specifies a bunch of modes that are suitable to > display old SD TV content. But these modes have such low pixel clocks > that pixels need to be doubled to reach the minimal clock of the HDMI > interface. > I just tested the 2 patches. They don't work for me... My monitor complains the timings are not supported, and now I get a black screen instead of a screen with half of the vertical pixel columns. HTOTAL_B: 0x035f02cf (720 active, 864 total) HBLANK_B: 0x035f02cf (720 start, 864 end) HSYNC_B: 0x031a02db (732 start, 795 end) VTOTAL_B: 0x026f023f (576 active, 624 total) VBLANK_B: 0x026f023f (576 start, 624 end) VSYNC_B: 0x02490243 (580 start, 586 end) I believe the timings sent to the hardware must be the ones we were already sending before the patches... -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/edid: adjust double-clocked cea modes
2012/5/14 Paulo Zanoni : > I haven't bothered to even try to fix them since I couldn't find any > TV/Monitor that supports them. Also, I think flag DRM_MODE_FLAG_DBLCLK does not sound correct for them, so we would need to create flags: - DRM_MODE_FLAG_PR_1_to_10 - DRM_MODE_FLAG_PR_1_or_2 - DRM_MODE_FLAG_PR_1_or_2_or_4 Or any other more creative names. And then we should cross our fingers in the hope that they don't start creating modes with other possible variations of PR :) Anyway, how will the user be able to choose the wanted PR? What about the aspect ratio? Yet Another Connector Property? Or we add the amount of PR acceptable in another field of some struct (possibly in the same struct where we're going to add the VIC number). -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/edid: adjust double-clocked cea modes
2012/5/14 Adam Jackson : > > ... right after this are the 2800x480 modes. Are they really 720, 1440, > or 2880 wide? Both before and after this change they're 2880 wide, but > the logic of this change makes me believe they should not be. These modes you're talking about (10-15, 25-30, 35-38), are even more different than the ones we're touching. The pixel data can be sent 1 to N times on these modes, not exactly 2 (like on the modes we're touching). Look at table 18 "Valid Pixel Repeat Values For Each Video Format Timing" on the spec. Considering that pixels data *can* be sent 1 time on these modes, maybe they will even work out of the box? I haven't bothered to even try to fix them since I couldn't find any TV/Monitor that supports them. Anybody here got one? And BTW, why would I ever want to use 2880x480? Since we're talking about fixing CEA modes, it might be worth mentioning that, for now, some modes are defined exactly the same (like modes 58 and 59), but the spec says that the difference between these modes is the aspect ratio, which we should specify using AVI InfoFrames. I couldn't get this working on my monitors (maybe I'm doing something wrong)... See Table 4 "Video Formats - Video ID Code and Aspect Ratios". Yet Another Problem (TM) is that we should be sending the VIC (these numbers representing the modes) on the AVI InfoFrame too... I have no idea what problems we can fix with this, but if we discover this is necessary, we need to figure out in which struct to add this kind of information to pass to the drivers... -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/edid: adjust double-clocked cea modes
On Sun, 2012-05-13 at 00:07 +0200, Daniel Vetter wrote:
> The problem left is that the CEA spec specifies these modes with
> timings as they get transmitted, but because the double-clocking is
> just to make the HDMI phy happy, the TV actually drops every 2nd
> pixel. So e.g. a 1440x576 mode in our CEA mode list is actually a
> 720x576 mode.
Well, okay, but...
> /* 9 - 1440x240@60Hz */
> - { DRM_MODE("1440x240", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
> + { DRM_MODE("720x240", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
> 1602, 1716, 0, 240, 244, 247, 262, 0,
> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
> DRM_MODE_FLAG_DBLCLK) },
... right after this are the 2800x480 modes. Are they really 720, 1440,
or 2880 wide? Both before and after this change they're 2880 wide, but
the logic of this change makes me believe they should not be.
- ajax
signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/edid: adjust double-clocked cea modes
The CEA spec has a bunch of very peculiar modes. For backwards
compatibility it specifies a bunch of modes that are suitable to
display old SD TV content. But these modes have such low pixel clocks
that pixels need to be doubled to reach the minimal clock of the HDMI
interface.
Paulo Zanoni already started to fix this up by properly marking these
modes as double-clocked and sending the correct avi infoframe as
required by CEA.
The problem left is that the CEA spec specifies these modes with
timings as they get transmitted, but because the double-clocking is
just to make the HDMI phy happy, the TV actually drops every 2nd
pixel. So e.g. a 1440x576 mode in our CEA mode list is actually a
720x576 mode.
In a way this is similar to the interlaced stuff, where the frame that
logically gets send out is not the same that actually goes through the
wire. To be consistent with how we handle interlaced modes, we want
only half the horizontal timings of what the CEA tables list (which
then also properly tells userspace how big the scanned-out framebuffer
actually is).
To avoid digressing from the CEA table, fix up the horizontal timings
in the cea parsing code and not in the table. Wrt the pixel clock I've
opted to keep it as-is - in interlaced modes we also specify the
pixelclock on the wire. Furthermore this makes checking for bandwidth
constraints simpler.
Similarly to interlaced modes it could be useful to add a doubleclock
flag to drm_mode_set_crtcinfo to specify whether we want double
clocked mode timings specified in doubled-up pixels or not. But
because only the intel driver bothers atm to correctly implement these
modes I've figured adding dead code for potential future use is not
worth it.
Because the mode names are used in userspace tools, these need to be
adjusted, too. But I've left the comments as-is to make comparing with
the CEA spec easier.
Note also that this completely breaks these modes on intel, but that
will be fixed in the next patch to correctly handle double-clocked
modes.
v2: Fix English fail in the commit message
Signed-Off-by: Daniel Vetter
---
drivers/gpu/drm/drm_edid.c | 19 +++
drivers/gpu/drm/drm_edid_modes.h | 30 +++---
2 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 608bddf..9e22b61 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1467,6 +1467,25 @@ do_cea_modes (struct drm_connector *connector, u8 *db,
u8 len)
struct drm_display_mode *newmode;
newmode = drm_mode_duplicate(dev,
&edid_cea_modes[cea_mode]);
+
+ /*
+* CEA specifies horizontal timings for double-clocked
+* modes in doubled pixels. In drm, we want actual
+* pixels (for consistency with other modes where a
+* framebuffer pixel is not equal to a scanned-out
+* pixel, e.g. interlaced modes).
+*
+* To keep the table in-sync with the CEA spec, adjust
+* horizontal timings here.
+*/
+ if (newmode->flags & DRM_MODE_FLAG_DBLCLK) {
+ newmode->hdisplay /= 2;
+ newmode->hsync_start /= 2;
+ newmode->hsync_end /= 2;
+ newmode->htotal /= 2;
+ newmode->hskew /= 2;
+ }
+
if (newmode) {
drm_mode_probed_add(connector, newmode);
modes++;
diff --git a/drivers/gpu/drm/drm_edid_modes.h b/drivers/gpu/drm/drm_edid_modes.h
index ff98a7e..9226cbe 100644
--- a/drivers/gpu/drm/drm_edid_modes.h
+++ b/drivers/gpu/drm/drm_edid_modes.h
@@ -511,22 +511,22 @@ static const struct drm_display_mode edid_cea_modes[] = {
DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC |
DRM_MODE_FLAG_INTERLACE) },
/* 6 - 1440x480i@60Hz */
- { DRM_MODE("1440x480", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
+ { DRM_MODE("720x480", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
1602, 1716, 0, 480, 488, 494, 525, 0,
DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK) },
/* 7 - 1440x480i@60Hz */
- { DRM_MODE("1440x480", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
+ { DRM_MODE("720x480", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
1602, 1716, 0, 480, 488, 494, 525, 0,
DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK) },
/* 8 - 1440
[Intel-gfx] [PATCH 1/2] drm/edid: adjust double-clocked cea modes
The CEA has a bunch of very peculiar modes. For backwards
compatibility is specifies a bunch of resulting that are suitable to
display old SD TV content. But these modes have such low pixel clocks
that pixels need to be double to reach the minimal clock of the HDMI
interface.
Paulo Zanoni already started to fix this up by properly marking these
modes as double-clocked and sending the correct avi infoframe as
required by CEA.
The problem left is that the CEA spec specifies these modes with
timings as they get transmitted, but because the double-clocking is
just to make the HDMI phy happy, the TV actually drops every 2nd
pixel. So e.g. a 1440x576 mode in our CEA mode list is actually a
720x576 mode.
In a way this is similar to the interlace stuff, where the frame that
logically gets send out is not the same that actually goes through the
wire. To be consistent with how we handle interlaced modes, we want
only half the horizontal timings of what the CEA tables list (which
also properly tells userspace how big the scanned-out framebuffer
actually is).
To avoid digressing from the CEA table, fix up the horizontal timings
in the cea parsing code and not in the table. Wrt the pixel clock I've
opted to keep it as-is - in interlaced modes we also specify the
pixelclock on the wire. Furthermore this makes checking for bandwidth
constrains simpler.
Similarly to interlaced modes it could be useful to add a doubleclock
flag to drm_mode_set_crtcinfo to specify whether we want
double-clocked mode timings specified in doubled-up pixels or not. But
because only the intel driver bothers atm to correctly implement these
modes I've figured adding dead code for potential future use is not
worth it.
Because the mode names are used in userspace tools, these need to be
adjusted, too. But I've left the comments as-is to make comparing with
the CEA spec easier.
Note also that this completely breaks these modes on intel, but that
will be fixed in the next patch to correctly handle double-clocked
modes.
Signed-Off-by: Daniel Vetter
---
drivers/gpu/drm/drm_edid.c | 19 +++
drivers/gpu/drm/drm_edid_modes.h | 30 +++---
2 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 608bddf..9e22b61 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1467,6 +1467,25 @@ do_cea_modes (struct drm_connector *connector, u8 *db,
u8 len)
struct drm_display_mode *newmode;
newmode = drm_mode_duplicate(dev,
&edid_cea_modes[cea_mode]);
+
+ /*
+* CEA specifies horizontal timings for double-clocked
+* modes in doubled pixels. In drm, we want actual
+* pixels (for consistency with other modes where a
+* framebuffer pixel is not equal to a scanned-out
+* pixel, e.g. interlaced modes).
+*
+* To keep the table in-sync with the CEA spec, adjust
+* horizontal timings here.
+*/
+ if (newmode->flags & DRM_MODE_FLAG_DBLCLK) {
+ newmode->hdisplay /= 2;
+ newmode->hsync_start /= 2;
+ newmode->hsync_end /= 2;
+ newmode->htotal /= 2;
+ newmode->hskew /= 2;
+ }
+
if (newmode) {
drm_mode_probed_add(connector, newmode);
modes++;
diff --git a/drivers/gpu/drm/drm_edid_modes.h b/drivers/gpu/drm/drm_edid_modes.h
index ff98a7e..9226cbe 100644
--- a/drivers/gpu/drm/drm_edid_modes.h
+++ b/drivers/gpu/drm/drm_edid_modes.h
@@ -511,22 +511,22 @@ static const struct drm_display_mode edid_cea_modes[] = {
DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC |
DRM_MODE_FLAG_INTERLACE) },
/* 6 - 1440x480i@60Hz */
- { DRM_MODE("1440x480", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
+ { DRM_MODE("720x480", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
1602, 1716, 0, 480, 488, 494, 525, 0,
DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK) },
/* 7 - 1440x480i@60Hz */
- { DRM_MODE("1440x480", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
+ { DRM_MODE("720x480", DRM_MODE_TYPE_DRIVER, 27000, 1440, 1478,
1602, 1716, 0, 480, 488, 494, 525, 0,
DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC |
DRM_MODE_FLAG_INTERLACE | DRM_MODE_FLAG_DBLCLK) },
/* 8 - 1440x240@60Hz */
- { DRM_MODE("1440x240", DRM_MODE_
