* Check the fb size against the correct CRTC viewport for stereo modes
@ 2013-09-23 14:57 Damien Lespiau
2013-09-23 14:57 ` [PATCH 1/2] drm: Factor out common CRTC viewport checking code Damien Lespiau
2013-09-23 14:57 ` [PATCH 2/2] drm: Check the fb size against the adjusted v/hdisplay for stereo modes Damien Lespiau
0 siblings, 2 replies; 5+ messages in thread
From: Damien Lespiau @ 2013-09-23 14:57 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
I've sent this 2 patches separately from the main stereo series so we can have
the usual rounds of review without having to resend the full 20+ patches. Once
that last bit has been reviewed as well, I'll send a consolidated series that
will be a serious candidate for merging.
--
Damien
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] drm: Factor out common CRTC viewport checking code
2013-09-23 14:57 Check the fb size against the correct CRTC viewport for stereo modes Damien Lespiau
@ 2013-09-23 14:57 ` Damien Lespiau
2013-09-23 14:57 ` [PATCH 2/2] drm: Check the fb size against the adjusted v/hdisplay for stereo modes Damien Lespiau
1 sibling, 0 replies; 5+ messages in thread
From: Damien Lespiau @ 2013-09-23 14:57 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
Both setcrtc and page_flip are checking that the framebuffer is big
enough for the defined crtc viewport (x, y, hdisplay, vdisplay). Factor
that code out in a single function.
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
drivers/gpu/drm/drm_crtc.c | 70 ++++++++++++++++++++++++----------------------
1 file changed, 37 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 090415f..db05864 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2063,6 +2063,37 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
}
EXPORT_SYMBOL(drm_mode_set_config_internal);
+/*
+ * Checks that the framebuffer is big enough for the CRTC viewport
+ * (x, y, hdisplay, vdisplay)
+ */
+static int drm_crtc_check_viewport(const struct drm_crtc *crtc,
+ int x, int y,
+ const struct drm_display_mode *mode,
+ const struct drm_framebuffer *fb)
+
+{
+ int hdisplay, vdisplay;
+
+ hdisplay = mode->hdisplay;
+ vdisplay = mode->vdisplay;
+
+ if (crtc->invert_dimensions)
+ swap(hdisplay, vdisplay);
+
+ if (hdisplay > fb->width ||
+ vdisplay > fb->height ||
+ x > fb->width - hdisplay ||
+ y > fb->height - vdisplay) {
+ DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
+ fb->width, fb->height, hdisplay, vdisplay, x, y,
+ crtc->invert_dimensions ? " (inverted)" : "");
+ return -ENOSPC;
+ }
+
+ return 0;
+}
+
/**
* drm_mode_setcrtc - set CRTC configuration
* @dev: drm device for the ioctl
@@ -2110,7 +2141,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
if (crtc_req->mode_valid) {
- int hdisplay, vdisplay;
/* If we have a mode we need a framebuffer. */
/* If we pass -1, set the mode with the currently bound fb */
if (crtc_req->fb_id == -1) {
@@ -2146,23 +2176,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
- hdisplay = mode->hdisplay;
- vdisplay = mode->vdisplay;
-
- if (crtc->invert_dimensions)
- swap(hdisplay, vdisplay);
-
- if (hdisplay > fb->width ||
- vdisplay > fb->height ||
- crtc_req->x > fb->width - hdisplay ||
- crtc_req->y > fb->height - vdisplay) {
- DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
- fb->width, fb->height,
- hdisplay, vdisplay, crtc_req->x, crtc_req->y,
- crtc->invert_dimensions ? " (inverted)" : "");
- ret = -ENOSPC;
+ ret = drm_crtc_check_viewport(crtc, crtc_req->x, crtc_req->y,
+ mode, fb);
+ if (ret)
goto out;
- }
+
}
if (crtc_req->count_connectors == 0 && mode) {
@@ -3579,7 +3597,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
struct drm_framebuffer *fb = NULL, *old_fb = NULL;
struct drm_pending_vblank_event *e = NULL;
unsigned long flags;
- int hdisplay, vdisplay;
int ret = -EINVAL;
if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
@@ -3611,22 +3628,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
if (!fb)
goto out;
- hdisplay = crtc->mode.hdisplay;
- vdisplay = crtc->mode.vdisplay;
-
- if (crtc->invert_dimensions)
- swap(hdisplay, vdisplay);
-
- if (hdisplay > fb->width ||
- vdisplay > fb->height ||
- crtc->x > fb->width - hdisplay ||
- crtc->y > fb->height - vdisplay) {
- DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
- fb->width, fb->height, hdisplay, vdisplay, crtc->x, crtc->y,
- crtc->invert_dimensions ? " (inverted)" : "");
- ret = -ENOSPC;
+ ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
+ if (ret)
goto out;
- }
if (crtc->fb->pixel_format != fb->pixel_format) {
DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] drm: Check the fb size against the adjusted v/hdisplay for stereo modes
2013-09-23 14:57 Check the fb size against the correct CRTC viewport for stereo modes Damien Lespiau
2013-09-23 14:57 ` [PATCH 1/2] drm: Factor out common CRTC viewport checking code Damien Lespiau
@ 2013-09-23 14:57 ` Damien Lespiau
2013-09-23 18:39 ` [Intel-gfx] " Ville Syrjälä
1 sibling, 1 reply; 5+ messages in thread
From: Damien Lespiau @ 2013-09-23 14:57 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
Some stereo modes, like frame packing, need a larger CRTC viewport than
the "natural" underlying 2D mode and thus drm_crtc_check_viewport()
needs to query the adjusted mode to use the correct h/vdisplay.
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
drivers/gpu/drm/drm_crtc.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index db05864..807309f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2078,6 +2078,14 @@ static int drm_crtc_check_viewport(const struct drm_crtc *crtc,
hdisplay = mode->hdisplay;
vdisplay = mode->vdisplay;
+ if (drm_mode_is_stereo(mode)) {
+ struct drm_display_mode adjusted = *mode;
+
+ drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
+ hdisplay = adjusted.crtc_hdisplay;
+ vdisplay = adjusted.crtc_vdisplay;
+ }
+
if (crtc->invert_dimensions)
swap(hdisplay, vdisplay);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] drm: Check the fb size against the adjusted v/hdisplay for stereo modes
2013-09-23 14:57 ` [PATCH 2/2] drm: Check the fb size against the adjusted v/hdisplay for stereo modes Damien Lespiau
@ 2013-09-23 18:39 ` Ville Syrjälä
2013-09-24 9:18 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Ville Syrjälä @ 2013-09-23 18:39 UTC (permalink / raw)
To: Damien Lespiau; +Cc: intel-gfx, dri-devel
On Mon, Sep 23, 2013 at 03:57:40PM +0100, Damien Lespiau wrote:
> Some stereo modes, like frame packing, need a larger CRTC viewport than
> the "natural" underlying 2D mode and thus drm_crtc_check_viewport()
> needs to query the adjusted mode to use the correct h/vdisplay.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
> drivers/gpu/drm/drm_crtc.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index db05864..807309f 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2078,6 +2078,14 @@ static int drm_crtc_check_viewport(const struct drm_crtc *crtc,
> hdisplay = mode->hdisplay;
> vdisplay = mode->vdisplay;
>
> + if (drm_mode_is_stereo(mode)) {
> + struct drm_display_mode adjusted = *mode;
> +
> + drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
> + hdisplay = adjusted.crtc_hdisplay;
> + vdisplay = adjusted.crtc_vdisplay;
> + }
Yeah, I think this is as good as we can do in generic code. Normally
the driver is free to adjust the non-visible portion of the timings,
but with frame packing the non-visible part also affects the viewport
size, so drivers supporting frame packing simply have to be more
careful how they adjust the timings, or they need to do another check
after the adjustments are done.
For the series:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> +
> if (crtc->invert_dimensions)
> swap(hdisplay, vdisplay);
>
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] drm: Check the fb size against the adjusted v/hdisplay for stereo modes
2013-09-23 18:39 ` [Intel-gfx] " Ville Syrjälä
@ 2013-09-24 9:18 ` Daniel Vetter
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2013-09-24 9:18 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, dri-devel
On Mon, Sep 23, 2013 at 09:39:28PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 23, 2013 at 03:57:40PM +0100, Damien Lespiau wrote:
> > Some stereo modes, like frame packing, need a larger CRTC viewport than
> > the "natural" underlying 2D mode and thus drm_crtc_check_viewport()
> > needs to query the adjusted mode to use the correct h/vdisplay.
> >
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> > drivers/gpu/drm/drm_crtc.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index db05864..807309f 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -2078,6 +2078,14 @@ static int drm_crtc_check_viewport(const struct drm_crtc *crtc,
> > hdisplay = mode->hdisplay;
> > vdisplay = mode->vdisplay;
> >
> > + if (drm_mode_is_stereo(mode)) {
> > + struct drm_display_mode adjusted = *mode;
> > +
> > + drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
> > + hdisplay = adjusted.crtc_hdisplay;
> > + vdisplay = adjusted.crtc_vdisplay;
> > + }
>
> Yeah, I think this is as good as we can do in generic code. Normally
> the driver is free to adjust the non-visible portion of the timings,
> but with frame packing the non-visible part also affects the viewport
> size, so drivers supporting frame packing simply have to be more
> careful how they adjust the timings, or they need to do another check
> after the adjustments are done.
>
> For the series:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Depending upon how the hw handles separate left/right framebuffers this
might be the wrong thing to do: If the hw wants double-up timings, but two
framebuffer pointers then we'd check the wrong thing.
But since this is all internal we can easily replace the mode argument
with an explicit h/vdisplay for drm_crtc_check_viewport and shovel the
handling down into drivers. So I think we can postpone this until we have
the separate left/right buffer stuff working.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-24 9:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-23 14:57 Check the fb size against the correct CRTC viewport for stereo modes Damien Lespiau
2013-09-23 14:57 ` [PATCH 1/2] drm: Factor out common CRTC viewport checking code Damien Lespiau
2013-09-23 14:57 ` [PATCH 2/2] drm: Check the fb size against the adjusted v/hdisplay for stereo modes Damien Lespiau
2013-09-23 18:39 ` [Intel-gfx] " Ville Syrjälä
2013-09-24 9:18 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).