| Message ID | 1378493845-476-8-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Damien Lespiau <damien.lespiau <at> intel.com> writes: > +static const struct s3d_mandatory_mode s3d_mandatory_modes[] = { > + { 1920, 1080, 24, 0, > + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING }, > + { 1920, 1080, 50, DRM_MODE_FLAG_INTERLACE, > + DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF }, > + { 1920, 1080, 60, DRM_MODE_FLAG_INTERLACE, > + DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF }, > + { 1280, 720, 50, 0, > + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING }, > + { 1280, 720, 60, 0, > + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING } > +}; I may be missing something here... But.. The frame packed modes are much higher in pixels than this and include frame packing. 1080*2+45=2050 720*2+30=1470 Unless you intend to hide the left/right split in mesa or other place, we need to get the ability to render to both fields somehow. Either as the full 2050 pixels high or at 1080*2 and the driver adds the blanking. Also, some logic aught to indicate pixel aspect ratio for the modes since they are non square for the half res modes. /Joakim
On Fri, Sep 13, 2013 at 6:10 PM, Joakim Plate <elupus@ecce.se> wrote: > > Also, some logic aught to indicate pixel aspect ratio for the modes since > they are non square for the half res modes. Atm we completely ignore pixel aspect ratio, also for flatworld CEA modes. So I don't think we need to concer ourselves here about this, imo pixel aspect ratio support is orthogonal. -Daniel
On Fri, Sep 13, 2013 at 04:10:24PM +0000, Joakim Plate wrote: > Damien Lespiau <damien.lespiau <at> intel.com> writes: > > > +static const struct s3d_mandatory_mode s3d_mandatory_modes[] = { > > + { 1920, 1080, 24, 0, > > + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING > }, > > + { 1920, 1080, 50, DRM_MODE_FLAG_INTERLACE, > > + DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF }, > > + { 1920, 1080, 60, DRM_MODE_FLAG_INTERLACE, > > + DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF }, > > + { 1280, 720, 50, 0, > > + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING > }, > > + { 1280, 720, 60, 0, > > + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING } > > +}; > > > I may be missing something here... But.. Oops, did not see your answer, please don't forget to include me in Cc: next time and not just the list. > The frame packed modes are much higher in pixels than this and include frame > packing. > 1080*2+45=2050 > 720*2+30=1470 > > Unless you intend to hide the left/right split in mesa or other place, we > need to get the ability to render to both fields somehow. > > Either as the full 2050 pixels high or at 1080*2 and the driver adds the > blanking. Right, so at the moment, my proposition is that userspace is responsible for giving us a framebuffer with the right dimensions. For instance in intel-gpu-tools's testdisplay I have: struct box { int x, y, width, height; }; struct stereo_fb_layout { int fb_width, fb_height; struct box left, right; }; static void stereo_fb_layout_from_mode(struct stereo_fb_layout *layout, drmModeModeInfo *mode) { unsigned int format = mode->flags & DRMTEST_MODE_FLAG_3D_MASK; const int hdisplay = mode->hdisplay, vdisplay = mode->vdisplay; switch (format) { [...] case DRM_MODE_FLAG_3D_FRAME_PACKING: { int vactive_space = mode->vtotal - vdisplay; layout->fb_width = hdisplay; layout->fb_height = 2 * vdisplay + vactive_space; box_init(&layout->left, 0, 0, hdisplay, vdisplay); box_init(&layout->right, 0, vdisplay + vactive_space, hdisplay, vdisplay); break; } and then adjust the timings if needed: static void adjust_stereo_timings(drmModeModeInfo *mode) { unsigned int layout = mode->flags & DRMTEST_MODE_FLAG_3D_MASK; uint16_t vdisplay, vactive_space; switch (layout) { case DRM_MODE_FLAG_3D_TOP_AND_BOTTOM: case DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF: return; case DRM_MODE_FLAG_3D_FRAME_PACKING: vactive_space = mode->vtotal - mode->vdisplay; vdisplay = mode->vdisplay; mode->vdisplay += vdisplay + vactive_space; mode->vsync_start += vdisplay + vactive_space; mode->vsync_end += vdisplay + vactive_space; mode->vtotal += vdisplay + vactive_space; mode->clock = (mode->vtotal * mode->htotal * mode->vrefresh) / 1000; return; [...] I think it makes quite a bit of sense to have the "underlying 2D mode" in the mode structure as this 2d mode is relevant to the 3d mode: - HDMI stereo modes are defined based on the unerdlying 2D mode. (eg the extra, non-mandatory, modes in the EDID have their definitions pointing to CEA 2D VICs) - HDMI VIC infoframe: one needs to indicate the CEA VIC of this underlying 2d mode when setting the stereo mode. Note that in the future, we also want to allow framebuffers with 2 distinct buffers for right and left. > Also, some logic aught to indicate pixel aspect ratio for the modes since > they are non square for the half res modes. As Daniel already answered, aspect ration in CEA modes is an orthogonal issue (that we want to sort out as well sooner rather than later)
On Mon, Sep 16, 2013 at 06:35:12PM +0100, Damien Lespiau wrote: > On Fri, Sep 13, 2013 at 04:10:24PM +0000, Joakim Plate wrote: > > Damien Lespiau <damien.lespiau <at> intel.com> writes: > > > > > +static const struct s3d_mandatory_mode s3d_mandatory_modes[] = { > > > + { 1920, 1080, 24, 0, > > > + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING > > }, > > > + { 1920, 1080, 50, DRM_MODE_FLAG_INTERLACE, > > > + DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF }, > > > + { 1920, 1080, 60, DRM_MODE_FLAG_INTERLACE, > > > + DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF }, > > > + { 1280, 720, 50, 0, > > > + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING > > }, > > > + { 1280, 720, 60, 0, > > > + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING } > > > +}; > > > > > > I may be missing something here... But.. > > Oops, did not see your answer, please don't forget to include me in Cc: > next time and not just the list. > > > The frame packed modes are much higher in pixels than this and include frame > > packing. > > 1080*2+45=2050 > > 720*2+30=1470 > > > > Unless you intend to hide the left/right split in mesa or other place, we > > need to get the ability to render to both fields somehow. > > > > Either as the full 2050 pixels high or at 1080*2 and the driver adds the > > blanking. > > Right, so at the moment, my proposition is that userspace is responsible for > giving us a framebuffer with the right dimensions. For instance in > intel-gpu-tools's testdisplay I have: > > struct box { > int x, y, width, height; > }; > > struct stereo_fb_layout { > int fb_width, fb_height; > struct box left, right; > }; > > static void stereo_fb_layout_from_mode(struct stereo_fb_layout *layout, > drmModeModeInfo *mode) > { > unsigned int format = mode->flags & DRMTEST_MODE_FLAG_3D_MASK; > const int hdisplay = mode->hdisplay, vdisplay = mode->vdisplay; > > switch (format) { > > [...] > > case DRM_MODE_FLAG_3D_FRAME_PACKING: > { > int vactive_space = mode->vtotal - vdisplay; > > layout->fb_width = hdisplay; > layout->fb_height = 2 * vdisplay + vactive_space; > > box_init(&layout->left, > 0, 0, hdisplay, vdisplay); > box_init(&layout->right, > 0, vdisplay + vactive_space, hdisplay, vdisplay); > break; > } > > and then adjust the timings if needed: > > static void adjust_stereo_timings(drmModeModeInfo *mode) > { > unsigned int layout = mode->flags & DRMTEST_MODE_FLAG_3D_MASK; > uint16_t vdisplay, vactive_space; > > switch (layout) { > case DRM_MODE_FLAG_3D_TOP_AND_BOTTOM: > case DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF: > return; > case DRM_MODE_FLAG_3D_FRAME_PACKING: > vactive_space = mode->vtotal - mode->vdisplay; > vdisplay = mode->vdisplay; > > mode->vdisplay += vdisplay + vactive_space; > mode->vsync_start += vdisplay + vactive_space; > mode->vsync_end += vdisplay + vactive_space; > mode->vtotal += vdisplay + vactive_space; > mode->clock = (mode->vtotal * mode->htotal * mode->vrefresh) / > 1000; I'm wondering if we should take in the 2D mode timings and do this adjustment in the kernel instead. Not quite sure what the pros/cons are for doing it in userland. Oh and now that vactive is actually part of the framebuffer as well, we need to be more careful in the kernel how we adjust the mode. I can't recall if we have special hardware needs wrt. the vertical timings, but if we do we should probably review them all to avoid adjustments that would cause issues with stereo modes.
On Mon, Sep 16, 2013 at 7:35 PM, Damien Lespiau <damien.lespiau@intel.com> wrote: > I think it makes quite a bit of sense to have the "underlying 2D mode" in the > mode structure as this 2d mode is relevant to the 3d mode: > - HDMI stereo modes are defined based on the unerdlying 2D mode. (eg the > extra, non-mandatory, modes in the EDID have their definitions pointing to > CEA 2D VICs) > - HDMI VIC infoframe: one needs to indicate the CEA VIC of this underlying 2d > mode when setting the stereo mode. > > Note that in the future, we also want to allow framebuffers with 2 distinct > buffers for right and left. I think this is the right approach. To extract a bit of common code we could add a bunch more flags to drm_mode_set_crtcinfo so that drivers don't need to compute the blow-up 3d modes (including the blank in between the 2 left/right frames) themselves. Cheers, Daniel
On Mon, Sep 16, 2013 at 7:56 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > Oh and now that vactive is actually part of the framebuffer as well, we > need to be more careful in the kernel how we adjust the mode. I can't > recall if we have special hardware needs wrt. the vertical timings, but > if we do we should probably review them all to avoid adjustments that > would cause issues with stereo modes. I think we need to either adjust the entire mode before handing it to userspace or if that's not possible we need to reject a modeset if the implicit vactive/vblank in the side-by-side framebuffer doesn't fit. On X even that shouldn't be an issue since the ddx could just duplicate this knowledge, more fun would be wayland or similar generic display servers. I'd opt to solve this when it's a real problem though and not before. -Daniel
On Mon, Sep 16, 2013 at 06:35:12PM +0100, Damien Lespiau wrote: > On Fri, Sep 13, 2013 at 04:10:24PM +0000, Joakim Plate wrote: > > Damien Lespiau <damien.lespiau <at> intel.com> writes: > > > > > +static const struct s3d_mandatory_mode s3d_mandatory_modes[] = { > > > + { 1920, 1080, 24, 0, > > > + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING > > }, > > > + { 1920, 1080, 50, DRM_MODE_FLAG_INTERLACE, > > > + DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF }, > > > + { 1920, 1080, 60, DRM_MODE_FLAG_INTERLACE, > > > + DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF }, > > > + { 1280, 720, 50, 0, > > > + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING > > }, > > > + { 1280, 720, 60, 0, > > > + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING } > > > +}; > > > > > > I may be missing something here... But.. > > Oops, did not see your answer, please don't forget to include me in Cc: > next time and not just the list. > > > The frame packed modes are much higher in pixels than this and include frame > > packing. > > 1080*2+45=2050 > > 720*2+30=1470 > > > > Unless you intend to hide the left/right split in mesa or other place, we > > need to get the ability to render to both fields somehow. > > > > Either as the full 2050 pixels high or at 1080*2 and the driver adds the > > blanking. > > Right, so at the moment, my proposition is that userspace is responsible for > giving us a framebuffer with the right dimensions. For instance in > intel-gpu-tools's testdisplay I have: [...] > and then adjust the timings if needed: So, actually it seems that this will change a bit. User space still needs to compute a correct fb size. In the case of frame packing, note that user-space also needs to add the vblank lines, because it has to know where to place the second eye starting at vdisplay + vblank. The kernel will be in charge of tweaking the timings if needed though, see: http://lists.freedesktop.org/archives/dri-devel/2013-September/045386.html
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a207cc3..9d9881b 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2550,13 +2550,60 @@ do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len) return modes; } +struct s3d_mandatory_mode { + int width, height, freq; + unsigned int interlace_flag, formats; +}; + +static const struct s3d_mandatory_mode s3d_mandatory_modes[] = { + { 1920, 1080, 24, 0, + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING }, + { 1920, 1080, 50, DRM_MODE_FLAG_INTERLACE, + DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF }, + { 1920, 1080, 60, DRM_MODE_FLAG_INTERLACE, + DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF }, + { 1280, 720, 50, 0, + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING }, + { 1280, 720, 60, 0, + DRM_MODE_FLAG_3D_TOP_AND_BOTTOM | DRM_MODE_FLAG_3D_FRAME_PACKING } +}; + +static bool match_s3d_mandatory_mode(const struct drm_display_mode *mode, + const struct s3d_mandatory_mode *s3d_mode) +{ + unsigned int interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE; + + return mode->hdisplay == s3d_mode->width && + mode->vdisplay == s3d_mode->height && + interlaced == s3d_mode->interlace_flag && + drm_mode_vrefresh(mode) == s3d_mode->freq; +} + +static void hdmi_patch_stereo_mode(struct drm_display_mode *mode) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(s3d_mandatory_modes); i++) + if (match_s3d_mandatory_mode(mode, &s3d_mandatory_modes[i])) + mode->flags |= s3d_mandatory_modes[i].formats; +} + +static void hdmi_patch_stereo_modes(struct drm_connector *connector) +{ + struct drm_display_mode *mode; + + list_for_each_entry(mode, &connector->probed_modes, head) + hdmi_patch_stereo_mode(mode); +} + /* * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block * @connector: connector corresponding to the HDMI sink * @db: start of the CEA vendor specific block * @len: length of the CEA block payload, ie. one can access up to db[len] * - * Parses the HDMI VSDB looking for modes to add to @connector. + * Parses the HDMI VSDB looking for modes to add to @connector. This function + * also adds the stereo 3d flags to already added modes. */ static int do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len) @@ -2582,10 +2629,15 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len) /* the declared length is not long enough for the 2 first bytes * of additional video format capabilities */ - offset += 2; - if (len < (8 + offset)) + if (len < (8 + offset + 2)) goto out; + /* 3D_Present */ + offset++; + if (db[8 + offset] & (1 << 7)) + hdmi_patch_stereo_modes(connector); + + offset++; vic_len = db[8 + offset] >> 5; for (i = 0; i < vic_len && len >= (9 + offset + i); i++) { @@ -2665,8 +2717,8 @@ static int add_cea_modes(struct drm_connector *connector, struct edid *edid) { const u8 *cea = drm_find_cea_extension(edid); - const u8 *db; - u8 dbl; + const u8 *db, *hdmi = NULL; + u8 dbl, hdmi_len; int modes = 0; if (cea && cea_revision(cea) >= 3) { @@ -2681,11 +2733,20 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid) if (cea_db_tag(db) == VIDEO_BLOCK) modes += do_cea_modes(connector, db + 1, dbl); - else if (cea_db_is_hdmi_vsdb(db)) - modes += do_hdmi_vsdb_modes(connector, db, dbl); + else if (cea_db_is_hdmi_vsdb(db)) { + hdmi = db; + hdmi_len = dbl; + } } } + /* + * We parse the HDMI VSDB after having added the cea modes as we will + * be patching their flags when the sink supports stereo 3D. + */ + if (hdmi) + modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len); + return modes; }
For now, let's just look at the 3D_present flag of the CEA HDMI vendor block to detect if the sink supports a small list of then mandatory 3D formats. See the HDMI 1.4a 3D extraction for detail: http://www.hdmi.org/manufacturer/specification.aspx Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- drivers/gpu/drm/drm_edid.c | 75 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 68 insertions(+), 7 deletions(-)