On Fri, 11 May 2018 19:54:02 +0300
Ville Syrjälä <ville.syrjala at 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 at 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 at 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.
