| Message ID | 1379353735-4472-11-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 16, 2013 at 06:48:53PM +0100, Damien Lespiau wrote: > There are a few things to be flushed out if we want to allow multiple > buffers stereo framebuffers: > - What with drm_planes? what semantics do they follow, what is the > hardware able to do with them? > - How do we define which buffer if the right/left one > - Do we allow flips between 1 buffer fbs and 2 buffers fbs (No.) > > So for now, and until I get access to hardware that can do that, let's > just disallow 2 buffers stereo framebuffers to not introduce ABI we > wouldn't be able to change afterwards. > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > --- > drivers/gpu/drm/drm_crtc.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 39f60ec..91d1c4b 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -2131,6 +2131,17 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, > goto out; > } > > + /* > + * Do not allow the use of framebuffers consisting of multiple > + * buffers with stereo modes until all the details API details > + * are fleshed out (eg. interaction with drm_planes, switch > + * between a 1 buffers and a 2 buffers fb, ...) > + */ > + if (fb->num_buffers > 1 && drm_mode_is_stereo(mode)) { > + ret = -EINVAL; > + goto out; > + } This would prevent planar buffers in stereo modes. I'm think we just ignore the matter for now and let drivers deal with it. We don't have enough handles anyway for planar stereo, so maybe we even want to add separate left/right fb attachment properties to the planes instead of tying it up in inside a single fb. Or we cook up addfb3 when we hit this problem for real. I think we'd anyway need some kind of flag for the fb if it contains both left and right buffers. > + > drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V); > > hdisplay = mode->hdisplay; > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Sep 17, 2013 at 11:20:46AM +0300, Ville Syrjälä wrote: > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -2131,6 +2131,17 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, > > goto out; > > } > > > > + /* > > + * Do not allow the use of framebuffers consisting of multiple > > + * buffers with stereo modes until all the details API details > > + * are fleshed out (eg. interaction with drm_planes, switch > > + * between a 1 buffers and a 2 buffers fb, ...) > > + */ > > + if (fb->num_buffers > 1 && drm_mode_is_stereo(mode)) { > > + ret = -EINVAL; > > + goto out; > > + } > > This would prevent planar buffers in stereo modes. I'm think we just > ignore the matter for now and let drivers deal with it. We don't have > enough handles anyway for planar stereo, so maybe we even want to add > separate left/right fb attachment properties to the planes instead of > tying it up in inside a single fb. Or we cook up addfb3 when we hit > this problem for real. I think we'd anyway need some kind of flag for > the fb if it contains both left and right buffers. I'm quite happy to ignore 3 planes YUV stereo fbs for now :) (2 planes YUV stereo fbs still fit!). Are you fine with this test though, or do you mean ignore the whole matter of forbidding this case (or just the multiplane stereo fb case)? I was just thinking that I missed the addition of this check in the page flip ioctl.
On Tue, Sep 17, 2013 at 10:03:12AM +0100, Damien Lespiau wrote: > On Tue, Sep 17, 2013 at 11:20:46AM +0300, Ville Syrjälä wrote: > > +++ b/drivers/gpu/drm/drm_crtc.c > > > @@ -2131,6 +2131,17 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, > > > goto out; > > > } > > > > > > + /* > > > + * Do not allow the use of framebuffers consisting of multiple > > > + * buffers with stereo modes until all the details API details > > > + * are fleshed out (eg. interaction with drm_planes, switch > > > + * between a 1 buffers and a 2 buffers fb, ...) > > > + */ > > > + if (fb->num_buffers > 1 && drm_mode_is_stereo(mode)) { > > > + ret = -EINVAL; > > > + goto out; > > > + } > > > > This would prevent planar buffers in stereo modes. I'm think we just > > ignore the matter for now and let drivers deal with it. We don't have > > enough handles anyway for planar stereo, so maybe we even want to add > > separate left/right fb attachment properties to the planes instead of > > tying it up in inside a single fb. Or we cook up addfb3 when we hit > > this problem for real. I think we'd anyway need some kind of flag for > > the fb if it contains both left and right buffers. > > I'm quite happy to ignore 3 planes YUV stereo fbs for now :) (2 planes > YUV stereo fbs still fit!). > > Are you fine with this test though, or do you mean ignore the whole > matter of forbidding this case (or just the multiplane stereo fb case)? > I was just thinking that I missed the addition of this check in the page > flip ioctl. Yeah, I was thinking we that we can ignore this issue for now, and so we wouldn't need the check. Currently for non-stereo cases the only thing we check is that there is a valid handle for each plane. If userspace passes more handles, we simply ignore the extra ones.
On Tue, Sep 17, 2013 at 12:54:09PM +0300, Ville Syrjälä wrote: > On Tue, Sep 17, 2013 at 10:03:12AM +0100, Damien Lespiau wrote: > > On Tue, Sep 17, 2013 at 11:20:46AM +0300, Ville Syrjälä wrote: > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > > @@ -2131,6 +2131,17 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, > > > > goto out; > > > > } > > > > > > > > + /* > > > > + * Do not allow the use of framebuffers consisting of multiple > > > > + * buffers with stereo modes until all the details API details > > > > + * are fleshed out (eg. interaction with drm_planes, switch > > > > + * between a 1 buffers and a 2 buffers fb, ...) > > > > + */ > > > > + if (fb->num_buffers > 1 && drm_mode_is_stereo(mode)) { > > > > + ret = -EINVAL; > > > > + goto out; > > > > + } > > > > > > This would prevent planar buffers in stereo modes. I'm think we just > > > ignore the matter for now and let drivers deal with it. We don't have > > > enough handles anyway for planar stereo, so maybe we even want to add > > > separate left/right fb attachment properties to the planes instead of > > > tying it up in inside a single fb. Or we cook up addfb3 when we hit > > > this problem for real. I think we'd anyway need some kind of flag for > > > the fb if it contains both left and right buffers. > > > > I'm quite happy to ignore 3 planes YUV stereo fbs for now :) (2 planes > > YUV stereo fbs still fit!). > > > > Are you fine with this test though, or do you mean ignore the whole > > matter of forbidding this case (or just the multiplane stereo fb case)? > > I was just thinking that I missed the addition of this check in the page > > flip ioctl. > > Yeah, I was thinking we that we can ignore this issue for now, and so we > wouldn't need the check. Currently for non-stereo cases the only thing > we check is that there is a valid handle for each plane. If userspace > passes more handles, we simply ignore the extra ones. I guess we should start to check that. For 3d framebuffers with 2 separate buffer handles for each plane I think we need to add another flag to addfb2, e.g. #define DRM_MODE_FB_3D_2_FRAMES (1<<1) /* separate left/right buffers, doubles plane count */ and then also throw in the respective check code into the core that userspace supplies sufficient amounts of buffers in framebuffer_check() by adjusting drM_format_num_planes and drm_format_plane_cpp. -Daniel
On Tue, Sep 17, 2013 at 12:37:48PM +0200, Daniel Vetter wrote: > On Tue, Sep 17, 2013 at 12:54:09PM +0300, Ville Syrjälä wrote: > > On Tue, Sep 17, 2013 at 10:03:12AM +0100, Damien Lespiau wrote: > > > On Tue, Sep 17, 2013 at 11:20:46AM +0300, Ville Syrjälä wrote: > > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > > > @@ -2131,6 +2131,17 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, > > > > > goto out; > > > > > } > > > > > > > > > > + /* > > > > > + * Do not allow the use of framebuffers consisting of multiple > > > > > + * buffers with stereo modes until all the details API details > > > > > + * are fleshed out (eg. interaction with drm_planes, switch > > > > > + * between a 1 buffers and a 2 buffers fb, ...) > > > > > + */ > > > > > + if (fb->num_buffers > 1 && drm_mode_is_stereo(mode)) { > > > > > + ret = -EINVAL; > > > > > + goto out; > > > > > + } > > > > > > > > This would prevent planar buffers in stereo modes. I'm think we just > > > > ignore the matter for now and let drivers deal with it. We don't have > > > > enough handles anyway for planar stereo, so maybe we even want to add > > > > separate left/right fb attachment properties to the planes instead of > > > > tying it up in inside a single fb. Or we cook up addfb3 when we hit > > > > this problem for real. I think we'd anyway need some kind of flag for > > > > the fb if it contains both left and right buffers. > > > > > > I'm quite happy to ignore 3 planes YUV stereo fbs for now :) (2 planes > > > YUV stereo fbs still fit!). > > > > > > Are you fine with this test though, or do you mean ignore the whole > > > matter of forbidding this case (or just the multiplane stereo fb case)? > > > I was just thinking that I missed the addition of this check in the page > > > flip ioctl. > > > > Yeah, I was thinking we that we can ignore this issue for now, and so we > > wouldn't need the check. Currently for non-stereo cases the only thing > > we check is that there is a valid handle for each plane. If userspace > > passes more handles, we simply ignore the extra ones. > > I guess we should start to check that. For 3d framebuffers with 2 > separate buffer handles for each plane I think we need to add another flag > to addfb2, e.g. > > #define DRM_MODE_FB_3D_2_FRAMES (1<<1) /* separate left/right buffers, doubles plane count */ > > and then also throw in the respective check code into the core that > userspace supplies sufficient amounts of buffers in framebuffer_check() > by adjusting drM_format_num_planes and drm_format_plane_cpp. Well, we shouldn't mix the plane vs. buffers/handles/whatever concepts. The number of planes is clearly defined by the pixel format. But yes, we should probably add a drm_format_num_buffers() function to figure out how many buffers are required. But the problem is that addfb2 can't supply more than 4. So we need a new ioctl if we want to collect all that information inside a single drm_fb object. If we do add another ioctl then I think we should go at least to 16, since we might also want to have separate buffers for each field in interlaced framebuffers. And then we need to clearly define which handle is which plane/field/eye. Something like this for example: eye=left field=top plane=0 [plane=1 ...] [field=bottom plane=0 [plane=1 ...]] [eye=right field=top plane=0 [plane=1 ...] [field=bottom plane=0 [plane=1 ...]]] so you first have all the planes for left/top, then all planes for left/bottom, then right/top, and finally right/bottom.
On Tue, Sep 17, 2013 at 12:37:48PM +0200, Daniel Vetter wrote: > I guess we should start to check that. For 3d framebuffers with 2 > separate buffer handles for each plane I think we need to add another flag > to addfb2, e.g. > > #define DRM_MODE_FB_3D_2_FRAMES (1<<1) /* separate left/right buffers, doubles plane count */ > > and then also throw in the respective check code into the core that > userspace supplies sufficient amounts of buffers in framebuffer_check() > by adjusting drM_format_num_planes and drm_format_plane_cpp. Would we really need that flag? we can just count the number of buffers and decide from that number whether we're in the 1 or 2 buffer(s) case?
On Tue, Sep 17, 2013 at 02:20:41PM +0100, Damien Lespiau wrote: > On Tue, Sep 17, 2013 at 12:37:48PM +0200, Daniel Vetter wrote: > > I guess we should start to check that. For 3d framebuffers with 2 > > separate buffer handles for each plane I think we need to add another flag > > to addfb2, e.g. > > > > #define DRM_MODE_FB_3D_2_FRAMES (1<<1) /* separate left/right buffers, doubles plane count */ > > > > and then also throw in the respective check code into the core that > > userspace supplies sufficient amounts of buffers in framebuffer_check() > > by adjusting drM_format_num_planes and drm_format_plane_cpp. > > Would we really need that flag? we can just count the number of buffers > and decide from that number whether we're in the 1 or 2 buffer(s) case? I'd go with explicit flags. We have one for interlaced already. Although ATM the interlaced flag must mean that the fields are interleaved in the same buffer. But if we want separate buffers for each field, we need a flag for that too. Or we just pretend the old interlace flag doesn't exist yet and steal it for that purpose. I don't think anyone is using the flag yet. But anyway we'd need a new ioctl :(
On Tue, Sep 17, 2013 at 3:20 PM, Damien Lespiau <damien.lespiau@intel.com> wrote: > On Tue, Sep 17, 2013 at 12:37:48PM +0200, Daniel Vetter wrote: >> I guess we should start to check that. For 3d framebuffers with 2 >> separate buffer handles for each plane I think we need to add another flag >> to addfb2, e.g. >> >> #define DRM_MODE_FB_3D_2_FRAMES (1<<1) /* separate left/right buffers, doubles plane count */ >> >> and then also throw in the respective check code into the core that >> userspace supplies sufficient amounts of buffers in framebuffer_check() >> by adjusting drM_format_num_planes and drm_format_plane_cpp. > > Would we really need that flag? we can just count the number of buffers > and decide from that number whether we're in the 1 or 2 buffer(s) case? We do not know at addfb2 time that it'll be a 3d mode buffer, and drivers might want to know that to internally assign the buffers to left/right buffer handle pointers. Similar to how we already have a flag for when the buffer is interlaced, which atm we don't support (since we only support scan-out of progressive buffers to interlaced modes, not interlaced to interlaced). So imo a flag here makes sense so that the driver can properly check constraints and make sense of the handles. -Daniel
On Tue, Sep 17, 2013 at 1:02 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > But the problem is that addfb2 can't supply more than 4. So we need a new > ioctl if we want to collect all that information inside a single drm_fb > object. If we do add another ioctl then I think we should go at least to > 16, since we might also want to have separate buffers for each field in > interlaced framebuffers. And then we need to clearly define which handle > is which plane/field/eye. Something like this for example: Blergh, I've thought we have gone with at least 6 buffers for the 3d case. Meh :( -Daniel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 39f60ec..91d1c4b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2131,6 +2131,17 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, goto out; } + /* + * Do not allow the use of framebuffers consisting of multiple + * buffers with stereo modes until all the details API details + * are fleshed out (eg. interaction with drm_planes, switch + * between a 1 buffers and a 2 buffers fb, ...) + */ + if (fb->num_buffers > 1 && drm_mode_is_stereo(mode)) { + ret = -EINVAL; + goto out; + } + drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V); hdisplay = mode->hdisplay;
There are a few things to be flushed out if we want to allow multiple buffers stereo framebuffers: - What with drm_planes? what semantics do they follow, what is the hardware able to do with them? - How do we define which buffer if the right/left one - Do we allow flips between 1 buffer fbs and 2 buffers fbs (No.) So for now, and until I get access to hardware that can do that, let's just disallow 2 buffers stereo framebuffers to not introduce ABI we wouldn't be able to change afterwards. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- drivers/gpu/drm/drm_crtc.c | 11 +++++++++++ 1 file changed, 11 insertions(+)