Ville Syrjälä <ville.syrjala at 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 at 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 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.
>> >
>> > 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180514/680055be/attachment.sig>
