| Message ID | 20180511145919.22447-3-boris.brezillon@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote: > Applying an underscan setup is just a matter of scaling all planes > appropriately and adjusting the CRTC X/Y offset to account for the > horizontal and vertical border. > > Create an vc4_plane_underscan_adj() function doing that and call it from > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach > underscan properties to the HDMI connector. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > Changes in v2: > - Take changes on hborder/vborder meaning into account > --- > drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 48 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c > index 71d44c357d35..61ed60841cd6 100644 > --- a/drivers/gpu/drm/vc4/vc4_plane.c > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane) > } > } > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate) > +{ > + struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate); > + struct drm_connector_state *conn_state = NULL; > + struct drm_connector *conn; > + struct drm_crtc_state *crtc_state; > + int i; > + > + for_each_new_connector_in_state(pstate->state, conn, conn_state, i) { > + if (conn_state->crtc == pstate->crtc) > + break; > + } > + > + if (i == pstate->state->num_connector) > + return 0; > + > + if (conn_state->underscan.mode != DRM_UNDERSCAN_ON) > + return 0; > + > + crtc_state = drm_atomic_get_new_crtc_state(pstate->state, > + pstate->crtc); > + > + if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay || > + conn_state->underscan.vborder >= crtc_state->mode.vdisplay) > + return -EINVAL; border * 2 ? > + > + vc4_pstate->crtc_x += conn_state->underscan.hborder; > + vc4_pstate->crtc_y += conn_state->underscan.vborder; > + vc4_pstate->crtc_w = (vc4_pstate->crtc_w * > + (crtc_state->mode.hdisplay - > + (conn_state->underscan.hborder * 2))) / > + crtc_state->mode.hdisplay; > + vc4_pstate->crtc_h = (vc4_pstate->crtc_h * > + (crtc_state->mode.vdisplay - > + (conn_state->underscan.vborder * 2))) / > + crtc_state->mode.vdisplay; So you're now scaling all planes? The code seems to reject scaling for the cursor plane, how are you dealing with that? Or just no cursor allowed when underscanning? I'm also wondering if there's any way we can reconcile these border props with the scaling mode prop, should we ever wish to expose these props on connectors that already have the scaling mode prop. Maybe we just have to require the scaling mode to be set to fullscreen to allow borders to be specified explictly? > + > + if (!vc4_pstate->crtc_w || !vc4_pstate->crtc_h) > + return -EINVAL; > + > + return 0; > +} > + > static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) > { > struct drm_plane *plane = state->plane; > @@ -269,7 +312,7 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) > int num_planes = fb->format->num_planes; > u32 h_subsample = 1; > u32 v_subsample = 1; > - int i; > + int i, ret; > > for (i = 0; i < num_planes; i++) > vc4_state->offsets[i] = bo->paddr + fb->offsets[i]; > @@ -292,6 +335,10 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) > vc4_state->crtc_w = state->crtc_w; > vc4_state->crtc_h = state->crtc_h; > > + ret = vc4_plane_underscan_adj(state); > + if (ret) > + return ret; > + > vc4_state->x_scaling[0] = vc4_get_scaling_mode(vc4_state->src_w[0], > vc4_state->crtc_w); > vc4_state->y_scaling[0] = vc4_get_scaling_mode(vc4_state->src_h[0], > -- > 2.14.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, 11 May 2018 18:34:50 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote: > > Applying an underscan setup is just a matter of scaling all planes > > appropriately and adjusting the CRTC X/Y offset to account for the > > horizontal and vertical border. > > > > Create an vc4_plane_underscan_adj() function doing that and call it from > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach > > underscan properties to the HDMI connector. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > --- > > Changes in v2: > > - Take changes on hborder/vborder meaning into account > > --- > > drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 48 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c > > index 71d44c357d35..61ed60841cd6 100644 > > --- a/drivers/gpu/drm/vc4/vc4_plane.c > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane) > > } > > } > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate) > > +{ > > + struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate); > > + struct drm_connector_state *conn_state = NULL; > > + struct drm_connector *conn; > > + struct drm_crtc_state *crtc_state; > > + int i; > > + > > + for_each_new_connector_in_state(pstate->state, conn, conn_state, i) { > > + if (conn_state->crtc == pstate->crtc) > > + break; > > + } > > + > > + if (i == pstate->state->num_connector) > > + return 0; > > + > > + if (conn_state->underscan.mode != DRM_UNDERSCAN_ON) > > + return 0; > > + > > + crtc_state = drm_atomic_get_new_crtc_state(pstate->state, > > + pstate->crtc); > > + > > + if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay || > > + conn_state->underscan.vborder >= crtc_state->mode.vdisplay) > > + return -EINVAL; > > border * 2 ? Oops, indeed. I'll fix that. > > > + > > + vc4_pstate->crtc_x += conn_state->underscan.hborder; > > + vc4_pstate->crtc_y += conn_state->underscan.vborder; > > + vc4_pstate->crtc_w = (vc4_pstate->crtc_w * > > + (crtc_state->mode.hdisplay - > > + (conn_state->underscan.hborder * 2))) / > > + crtc_state->mode.hdisplay; > > + vc4_pstate->crtc_h = (vc4_pstate->crtc_h * > > + (crtc_state->mode.vdisplay - > > + (conn_state->underscan.vborder * 2))) / > > + crtc_state->mode.vdisplay; > > So you're now scaling all planes? The code seems to reject scaling for > the cursor plane, how are you dealing with that? Or just no cursor > allowed when underscanning? No, I just didn't test with a cursor plane. We should probably avoid scaling the cursor plane and just adjust its position. Eric any opinion on that? > > I'm also wondering if there's any way we can reconcile these border > props with the scaling mode prop, should we ever wish to expose > these props on connectors that already have the scaling mode prop. Nouveau seems to expose both, and I don't see why we couldn't. > Maybe we just have to require the scaling mode to be set to fullscreen > to allow borders to be specified explictly? > > > + > > + if (!vc4_pstate->crtc_w || !vc4_pstate->crtc_h) > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) > > { > > struct drm_plane *plane = state->plane; > > @@ -269,7 +312,7 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) > > int num_planes = fb->format->num_planes; > > u32 h_subsample = 1; > > u32 v_subsample = 1; > > - int i; > > + int i, ret; > > > > for (i = 0; i < num_planes; i++) > > vc4_state->offsets[i] = bo->paddr + fb->offsets[i]; > > @@ -292,6 +335,10 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) > > vc4_state->crtc_w = state->crtc_w; > > vc4_state->crtc_h = state->crtc_h; > > > > + ret = vc4_plane_underscan_adj(state); > > + if (ret) > > + return ret; > > + > > vc4_state->x_scaling[0] = vc4_get_scaling_mode(vc4_state->src_w[0], > > vc4_state->crtc_w); > > vc4_state->y_scaling[0] = vc4_get_scaling_mode(vc4_state->src_h[0], > > -- > > 2.14.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote: > On Fri, 11 May 2018 18:34:50 +0300 > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote: > > > Applying an underscan setup is just a matter of scaling all planes > > > appropriately and adjusting the CRTC X/Y offset to account for the > > > horizontal and vertical border. > > > > > > Create an vc4_plane_underscan_adj() function doing that and call it from > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach > > > underscan properties to the HDMI connector. > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > --- > > > Changes in v2: > > > - Take changes on hborder/vborder meaning into account > > > --- > > > drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 48 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c > > > index 71d44c357d35..61ed60841cd6 100644 > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane) > > > } > > > } > > > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate) > > > +{ > > > + struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate); > > > + struct drm_connector_state *conn_state = NULL; > > > + struct drm_connector *conn; > > > + struct drm_crtc_state *crtc_state; > > > + int i; > > > + > > > + for_each_new_connector_in_state(pstate->state, conn, conn_state, i) { > > > + if (conn_state->crtc == pstate->crtc) > > > + break; > > > + } > > > + > > > + if (i == pstate->state->num_connector) > > > + return 0; > > > + > > > + if (conn_state->underscan.mode != DRM_UNDERSCAN_ON) > > > + return 0; > > > + > > > + crtc_state = drm_atomic_get_new_crtc_state(pstate->state, > > > + pstate->crtc); > > > + > > > + if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay || > > > + conn_state->underscan.vborder >= crtc_state->mode.vdisplay) > > > + return -EINVAL; > > > > border * 2 ? > > Oops, indeed. I'll fix that. > > > > > > + > > > + vc4_pstate->crtc_x += conn_state->underscan.hborder; > > > + vc4_pstate->crtc_y += conn_state->underscan.vborder; > > > + vc4_pstate->crtc_w = (vc4_pstate->crtc_w * > > > + (crtc_state->mode.hdisplay - > > > + (conn_state->underscan.hborder * 2))) / > > > + crtc_state->mode.hdisplay; > > > + vc4_pstate->crtc_h = (vc4_pstate->crtc_h * > > > + (crtc_state->mode.vdisplay - > > > + (conn_state->underscan.vborder * 2))) / > > > + crtc_state->mode.vdisplay; > > > > So you're now scaling all planes? The code seems to reject scaling for > > the cursor plane, how are you dealing with that? Or just no cursor > > allowed when underscanning? > > No, I just didn't test with a cursor plane. We should probably avoid > scaling the cursor plane and just adjust its position. Eric any opinion > on that? I don't think you can just not scale it. The user asked for the cursor to be at a specific place with a specific size. Can't just ignore that and do something else. Also eg. i915 would definitely scale the cursor since we'd just scale the entire crtc instead of scaling individual planes. Different drivers doing different things wouldn't be good. > > > > > I'm also wondering if there's any way we can reconcile these border > > props with the scaling mode prop, should we ever wish to expose > > these props on connectors that already have the scaling mode prop. > > Nouveau seems to expose both, and I don't see why we couldn't. First of all the interaction of these borders with panels that have a fixed mode would need to be properly specified. Do we apply the borders against the user mode or the panel's native mode? I guess the latter would be a bit easier (would also match how the TV margins behave I suppose). But that does leave open the question of how would generic userspace know which case it's dealing with? Eg. if it wants to underscan by some specific percentage it has to calculate the borders based on the correct mode, otherwise the results aren't going to match the expectations. > > > Maybe we just have to require the scaling mode to be set to fullscreen > > to allow borders to be specified explictly? > > > > > + > > > + if (!vc4_pstate->crtc_w || !vc4_pstate->crtc_h) > > > + return -EINVAL; > > > + > > > + return 0; > > > +} > > > + > > > static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) > > > { > > > struct drm_plane *plane = state->plane; > > > @@ -269,7 +312,7 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) > > > int num_planes = fb->format->num_planes; > > > u32 h_subsample = 1; > > > u32 v_subsample = 1; > > > - int i; > > > + int i, ret; > > > > > > for (i = 0; i < num_planes; i++) > > > vc4_state->offsets[i] = bo->paddr + fb->offsets[i]; > > > @@ -292,6 +335,10 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) > > > vc4_state->crtc_w = state->crtc_w; > > > vc4_state->crtc_h = state->crtc_h; > > > > > > + ret = vc4_plane_underscan_adj(state); > > > + if (ret) > > > + return ret; > > > + > > > vc4_state->x_scaling[0] = vc4_get_scaling_mode(vc4_state->src_w[0], > > > vc4_state->crtc_w); > > > vc4_state->y_scaling[0] = vc4_get_scaling_mode(vc4_state->src_h[0], > > > -- > > > 2.14.1 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > >
On Fri, 11 May 2018 19:54:02 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote: > > On Fri, 11 May 2018 18:34:50 +0300 > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote: > > > > Applying an underscan setup is just a matter of scaling all planes > > > > appropriately and adjusting the CRTC X/Y offset to account for the > > > > horizontal and vertical border. > > > > > > > > Create an vc4_plane_underscan_adj() function doing that and call it from > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach > > > > underscan properties to the HDMI connector. > > > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > > --- > > > > Changes in v2: > > > > - Take changes on hborder/vborder meaning into account > > > > --- > > > > drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 48 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c > > > > index 71d44c357d35..61ed60841cd6 100644 > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane) > > > > } > > > > } > > > > > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate) > > > > +{ > > > > + struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate); > > > > + struct drm_connector_state *conn_state = NULL; > > > > + struct drm_connector *conn; > > > > + struct drm_crtc_state *crtc_state; > > > > + int i; > > > > + > > > > + for_each_new_connector_in_state(pstate->state, conn, conn_state, i) { > > > > + if (conn_state->crtc == pstate->crtc) > > > > + break; > > > > + } > > > > + > > > > + if (i == pstate->state->num_connector) > > > > + return 0; > > > > + > > > > + if (conn_state->underscan.mode != DRM_UNDERSCAN_ON) > > > > + return 0; > > > > + > > > > + crtc_state = drm_atomic_get_new_crtc_state(pstate->state, > > > > + pstate->crtc); > > > > + > > > > + if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay || > > > > + conn_state->underscan.vborder >= crtc_state->mode.vdisplay) > > > > + return -EINVAL; > > > > > > border * 2 ? > > > > Oops, indeed. I'll fix that. > > > > > > > > > + > > > > + vc4_pstate->crtc_x += conn_state->underscan.hborder; > > > > + vc4_pstate->crtc_y += conn_state->underscan.vborder; > > > > + vc4_pstate->crtc_w = (vc4_pstate->crtc_w * > > > > + (crtc_state->mode.hdisplay - > > > > + (conn_state->underscan.hborder * 2))) / > > > > + crtc_state->mode.hdisplay; > > > > + vc4_pstate->crtc_h = (vc4_pstate->crtc_h * > > > > + (crtc_state->mode.vdisplay - > > > > + (conn_state->underscan.vborder * 2))) / > > > > + crtc_state->mode.vdisplay; > > > > > > So you're now scaling all planes? The code seems to reject scaling for > > > the cursor plane, how are you dealing with that? Or just no cursor > > > allowed when underscanning? > > > > No, I just didn't test with a cursor plane. We should probably avoid > > scaling the cursor plane and just adjust its position. Eric any opinion > > on that? > > I don't think you can just not scale it. The user asked for the cursor > to be at a specific place with a specific size. Can't just ignore > that and do something else. Also eg. i915 would definitely scale the > cursor since we'd just scale the entire crtc instead of scaling > individual planes. Different drivers doing different things wouldn't > be good. Except in our case the scaling takes place before the composition, so we don't have a choice. > > > > > > > > > I'm also wondering if there's any way we can reconcile these border > > > props with the scaling mode prop, should we ever wish to expose > > > these props on connectors that already have the scaling mode prop. > > > > Nouveau seems to expose both, and I don't see why we couldn't. > > First of all the interaction of these borders with panels that have > a fixed mode would need to be properly specified. Do we apply the > borders against the user mode or the panel's native mode? Hm, I'm a bit lost. Isn't crtc_state->mode supposed to contain the mode that is about to be applied, no matter if it's a usermode or one of the mode returned by the display? > I guess > the latter would be a bit easier (would also match how the TV margins > behave I suppose). But that does leave open the question of how > would generic userspace know which case it's dealing with? Eg. if it > wants to underscan by some specific percentage it has to calculate > the borders based on the correct mode, otherwise the results aren't > going to match the expectations. I don't get it, sorry. Borders are relative to the mode applied by userspace. So if it needs to express borders as a ratio of the resolution, then for sure userspace will have to do the math, but I don't see the problem here.
On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote: > On Fri, 11 May 2018 19:54:02 +0300 > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote: > > > On Fri, 11 May 2018 18:34:50 +0300 > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote: > > > > > Applying an underscan setup is just a matter of scaling all planes > > > > > appropriately and adjusting the CRTC X/Y offset to account for the > > > > > horizontal and vertical border. > > > > > > > > > > Create an vc4_plane_underscan_adj() function doing that and call it from > > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach > > > > > underscan properties to the HDMI connector. > > > > > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > > > --- > > > > > Changes in v2: > > > > > - Take changes on hborder/vborder meaning into account > > > > > --- > > > > > drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++- > > > > > 1 file changed, 48 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c > > > > > index 71d44c357d35..61ed60841cd6 100644 > > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c > > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane) > > > > > } > > > > > } > > > > > > > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate) > > > > > +{ > > > > > + struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate); > > > > > + struct drm_connector_state *conn_state = NULL; > > > > > + struct drm_connector *conn; > > > > > + struct drm_crtc_state *crtc_state; > > > > > + int i; > > > > > + > > > > > + for_each_new_connector_in_state(pstate->state, conn, conn_state, i) { > > > > > + if (conn_state->crtc == pstate->crtc) > > > > > + break; > > > > > + } > > > > > + > > > > > + if (i == pstate->state->num_connector) > > > > > + return 0; > > > > > + > > > > > + if (conn_state->underscan.mode != DRM_UNDERSCAN_ON) > > > > > + return 0; > > > > > + > > > > > + crtc_state = drm_atomic_get_new_crtc_state(pstate->state, > > > > > + pstate->crtc); > > > > > + > > > > > + if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay || > > > > > + conn_state->underscan.vborder >= crtc_state->mode.vdisplay) > > > > > + return -EINVAL; > > > > > > > > border * 2 ? > > > > > > Oops, indeed. I'll fix that. > > > > > > > > > > > > + > > > > > + vc4_pstate->crtc_x += conn_state->underscan.hborder; > > > > > + vc4_pstate->crtc_y += conn_state->underscan.vborder; > > > > > + vc4_pstate->crtc_w = (vc4_pstate->crtc_w * > > > > > + (crtc_state->mode.hdisplay - > > > > > + (conn_state->underscan.hborder * 2))) / > > > > > + crtc_state->mode.hdisplay; > > > > > + vc4_pstate->crtc_h = (vc4_pstate->crtc_h * > > > > > + (crtc_state->mode.vdisplay - > > > > > + (conn_state->underscan.vborder * 2))) / > > > > > + crtc_state->mode.vdisplay; > > > > > > > > So you're now scaling all planes? The code seems to reject scaling for > > > > the cursor plane, how are you dealing with that? Or just no cursor > > > > allowed when underscanning? > > > > > > No, I just didn't test with a cursor plane. We should probably avoid > > > scaling the cursor plane and just adjust its position. Eric any opinion > > > on that? > > > > I don't think you can just not scale it. The user asked for the cursor > > to be at a specific place with a specific size. Can't just ignore > > that and do something else. Also eg. i915 would definitely scale the > > cursor since we'd just scale the entire crtc instead of scaling > > individual planes. Different drivers doing different things wouldn't > > be good. > > Except in our case the scaling takes place before the composition, so > we don't have a choice. The choice is to either do what userspace asked, or return an error. > > > > > > > > > > > > > > I'm also wondering if there's any way we can reconcile these border > > > > props with the scaling mode prop, should we ever wish to expose > > > > these props on connectors that already have the scaling mode prop. > > > > > > Nouveau seems to expose both, and I don't see why we couldn't. > > > > First of all the interaction of these borders with panels that have > > a fixed mode would need to be properly specified. Do we apply the > > borders against the user mode or the panel's native mode? > > Hm, I'm a bit lost. Isn't crtc_state->mode supposed to contain the mode > that is about to be applied, no matter if it's a usermode or one of the > mode returned by the display? No. With fixed mode panels the user mode is a lie. Only hdisplay/vdisplay mean anything. The actual timings are known only to the kernel. Well, userspace can perhaps make an educated guess based on the connector's mode list and the presence of the scaling mode property. > > > I guess > > the latter would be a bit easier (would also match how the TV margins > > behave I suppose). But that does leave open the question of how > > would generic userspace know which case it's dealing with? Eg. if it > > wants to underscan by some specific percentage it has to calculate > > the borders based on the correct mode, otherwise the results aren't > > going to match the expectations. > > I don't get it, sorry. Borders are relative to the mode applied by > userspace. So if it needs to express borders as a ratio of the > resolution, then for sure userspace will have to do the math, but I > don't see the problem here.
On Fri, 11 May 2018 20:29:48 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote: > > On Fri, 11 May 2018 19:54:02 +0300 > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote: > > > > On Fri, 11 May 2018 18:34:50 +0300 > > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > > > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote: > > > > > > Applying an underscan setup is just a matter of scaling all planes > > > > > > appropriately and adjusting the CRTC X/Y offset to account for the > > > > > > horizontal and vertical border. > > > > > > > > > > > > Create an vc4_plane_underscan_adj() function doing that and call it from > > > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach > > > > > > underscan properties to the HDMI connector. > > > > > > > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > > > > --- > > > > > > Changes in v2: > > > > > > - Take changes on hborder/vborder meaning into account > > > > > > --- > > > > > > drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++- > > > > > > 1 file changed, 48 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c > > > > > > index 71d44c357d35..61ed60841cd6 100644 > > > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c > > > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > > > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane) > > > > > > } > > > > > > } > > > > > > > > > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate) > > > > > > +{ > > > > > > + struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate); > > > > > > + struct drm_connector_state *conn_state = NULL; > > > > > > + struct drm_connector *conn; > > > > > > + struct drm_crtc_state *crtc_state; > > > > > > + int i; > > > > > > + > > > > > > + for_each_new_connector_in_state(pstate->state, conn, conn_state, i) { > > > > > > + if (conn_state->crtc == pstate->crtc) > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > + if (i == pstate->state->num_connector) > > > > > > + return 0; > > > > > > + > > > > > > + if (conn_state->underscan.mode != DRM_UNDERSCAN_ON) > > > > > > + return 0; > > > > > > + > > > > > > + crtc_state = drm_atomic_get_new_crtc_state(pstate->state, > > > > > > + pstate->crtc); > > > > > > + > > > > > > + if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay || > > > > > > + conn_state->underscan.vborder >= crtc_state->mode.vdisplay) > > > > > > + return -EINVAL; > > > > > > > > > > border * 2 ? > > > > > > > > Oops, indeed. I'll fix that. > > > > > > > > > > > > > > > + > > > > > > + vc4_pstate->crtc_x += conn_state->underscan.hborder; > > > > > > + vc4_pstate->crtc_y += conn_state->underscan.vborder; > > > > > > + vc4_pstate->crtc_w = (vc4_pstate->crtc_w * > > > > > > + (crtc_state->mode.hdisplay - > > > > > > + (conn_state->underscan.hborder * 2))) / > > > > > > + crtc_state->mode.hdisplay; > > > > > > + vc4_pstate->crtc_h = (vc4_pstate->crtc_h * > > > > > > + (crtc_state->mode.vdisplay - > > > > > > + (conn_state->underscan.vborder * 2))) / > > > > > > + crtc_state->mode.vdisplay; > > > > > > > > > > So you're now scaling all planes? The code seems to reject scaling for > > > > > the cursor plane, how are you dealing with that? Or just no cursor > > > > > allowed when underscanning? > > > > > > > > No, I just didn't test with a cursor plane. We should probably avoid > > > > scaling the cursor plane and just adjust its position. Eric any opinion > > > > on that? > > > > > > I don't think you can just not scale it. The user asked for the cursor > > > to be at a specific place with a specific size. Can't just ignore > > > that and do something else. Also eg. i915 would definitely scale the > > > cursor since we'd just scale the entire crtc instead of scaling > > > individual planes. Different drivers doing different things wouldn't > > > be good. > > > > Except in our case the scaling takes place before the composition, so > > we don't have a choice. > > The choice is to either do what userspace asked, or return an error. Come on! If we can't use underscan when there's a cursor plane enabled this feature is pretty much useless. But let's take a real use case to show you how negligible the lack of scaling on the cursor plane will be. Say you have borders taking 10% of you screen (which is already a lot), and your cursor is a plane of 64x64 pixels, you'll end up with a 64x64 cursor instead of 58x58. Quite frankly, I doubt you'll notice the difference. Anyway, I'd like to hear back from Eric on that, since he is the one who asked me to work on this feature. > > > > > > > > > > > > > > > > > > > > I'm also wondering if there's any way we can reconcile these border > > > > > props with the scaling mode prop, should we ever wish to expose > > > > > these props on connectors that already have the scaling mode prop. > > > > > > > > Nouveau seems to expose both, and I don't see why we couldn't. > > > > > > First of all the interaction of these borders with panels that have > > > a fixed mode would need to be properly specified. Do we apply the > > > borders against the user mode or the panel's native mode? > > > > Hm, I'm a bit lost. Isn't crtc_state->mode supposed to contain the mode > > that is about to be applied, no matter if it's a usermode or one of the > > mode returned by the display? > > No. With fixed mode panels the user mode is a lie. Only > hdisplay/vdisplay mean anything. The actual timings are known only to > the kernel. So now you say that the driver can silently ignore what the userspace asked for and set what he thinks is appropriate based on what the HW supports. This clearly contradicts the statement you make a few lines above: driver should apply what userspace asked for or return an error when it's not possible. > Well, userspace can perhaps make an educated guess based on > the connector's mode list and the presence of the scaling mode property. But guess what? Where is the actual 'HW' mode stored if it's not in crtc_state->mode? I mean, the drivers needs to know what to apply, and almost all drivers are using atomic states to extract this information. On a side note, I'd like to repeat what I already said before: I don't care how this "define borders" feature is exposed to userspace, as long as we have a way to express that the display we're using has borders and the output image should be scaled+moved accordingly. So now it's time to reach a consensus, because I'm not going to try and guess what you expect. When I asked Daniel earlier this week whether he wanted me to do things differently than I did in my v1 or try to match what other drivers were already doing with the underscan prop, he told me we should try to converge to a generic underscan solution that keeps the existing userspace interface unchanged (basically what radeon, amdgpu and nouveau drivers expose today). You don't seem to agree with that. So now I'd like a clear answer, what should I do?
Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote: >> On Fri, 11 May 2018 19:54:02 +0300 >> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> >> > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote: >> > > On Fri, 11 May 2018 18:34:50 +0300 >> > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > > >> > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote: >> > > > > Applying an underscan setup is just a matter of scaling all planes >> > > > > appropriately and adjusting the CRTC X/Y offset to account for the >> > > > > horizontal and vertical border. >> > > > > >> > > > > Create an vc4_plane_underscan_adj() function doing that and call it from >> > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach >> > > > > underscan properties to the HDMI connector. >> > > > > >> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >> > > > > --- >> > > > > Changes in v2: >> > > > > - Take changes on hborder/vborder meaning into account >> > > > > --- >> > > > > drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++- >> > > > > 1 file changed, 48 insertions(+), 1 deletion(-) >> > > > > >> > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c >> > > > > index 71d44c357d35..61ed60841cd6 100644 >> > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c >> > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c >> > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane) >> > > > > } >> > > > > } >> > > > > >> > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate) >> > > > > +{ >> > > > > + struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate); >> > > > > + struct drm_connector_state *conn_state = NULL; >> > > > > + struct drm_connector *conn; >> > > > > + struct drm_crtc_state *crtc_state; >> > > > > + int i; >> > > > > + >> > > > > + for_each_new_connector_in_state(pstate->state, conn, conn_state, i) { >> > > > > + if (conn_state->crtc == pstate->crtc) >> > > > > + break; >> > > > > + } >> > > > > + >> > > > > + if (i == pstate->state->num_connector) >> > > > > + return 0; >> > > > > + >> > > > > + if (conn_state->underscan.mode != DRM_UNDERSCAN_ON) >> > > > > + return 0; >> > > > > + >> > > > > + crtc_state = drm_atomic_get_new_crtc_state(pstate->state, >> > > > > + pstate->crtc); >> > > > > + >> > > > > + if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay || >> > > > > + conn_state->underscan.vborder >= crtc_state->mode.vdisplay) >> > > > > + return -EINVAL; >> > > > >> > > > border * 2 ? >> > > >> > > Oops, indeed. I'll fix that. >> > > >> > > > >> > > > > + >> > > > > + vc4_pstate->crtc_x += conn_state->underscan.hborder; >> > > > > + vc4_pstate->crtc_y += conn_state->underscan.vborder; >> > > > > + vc4_pstate->crtc_w = (vc4_pstate->crtc_w * >> > > > > + (crtc_state->mode.hdisplay - >> > > > > + (conn_state->underscan.hborder * 2))) / >> > > > > + crtc_state->mode.hdisplay; >> > > > > + vc4_pstate->crtc_h = (vc4_pstate->crtc_h * >> > > > > + (crtc_state->mode.vdisplay - >> > > > > + (conn_state->underscan.vborder * 2))) / >> > > > > + crtc_state->mode.vdisplay; >> > > > >> > > > So you're now scaling all planes? The code seems to reject scaling for >> > > > the cursor plane, how are you dealing with that? Or just no cursor >> > > > allowed when underscanning? >> > > >> > > No, I just didn't test with a cursor plane. We should probably avoid >> > > scaling the cursor plane and just adjust its position. Eric any opinion >> > > on that? >> > >> > I don't think you can just not scale it. The user asked for the cursor >> > to be at a specific place with a specific size. Can't just ignore >> > that and do something else. Also eg. i915 would definitely scale the >> > cursor since we'd just scale the entire crtc instead of scaling >> > individual planes. Different drivers doing different things wouldn't >> > be good. >> >> Except in our case the scaling takes place before the composition, so >> we don't have a choice. > > The choice is to either do what userspace asked, or return an error. > >> >> > >> > > >> > > > >> > > > I'm also wondering if there's any way we can reconcile these border >> > > > props with the scaling mode prop, should we ever wish to expose >> > > > these props on connectors that already have the scaling mode prop. >> > > >> > > Nouveau seems to expose both, and I don't see why we couldn't. >> > >> > First of all the interaction of these borders with panels that have >> > a fixed mode would need to be properly specified. Do we apply the >> > borders against the user mode or the panel's native mode? >> >> Hm, I'm a bit lost. Isn't crtc_state->mode supposed to contain the mode >> that is about to be applied, no matter if it's a usermode or one of the >> mode returned by the display? > > No. With fixed mode panels the user mode is a lie. Only > hdisplay/vdisplay mean anything. The actual timings are known only to > the kernel. Well, userspace can perhaps make an educated guess based on > the connector's mode list and the presence of the scaling mode property. Let's just not expose the property on devices with fixed mode panels. Problem solved because all this property is is a way to work around HDMI monitors doing something awful, and trying to build something more general isn't serving anyone.
On Fri, May 11, 2018 at 09:47:49PM +0200, Boris Brezillon wrote: > On Fri, 11 May 2018 20:29:48 +0300 > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote: > > > On Fri, 11 May 2018 19:54:02 +0300 > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > > > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote: > > > > > On Fri, 11 May 2018 18:34:50 +0300 > > > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > > > > > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote: > > > > > > > Applying an underscan setup is just a matter of scaling all planes > > > > > > > appropriately and adjusting the CRTC X/Y offset to account for the > > > > > > > horizontal and vertical border. > > > > > > > > > > > > > > Create an vc4_plane_underscan_adj() function doing that and call it from > > > > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach > > > > > > > underscan properties to the HDMI connector. > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > > > > > --- > > > > > > > Changes in v2: > > > > > > > - Take changes on hborder/vborder meaning into account > > > > > > > --- > > > > > > > drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++- > > > > > > > 1 file changed, 48 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c > > > > > > > index 71d44c357d35..61ed60841cd6 100644 > > > > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c > > > > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > > > > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane) > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate) > > > > > > > +{ > > > > > > > + struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate); > > > > > > > + struct drm_connector_state *conn_state = NULL; > > > > > > > + struct drm_connector *conn; > > > > > > > + struct drm_crtc_state *crtc_state; > > > > > > > + int i; > > > > > > > + > > > > > > > + for_each_new_connector_in_state(pstate->state, conn, conn_state, i) { > > > > > > > + if (conn_state->crtc == pstate->crtc) > > > > > > > + break; > > > > > > > + } > > > > > > > + > > > > > > > + if (i == pstate->state->num_connector) > > > > > > > + return 0; > > > > > > > + > > > > > > > + if (conn_state->underscan.mode != DRM_UNDERSCAN_ON) > > > > > > > + return 0; > > > > > > > + > > > > > > > + crtc_state = drm_atomic_get_new_crtc_state(pstate->state, > > > > > > > + pstate->crtc); > > > > > > > + > > > > > > > + if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay || > > > > > > > + conn_state->underscan.vborder >= crtc_state->mode.vdisplay) > > > > > > > + return -EINVAL; > > > > > > > > > > > > border * 2 ? > > > > > > > > > > Oops, indeed. I'll fix that. > > > > > > > > > > > > > > > > > > + > > > > > > > + vc4_pstate->crtc_x += conn_state->underscan.hborder; > > > > > > > + vc4_pstate->crtc_y += conn_state->underscan.vborder; > > > > > > > + vc4_pstate->crtc_w = (vc4_pstate->crtc_w * > > > > > > > + (crtc_state->mode.hdisplay - > > > > > > > + (conn_state->underscan.hborder * 2))) / > > > > > > > + crtc_state->mode.hdisplay; > > > > > > > + vc4_pstate->crtc_h = (vc4_pstate->crtc_h * > > > > > > > + (crtc_state->mode.vdisplay - > > > > > > > + (conn_state->underscan.vborder * 2))) / > > > > > > > + crtc_state->mode.vdisplay; > > > > > > > > > > > > So you're now scaling all planes? The code seems to reject scaling for > > > > > > the cursor plane, how are you dealing with that? Or just no cursor > > > > > > allowed when underscanning? > > > > > > > > > > No, I just didn't test with a cursor plane. We should probably avoid > > > > > scaling the cursor plane and just adjust its position. Eric any opinion > > > > > on that? > > > > > > > > I don't think you can just not scale it. The user asked for the cursor > > > > to be at a specific place with a specific size. Can't just ignore > > > > that and do something else. Also eg. i915 would definitely scale the > > > > cursor since we'd just scale the entire crtc instead of scaling > > > > individual planes. Different drivers doing different things wouldn't > > > > be good. > > > > > > Except in our case the scaling takes place before the composition, so > > > we don't have a choice. > > > > The choice is to either do what userspace asked, or return an error. > > Come on! If we can't use underscan when there's a cursor plane enabled > this feature is pretty much useless. But let's take a real use case to > show you how negligible the lack of scaling on the cursor plane will > be. Say you have borders taking 10% of you screen (which is already a > lot), and your cursor is a plane of 64x64 pixels, you'll end up with a > 64x64 cursor instead of 58x58. Quite frankly, I doubt you'll notice > the difference. Now you're assuming the cursor is only ever used as a cursor. It can be used for other things and those may need to be positioned pixel perfect in relation to other planes/fb contents. We used to play is fast and loose in i915 when it came to the sprite plane coordinates. People generally hated that, at least when it came to the atomic ioctl. Basically we just adjusted the src/dst coordinates until the hw was happy with them, partially ignoring what the user had asked. Maarten recently nuked that code, and so now we either respect the user's choice or we return an error. I guess one way out of this conundrum would be to allow the cursor to violate the user's requested parameters when controlled via the legacy cursor ioctls. There are no atomicity guarantees there, so I guess we could also say there are no other correctness guarantees either. Not sure if the accuracy of the hotspot might become an issue though. Another option might be to just scale the cursor as well. If I understand correctly the "cursor can't be scaled" limitation just comes from the fact that some vblank synced resource needs to be reconfigured whenever the scaling changes. So doing that for unthrottled cursor updates is not easy. But in this case the underscan properties are what determines the scaling so maybe that resource could be reconfigured whenever the those props change to make sure the cursor can always be scaled appropriately? > > Anyway, I'd like to hear back from Eric on that, since he is the one > who asked me to work on this feature. > > > > > > > > > > > > > > > > > > > > > > > > > > > I'm also wondering if there's any way we can reconcile these border > > > > > > props with the scaling mode prop, should we ever wish to expose > > > > > > these props on connectors that already have the scaling mode prop. > > > > > > > > > > Nouveau seems to expose both, and I don't see why we couldn't. > > > > > > > > First of all the interaction of these borders with panels that have > > > > a fixed mode would need to be properly specified. Do we apply the > > > > borders against the user mode or the panel's native mode? > > > > > > Hm, I'm a bit lost. Isn't crtc_state->mode supposed to contain the mode > > > that is about to be applied, no matter if it's a usermode or one of the > > > mode returned by the display? > > > > No. With fixed mode panels the user mode is a lie. Only > > hdisplay/vdisplay mean anything. The actual timings are known only to > > the kernel. > > So now you say that the driver can silently ignore what the userspace > asked for and set what he thinks is appropriate based on what the HW > supports. This clearly contradicts the statement you make a few lines > above: driver should apply what userspace asked for or return an error > when it's not possible. It's an unfortunate legacy uapi design we have to live with. I agree that it would be much better if userspace had more explicit control over the crtc scaling/borders/etc. > > > Well, userspace can perhaps make an educated guess based on > > the connector's mode list and the presence of the scaling mode property. > > But guess what? Where is the actual 'HW' mode stored if it's not in > crtc_state->mode? I mean, the drivers needs to know what to apply, and > almost all drivers are using atomic states to extract this information. adjusted_mode is there for the driver to do as they wish. Eg. in i915 LVDS/eDP/DSI/etc. encoders will overwrite the adjusted_mode with the panel's native mode at the appropriate time (for other encoder types we just populate adjusted_mode with the user mode). For the user it will still *look* like the mode they asked for (ie. hdisplay/vdisplay determine how many visible pixels there are), but internally we've replaced the timings with something different and there may or may not scaling involved (depend on the scaling mode property). > > On a side note, I'd like to repeat what I already said before: I don't > care how this "define borders" feature is exposed to userspace, as long > as we have a way to express that the display we're using has borders > and the output image should be scaled+moved accordingly. > > So now it's time to reach a consensus, because I'm not going to try and > guess what you expect. When I asked Daniel earlier this week whether he > wanted me to do things differently than I did in my v1 or try to match > what other drivers were already doing with the underscan prop, he told > me we should try to converge to a generic underscan solution that keeps > the existing userspace interface unchanged (basically what radeon, > amdgpu and nouveau drivers expose today). You don't seem to agree with > that. So now I'd like a clear answer, what should I do? What do those drivers do when it comes to planes? I would expect that they just scale the entire crtc instead of individual planes.
Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > On Fri, May 11, 2018 at 09:47:49PM +0200, Boris Brezillon wrote: >> On Fri, 11 May 2018 20:29:48 +0300 >> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> >> > On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote: >> > > On Fri, 11 May 2018 19:54:02 +0300 >> > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > > >> > > > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote: >> > > > > On Fri, 11 May 2018 18:34:50 +0300 >> > > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > > > > >> > > > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote: >> > > > > > > Applying an underscan setup is just a matter of scaling all planes >> > > > > > > appropriately and adjusting the CRTC X/Y offset to account for the >> > > > > > > horizontal and vertical border. >> > > > > > > >> > > > > > > Create an vc4_plane_underscan_adj() function doing that and call it from >> > > > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach >> > > > > > > underscan properties to the HDMI connector. >> > > > > > > >> > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >> > > > > > > --- >> > > > > > > Changes in v2: >> > > > > > > - Take changes on hborder/vborder meaning into account >> > > > > > > --- >> > > > > > > drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++- >> > > > > > > 1 file changed, 48 insertions(+), 1 deletion(-) >> > > > > > > >> > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c >> > > > > > > index 71d44c357d35..61ed60841cd6 100644 >> > > > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c >> > > > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c >> > > > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane) >> > > > > > > } >> > > > > > > } >> > > > > > > >> > > > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate) >> > > > > > > +{ >> > > > > > > + struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate); >> > > > > > > + struct drm_connector_state *conn_state = NULL; >> > > > > > > + struct drm_connector *conn; >> > > > > > > + struct drm_crtc_state *crtc_state; >> > > > > > > + int i; >> > > > > > > + >> > > > > > > + for_each_new_connector_in_state(pstate->state, conn, conn_state, i) { >> > > > > > > + if (conn_state->crtc == pstate->crtc) >> > > > > > > + break; >> > > > > > > + } >> > > > > > > + >> > > > > > > + if (i == pstate->state->num_connector) >> > > > > > > + return 0; >> > > > > > > + >> > > > > > > + if (conn_state->underscan.mode != DRM_UNDERSCAN_ON) >> > > > > > > + return 0; >> > > > > > > + >> > > > > > > + crtc_state = drm_atomic_get_new_crtc_state(pstate->state, >> > > > > > > + pstate->crtc); >> > > > > > > + >> > > > > > > + if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay || >> > > > > > > + conn_state->underscan.vborder >= crtc_state->mode.vdisplay) >> > > > > > > + return -EINVAL; >> > > > > > >> > > > > > border * 2 ? >> > > > > >> > > > > Oops, indeed. I'll fix that. >> > > > > >> > > > > > >> > > > > > > + >> > > > > > > + vc4_pstate->crtc_x += conn_state->underscan.hborder; >> > > > > > > + vc4_pstate->crtc_y += conn_state->underscan.vborder; >> > > > > > > + vc4_pstate->crtc_w = (vc4_pstate->crtc_w * >> > > > > > > + (crtc_state->mode.hdisplay - >> > > > > > > + (conn_state->underscan.hborder * 2))) / >> > > > > > > + crtc_state->mode.hdisplay; >> > > > > > > + vc4_pstate->crtc_h = (vc4_pstate->crtc_h * >> > > > > > > + (crtc_state->mode.vdisplay - >> > > > > > > + (conn_state->underscan.vborder * 2))) / >> > > > > > > + crtc_state->mode.vdisplay; >> > > > > > >> > > > > > So you're now scaling all planes? The code seems to reject scaling for >> > > > > > the cursor plane, how are you dealing with that? Or just no cursor >> > > > > > allowed when underscanning? >> > > > > >> > > > > No, I just didn't test with a cursor plane. We should probably avoid >> > > > > scaling the cursor plane and just adjust its position. Eric any opinion >> > > > > on that? >> > > > >> > > > I don't think you can just not scale it. The user asked for the cursor >> > > > to be at a specific place with a specific size. Can't just ignore >> > > > that and do something else. Also eg. i915 would definitely scale the >> > > > cursor since we'd just scale the entire crtc instead of scaling >> > > > individual planes. Different drivers doing different things wouldn't >> > > > be good. >> > > >> > > Except in our case the scaling takes place before the composition, so >> > > we don't have a choice. >> > >> > The choice is to either do what userspace asked, or return an error. >> >> Come on! If we can't use underscan when there's a cursor plane enabled >> this feature is pretty much useless. But let's take a real use case to >> show you how negligible the lack of scaling on the cursor plane will >> be. Say you have borders taking 10% of you screen (which is already a >> lot), and your cursor is a plane of 64x64 pixels, you'll end up with a >> 64x64 cursor instead of 58x58. Quite frankly, I doubt you'll notice >> the difference. > > Now you're assuming the cursor is only ever used as a cursor. It can > be used for other things and those may need to be positioned pixel > perfect in relation to other planes/fb contents. > > We used to play is fast and loose in i915 when it came to the sprite > plane coordinates. People generally hated that, at least when it came > to the atomic ioctl. Basically we just adjusted the src/dst > coordinates until the hw was happy with them, partially ignoring > what the user had asked. Maarten recently nuked that code, and so > now we either respect the user's choice or we return an error. > > I guess one way out of this conundrum would be to allow the cursor > to violate the user's requested parameters when controlled via the > legacy cursor ioctls. There are no atomicity guarantees there, so > I guess we could also say there are no other correctness guarantees > either. Not sure if the accuracy of the hotspot might become an issue > though. > > Another option might be to just scale the cursor as well. If I > understand correctly the "cursor can't be scaled" limitation just > comes from the fact that some vblank synced resource needs to be > reconfigured whenever the scaling changes. So doing that for > unthrottled cursor updates is not easy. But in this case the > underscan properties are what determines the scaling so maybe that > resource could be reconfigured whenever the those props change > to make sure the cursor can always be scaled appropriately? Yeah, we had no application for it, so it wasn't supported. I don't think it would be hard to have a previously-scaled cursor move, it's just that we need to fall back to a synchronous plane update if the scaling parameters change.
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 71d44c357d35..61ed60841cd6 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct drm_plane_state *state, int plane) } } +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate) +{ + struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate); + struct drm_connector_state *conn_state = NULL; + struct drm_connector *conn; + struct drm_crtc_state *crtc_state; + int i; + + for_each_new_connector_in_state(pstate->state, conn, conn_state, i) { + if (conn_state->crtc == pstate->crtc) + break; + } + + if (i == pstate->state->num_connector) + return 0; + + if (conn_state->underscan.mode != DRM_UNDERSCAN_ON) + return 0; + + crtc_state = drm_atomic_get_new_crtc_state(pstate->state, + pstate->crtc); + + if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay || + conn_state->underscan.vborder >= crtc_state->mode.vdisplay) + return -EINVAL; + + vc4_pstate->crtc_x += conn_state->underscan.hborder; + vc4_pstate->crtc_y += conn_state->underscan.vborder; + vc4_pstate->crtc_w = (vc4_pstate->crtc_w * + (crtc_state->mode.hdisplay - + (conn_state->underscan.hborder * 2))) / + crtc_state->mode.hdisplay; + vc4_pstate->crtc_h = (vc4_pstate->crtc_h * + (crtc_state->mode.vdisplay - + (conn_state->underscan.vborder * 2))) / + crtc_state->mode.vdisplay; + + if (!vc4_pstate->crtc_w || !vc4_pstate->crtc_h) + return -EINVAL; + + return 0; +} + static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) { struct drm_plane *plane = state->plane; @@ -269,7 +312,7 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) int num_planes = fb->format->num_planes; u32 h_subsample = 1; u32 v_subsample = 1; - int i; + int i, ret; for (i = 0; i < num_planes; i++) vc4_state->offsets[i] = bo->paddr + fb->offsets[i]; @@ -292,6 +335,10 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) vc4_state->crtc_w = state->crtc_w; vc4_state->crtc_h = state->crtc_h; + ret = vc4_plane_underscan_adj(state); + if (ret) + return ret; + vc4_state->x_scaling[0] = vc4_get_scaling_mode(vc4_state->src_w[0], vc4_state->crtc_w); vc4_state->y_scaling[0] = vc4_get_scaling_mode(vc4_state->src_h[0],
Applying an underscan setup is just a matter of scaling all planes appropriately and adjusting the CRTC X/Y offset to account for the horizontal and vertical border. Create an vc4_plane_underscan_adj() function doing that and call it from vc4_plane_setup_clipping_and_scaling() so that we are ready to attach underscan properties to the HDMI connector. Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- Changes in v2: - Take changes on hborder/vborder meaning into account --- drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-)