Re: [PATCH 7/9] drm/edid: Expose mandatory stereo modes for HDMI sinks

[Damien Lespiau <damien.lespiau@intel.com>]

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)

-- 
Damien