CRTC properties patches

[Paulo Zanoni]

Hi

Here is a new version of the patches.

Summary: almost all patches changed at least one line, Kernel patches
7 and 8 are RFCs.

The main difference is that now the ioctls are not for CRTC properties, but for
generic "object" properties. I decided to write it like this after Rob Clark's
review. So when he creates plane properties we won't need a new pair of ioctls
for get/set. User space patches got updated to this change and one or two minor
improvements and fixes.

Kernel patches now on top of ~airlied/linux tree, drm-next branch.

Most patches from Kernel, Libdrm and xf86-video-intel have small changes, so all
the previous patches should be discarded.

- Kernel:
To achieve "generic object properties" and make it easier to add properties to
other objects, I did some small changes: I created a "struct
drm_object_properties" and added a pointer to such a struct inside "struct
drm_mode_object", so we can access the object properties without knowing the
type of the object and, more important, without needing to call
obj_to_something() needlessly. I also did some small modifications suggested by
the other reviewers (changed int to bool and changed the property names to match
radeon properties). IMHO the patch series is now easier to read too.

After talking to Rob we concluded that the 'rotation' property should be a bit
mask instead of an number, so we allow not only rotation but also reflection (at
the cost of rotations that are not 0, 90, 180, 270 degrees, but these should
really be implemented as a property equivalent to  X's transformation matrix).
So patch 7 is still an RFC (as a consequence, xf86-video-intel patches are
still RFCs): I'm waiting for his implementation of "bit mask properties".

0001: drm: add drm_property_change_is_valid
0002: drm: WARN() when drm_connector_attach_property fails
0003: drm: create struct drm_object_properties and use it
0004: drm: add generic ioctls to get/set properties on any object
0005: drm: make the connector properties code use the object properties code
0006: drm: add CRTC properties
0007 (RFC): drm/i915: add 'rotation' CRTC property
0008 (RFC): drm/i915: add overscan compensation CRTC properties

- Libdrm
The libdrm code is basically the same, except that it now implements the
"generic" ioctl instead of the crtc one. Still, basically only function names
have changed. Patches 1, 2 and 3 could be applied even if we don't ever
accept any other patch of the series.

0001: modetest: fix some compiler warnings
0002: modetest: fix memory leak
0003: modetest: print more about our properties
0004: Add support for generic object properties IOCTLs
0005: modetest: print CRTC properties

- xf86-video-intel:
Changes on ddx are also small. Two changes: for the case where you have a newer
ddx+libdrm with an older Kernel, X won't segfault. Also, if the underscan
properties don't exist, the X atoms won't be created. These patches
are still RFCs
(since the Rotation property patch will change).

I still didn't add the libdrm dependency and the sna/ code, so for now these are
just for review.

0001: Update the rotation property whenever we change the rotation.
0002: Add underscan properties

Cheers,
Paulo

2012/3/20 Paulo Zanoni <przanoni at gmail.com>:
> Hi
>
> Some time ago I posted a patch series to implement Kernel CRTC
> properties and a "rotation" property for the i915 driver. No one
> objected those patches, so now I'm resending the rebased versions, with
> some minor changes and also "overscan compensation" properties. The
> patches apply to the ~danvet/drm-intel tree, drm-intel-next-queued
> branch.
>
> Summary (i.e., tl;dr)
> ---------------------
>
> - CRTC properties seem to be the most correct way to solve some problems
> - "Rotation" property solves problems with Intel AMT KVM
> - "Overscan Compensation" properties solve problems with TVs/monitors
>  that do overscan
> - patches for kernel, libdrm, and xf86-video-intel
>
> Definition of the problem
> -------------------------
>
> Some of the "features" of the Intel hardware have to be configured per
> pipe. In the DRM world, the pipe is abstracted as a CRTC, so the thing
> that makes most sense is to implement these features as CRTC properties.
>
> Why not connector properties? In theory, we can have more than one
> connector per each pipe, so we would have to keep synchronizing the
> properties with all the connectors associated with a pipe, and also
> update everything each time a connector changes pipe. If we can have the
> "correct" (IMHO) CRTC properties implementation, why make hackish
> connector properties?
>
> So far, there are two features that the i915 driver can/should implement
> as CRTC properties: "rotation" and "overscan compensation".
>
> Rotation
> --------
>
> (Disclaimer: this description is purely based on my own experiences with
> the feature and it represents only my own opinion, not Intel's opinion.
> The description might even be wrong.)
>
> Intel AMT has a feature called KVM (Keyboard, Video, Mouse). AFAIK, this
> is available for all machines labeled as "vPro" (like my Lenovo
> laptops). Once you enable this feature on your machine, you can
> remotely connect to the machine using standard VNC protocol
> (additionally, there are also non-standard features for a closed-source
> VNC client, but I've never used them). This is a very cool thing since
> you don't need to install any software on the KVM machine, and you can
> even reboot the machine and browse the BIOS using the VNC.
>
> The KVM implementation reads the "screen" directly from the video
> graphics registers (it's OS independent, remember?). The problem is:
> whenever the screen is rotated by X, the KVM implementation needs to
> know this, otherwise the mouse pointer will be moved to the wrong
> direction. How do we tell the KVM that the screen is rotated? We need to
> set some bits on the PIPECONF registers.
>
> So my implementation creates a CRTC property called "rotation" and then
> whenever X rotates the screen, xf86-video-intel updates the property,
> and everyone gets happy. The property is not exposed to the "users".
>
> This problem is very easy to reproduce:
> - acquire a vPro machine (bro tip: use magnets for faster acquiring)
> - configure AMT KVM (there's a "ctrl+p" menu somewhere when you boot)
> - boot the machine, use xrandr to rotate the screen
> - connect to the KVM through VNC (any client will do)
> - have fun trying to use the mouse
>
> Overscan Compensation (a.k.a. "Underscan")
> ------------------------------------------
>
> Recently we have enabled Interlaced modes in our driver. One of the
> problems remaining is that some TVs and monitors will enable overscan
> when you use Interlaced modes (actually, some enable overscan even on
> progressive modes, like my own TV). One possible solution to this
> problem would be to send an AVI infoframe that asks the TV to not
> overscan (this patch is on my TODO list), but most TVs/monitors don't
> repect the infoframes, so another solution is to use the panel fitters
> to compensate for overscan, and since this is independent from the
> TV/monitor, it should work with every device (even LVDS).
>
> I already have submitted to intel-gpu-tools a tool called
> "intel_panel_fitter" that does overscan compensation using the panel
> fitter (so people that don't have future kernels can already have
> overscan compensation). The tool works but requires you to be root, so
> now I am proposing two new CRTC properties: "underscan x" and "underscan
> y".
>
> When I implemented CRTC properties I followed the examples and made the
> ioctls require you to be DRM master, so we can't just rely on having our
> users calling the libdrm functions: we need to make X (or
> xf86-video-intel) expose the properties to the user. How do we expose
> those properties? I imagined 3 solutions:
>
> 1 - Change the xrandr protocol to include crtc properties. This would
>    require a lot of releases, the code would take many months to arrive
>    the distros.
> 2 - Implement a xorg.conf option to enable/disable underscan. At first
>    this seems simple, but when you start thinking that the user wants
>    underscan for just some modes on a specific output, and that
>    sometimes compensation must be on and sometimes it must be off, it
>    gets complicated: how do we specify this in xorg.conf?
> 3 - Do the hack I wanted to avoid inside the Kernel: expose the
>    underscan properties as connector properties, and whenever these
>    connector properties change, change the Kernel CRTC properties.
>
> So I chose option 3 and, if people show interest, I can also implement
> option 1 for the "long-term", and then deprecate option 3 once option 1
> gets into the X server.
>
> Q: But you kept pushing CRTC properties as the "right" solution and now
> you expose overscan properties to the user as connector properties! Why
> don't you just implement everything inside the kernel as connector
> properties?
>
> A: First of all, don't forget the "rotation" property that is used
> correctly, and also all the other properties that we might create in the
> future. For the overscan compensation properties specifically, having
> the Kernel implement the correct solution and then xf86-video-intel
> implement the "hackish" solution is better than having the Kernel
> implement the "hackish" solution and force every single  app (like
> Wayland) also implement a hackish solution. Also, adding CRTC properties
> to RandR in the future would solve the problem.
>
>
> The patches
> -----------
>
> -> Kernel
>
> - 0001: drm: add drm_property_change_is_valid
> Preparatory work to avoid code duplication in the future.
>
> - 0002: drm: WARN() when drm_connector_attach_property fails
> When I was implementing CRTC properties I was looking at how connector
> properties were implemented and found this. The WARN is a must (IMHO), but
> changing from void to int could be reverted if you want (then I'll also
> change drm_crtc_attach_property to return int, since I want both the
> connector and crtc property functions to have the same usage).
>
> - 0003: drm: add CRTC properties
> Adds the CRTC properties interface, but no properties.
>
> - 0004: drm/i915: add 'rotation' CRTC property
> First i915 property.
>
> - 0005: drm/i915: add overscan compensation CRTC properties
> Second and third i915 properties.
>
>
> -> libdrm
>
> - 0001: modetest: fix some compiler warnings
> This patch is independent from CRTC properties. While learning
> everything I was using the modetest tool, so I decided to fix some of
> its trivial warnings.
>
> - 0002: modetest: fix memory leak
> Same as 0001: independent from CRTC properties.
>
> - 0003: modetest: print more about our properties
> Same as 0002: independent from CRTC properties.
>
> - 0004: Add support for CRTC properties
> This function adds the ioctl wrappers.
>
> - 0005: modetest: print CRTC properties
> Use the ioctls.
>
>
> -> xf86-video-intel
>
> - 0001: Avoid duplicated code with intel_output_create_ranged_atom
> This is just a change that reduces code duplication. It seems worth
> committing even if we reject all the other pathces. The next patches use
> the new function.
>
> - 0002: Update the rotation property whenever we change the rotation.
> This patch uses the "rotation" drm property to fix the Intel AMT bugs.
>
> - 0003: Add underscan properties
> This is the patch that implements underscan properties as connector
> properties instead of crtc properties.
>
> We still lack changes inside the sna/ directory. I'm waiting for the
> reviews on the non-sna code before writing the sna code. We also need to
> add a dependency on the "future" libdrm release that will include the
> new functions.
>
> --
> Paulo Zanoni



-- 
Paulo Zanoni