[RFCv3 12/14] drm: Specify cursor plane at CRTC initialization

2014-04-07 Thread Daniel Vetter 
On Mon, Apr 7, 2014 at 7:23 PM, Rob Clark  wrote:
> On Mon, Apr 7, 2014 at 6:05 AM, Thierry Reding  
> wrote:
>> On Fri, Mar 28, 2014 at 10:04:09PM +0100, Daniel Vetter wrote:
>>> On Tue, Mar 18, 2014 at 05:22:57PM -0700, Matt Roper wrote:
>>> > Add cursor plane as a parameter to drm_crtc_init() and update all
>>> > existing DRM drivers to use a helper-provided primary plane.  Passing
>>> > NULL for this parameter indicates that there is no hardware cursor
>>> > supported by the driver and no cursor plane should be provided to
>>> > userspace.
>>> >
>>> > Signed-off-by: Matt Roper 
>>>
>>> Ok, cursor planes. I've poked around in this a lot and unfortunately I
>>> don't think we can achieve nirvana :(
>> [...]
>>> - For the pixel format I think it's ok to always assume RGBA.
>>
>> How so? We do have hardware on Tegra that doesn't support RGBA cursors
>> and if at all possible I'd very much like to support that as well. New
>> generations can do RGBA but still support the old pixel format. Having
>> the option of supporting all formats would be nice.
>>
>> There was some discussion about implementing a bunch of plane properties
>> so I think having one to enumerate a set of pixel formats wouldn't be
>> such a big deal.
>
> No need for properties to expose formats, we already have that in
> getplane ioctl.  But would be nice to support more than just RGBA.

This is _just_ for the compat cursor plane setup helper. Of course we
can add whatever insane cursor formats exist out there as fourcc codes
and then drivers can add them for their custom-created cursor planes.
This is similar to the current primary plane helper code which only
exposes 32bit XRGB atm. If you want to take full advantage of this you
need to do a bit of work in your driver ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFCv3 12/14] drm: Specify cursor plane at CRTC initialization

2014-04-07 Thread Rob Clark 
On Mon, Apr 7, 2014 at 4:03 PM, Daniel Vetter  wrote:
> On Mon, Apr 7, 2014 at 7:23 PM, Rob Clark  wrote:
>> On Mon, Apr 7, 2014 at 6:05 AM, Thierry Reding  
>> wrote:
>>> On Fri, Mar 28, 2014 at 10:04:09PM +0100, Daniel Vetter wrote:
 On Tue, Mar 18, 2014 at 05:22:57PM -0700, Matt Roper wrote:
 > Add cursor plane as a parameter to drm_crtc_init() and update all
 > existing DRM drivers to use a helper-provided primary plane.  Passing
 > NULL for this parameter indicates that there is no hardware cursor
 > supported by the driver and no cursor plane should be provided to
 > userspace.
 >
 > Signed-off-by: Matt Roper 

 Ok, cursor planes. I've poked around in this a lot and unfortunately I
 don't think we can achieve nirvana :(
>>> [...]
 - For the pixel format I think it's ok to always assume RGBA.
>>>
>>> How so? We do have hardware on Tegra that doesn't support RGBA cursors
>>> and if at all possible I'd very much like to support that as well. New
>>> generations can do RGBA but still support the old pixel format. Having
>>> the option of supporting all formats would be nice.
>>>
>>> There was some discussion about implementing a bunch of plane properties
>>> so I think having one to enumerate a set of pixel formats wouldn't be
>>> such a big deal.
>>
>> No need for properties to expose formats, we already have that in
>> getplane ioctl.  But would be nice to support more than just RGBA.
>
> This is _just_ for the compat cursor plane setup helper. Of course we
> can add whatever insane cursor formats exist out there as fourcc codes
> and then drivers can add them for their custom-created cursor planes.
> This is similar to the current primary plane helper code which only
> exposes 32bit XRGB atm. If you want to take full advantage of this you
> need to do a bit of work in your driver ;-)

oh, heh.. should have read the whole thread first :-P

yeah, for compat helpers, only ARGB makes sense

BR,
-R


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFCv3 12/14] drm: Specify cursor plane at CRTC initialization

2014-04-07 Thread Rob Clark 
On Mon, Apr 7, 2014 at 6:05 AM, Thierry Reding  
wrote:
> On Fri, Mar 28, 2014 at 10:04:09PM +0100, Daniel Vetter wrote:
>> On Tue, Mar 18, 2014 at 05:22:57PM -0700, Matt Roper wrote:
>> > Add cursor plane as a parameter to drm_crtc_init() and update all
>> > existing DRM drivers to use a helper-provided primary plane.  Passing
>> > NULL for this parameter indicates that there is no hardware cursor
>> > supported by the driver and no cursor plane should be provided to
>> > userspace.
>> >
>> > Signed-off-by: Matt Roper 
>>
>> Ok, cursor planes. I've poked around in this a lot and unfortunately I
>> don't think we can achieve nirvana :(
> [...]
>> - For the pixel format I think it's ok to always assume RGBA.
>
> How so? We do have hardware on Tegra that doesn't support RGBA cursors
> and if at all possible I'd very much like to support that as well. New
> generations can do RGBA but still support the old pixel format. Having
> the option of supporting all formats would be nice.
>
> There was some discussion about implementing a bunch of plane properties
> so I think having one to enumerate a set of pixel formats wouldn't be
> such a big deal.

No need for properties to expose formats, we already have that in
getplane ioctl.  But would be nice to support more than just RGBA.

BR,
-R

> Thierry
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[RFCv3 12/14] drm: Specify cursor plane at CRTC initialization

2014-04-07 Thread Thierry Reding 
On Fri, Mar 28, 2014 at 10:04:09PM +0100, Daniel Vetter wrote:
> On Tue, Mar 18, 2014 at 05:22:57PM -0700, Matt Roper wrote:
> > Add cursor plane as a parameter to drm_crtc_init() and update all
> > existing DRM drivers to use a helper-provided primary plane.  Passing
> > NULL for this parameter indicates that there is no hardware cursor
> > supported by the driver and no cursor plane should be provided to
> > userspace.
> > 
> > Signed-off-by: Matt Roper 
> 
> Ok, cursor planes. I've poked around in this a lot and unfortunately I
> don't think we can achieve nirvana :(
[...]
> - For the pixel format I think it's ok to always assume RGBA.

How so? We do have hardware on Tegra that doesn't support RGBA cursors
and if at all possible I'd very much like to support that as well. New
generations can do RGBA but still support the old pixel format. Having
the option of supporting all formats would be nice.

There was some discussion about implementing a bunch of plane properties
so I think having one to enumerate a set of pixel formats wouldn't be
such a big deal.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:

[RFCv3 12/14] drm: Specify cursor plane at CRTC initialization

2014-03-28 Thread Daniel Vetter 
On Tue, Mar 18, 2014 at 05:22:57PM -0700, Matt Roper wrote:
> Add cursor plane as a parameter to drm_crtc_init() and update all
> existing DRM drivers to use a helper-provided primary plane.  Passing
> NULL for this parameter indicates that there is no hardware cursor
> supported by the driver and no cursor plane should be provided to
> userspace.
> 
> Signed-off-by: Matt Roper 

Ok, cursor planes. I've poked around in this a lot and unfortunately I
don't think we can achieve nirvana :(

The first direction is automatically getting cursor plane support from
existing drivers using the existing callbacks. The irky thing is that we
don't have any means to sanely unwrap the framebuffer since both the bo
handle used by the legacy cursor ioctl and how a framebuffer would wrap
such a handle isn't generic. And e.g. vmwgfx doesn't even use gem for
those.

So I think we'll have to give up on that and drivers which want to expose
cursors with the atomic ioctls simply have to have proper support.

The other direction is that converted drivers don't need to have support
for legacy ioctls should be possible and is imo something we want - at
least for i915 I don't want to carry around two interfaces doing the same.

Which means we need some way to forward the legacy cursor ioctls to the
new plane callbacks exposed when using cursor planes. Unfortunately we
have a bit an api mismatch between the legacy cursor interfaces:

int (*cursor_set2)(struct drm_crtc *crtc, struct drm_file *file_priv,
   uint32_t handle, uint32_t width, uint32_t height,
   int32_t hot_x, int32_t hot_y);
int (*cursor_move)(struct drm_crtc *crtc, int x, int y);

And the plane interfaces:

int (*update_plane)(struct drm_plane *plane,
struct drm_crtc *crtc, struct drm_framebuffer *fb,
int crtc_x, int crtc_y,
unsigned int crtc_w, unsigned int crtc_h,
uint32_t src_x, uint32_t src_y,
uint32_t src_w, uint32_t src_h);
int (*disable_plane)(struct drm_plane *plane);

cursor_set2 with handle == 0 simply maps to a disable_plane call. But any
other call of the legacy ioctls simply maps to an update_plane, but we
don't have all the information available all the time.

- width, height and handle isn't a real concern - we can just wrap this
  all up into a drm private drm framebuffer. As long as the only reference
  to that framebuffer is held by crtc->cursor->fb it will also get cleaned
  up at the right time and so won't extend the lifetime of the underlying
  buffer object. And we don't need to cache width/height anywhere since
  they're accessible through crtc->cursor->fb.

- For the pixel format I think it's ok to always assume RGBA.

- hot_x and hot_y can simply be mapped to new plane properties. I think
  it'd be good for this to be generally available, just to have a
  consistent interface.

- The x/y coordinates of cursor_move are more annoying - userspace
  potentially doesn't supply them, so we need to cache them in the crtc
  struct somewhere. Originally I've thought it would be good to have a
  special struct drm_cursor_plane and use that as the blessed cursor plane
  object in drm_crtc_init_with_planes. But that will wreak havoc with hw
  platforms which have fully unified planes and want to use the same
  struct everywhere. So I think we need to add crtc->cursor_x/y fields.

With these bits we should be able to always create a valid call to
update_plane. Which allows drivers to not implement the legacy
cursor_set/move interface. Of course we also need to go through the drm
core to make sure that all the cursor code also works when crtc->cursor is
set.

To make driver writers life as easy as possible I think we should provide
a default helper for their cursor_update_plane callback which checks that
the update_plane call is indeed valid for a cursor:

- Checks that widht == pitch*bpp since that's what we assume with cursors.

- Checks that there's no scaling going on.

- Checks that the viewport matches the full cursor size.

- Checks that there's no subpixel positioning.

With that the only thing drivers need to do is:
- Kill the handle to bo pointer lookup, just use the one wrapped up in the
  framebuffer object.
- Call the above helper.
- Call into the low-level set_cursor_bo/move_cursor functions - all the
  arguments of the old ioctls have a 1:1 mapping in update_plane, so this
  should be simple.

... and of course they need to set up the cursor plane object.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFCv3 12/14] drm: Specify cursor plane at CRTC initialization

2014-03-18 Thread Matt Roper 
Add cursor plane as a parameter to drm_crtc_init() and update all
existing DRM drivers to use a helper-provided primary plane.  Passing
NULL for this parameter indicates that there is no hardware cursor
supported by the driver and no cursor plane should be provided to
userspace.

Signed-off-by: Matt Roper 
---
 drivers/gpu/drm/armada/armada_crtc.c   | 2 +-
 drivers/gpu/drm/ast/ast_mode.c | 2 +-
 drivers/gpu/drm/bochs/bochs_kms.c  | 2 +-
 drivers/gpu/drm/cirrus/cirrus_mode.c   | 3 ++-
 drivers/gpu/drm/drm_crtc.c | 6 ++
 drivers/gpu/drm/exynos/exynos_drm_crtc.c   | 2 +-
 drivers/gpu/drm/gma500/psb_intel_display.c | 3 ++-
 drivers/gpu/drm/i915/intel_display.c   | 3 ++-
 drivers/gpu/drm/mgag200/mgag200_mode.c | 3 ++-
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c   | 2 +-
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c   | 2 +-
 drivers/gpu/drm/nouveau/dispnv04/crtc.c| 2 +-
 drivers/gpu/drm/nouveau/nv50_display.c | 2 +-
 drivers/gpu/drm/omapdrm/omap_crtc.c| 2 +-
 drivers/gpu/drm/qxl/qxl_display.c  | 2 +-
 drivers/gpu/drm/radeon/radeon_display.c| 3 ++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
 drivers/gpu/drm/shmobile/shmob_drm_crtc.c  | 2 +-
 drivers/gpu/drm/tegra/dc.c | 2 +-
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c   | 2 +-
 drivers/gpu/drm/udl/udl_modeset.c  | 2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c| 2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c   | 2 +-
 drivers/staging/imx-drm/imx-drm-core.c | 2 +-
 include/drm/drm_crtc.h | 6 ++
 25 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_crtc.c 
b/drivers/gpu/drm/armada/armada_crtc.c
index 0a14d24..a6eec51 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -1088,7 +1088,7 @@ int armada_drm_crtc_create(struct drm_device *dev, 
unsigned num,
priv->dcrtc[dcrtc->num] = dcrtc;

primary = drm_primary_helper_create_plane(dev);
-   drm_crtc_init(dev, >crtc, primary, _crtc_funcs);
+   drm_crtc_init(dev, >crtc, primary, NULL, _crtc_funcs);
drm_crtc_helper_add(>crtc, _crtc_helper_funcs);

drm_object_attach_property(>crtc.base, priv->csc_yuv_prop,
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index bd1e156..52415ca 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -634,7 +634,7 @@ static int ast_crtc_init(struct drm_device *dev)
return -ENOMEM;

primary = drm_primary_helper_create_plane(dev);
-   drm_crtc_init(dev, >base, primary, _crtc_funcs);
+   drm_crtc_init(dev, >base, primary, NULL, _crtc_funcs);
drm_mode_crtc_set_gamma_size(>base, 256);
drm_crtc_helper_add(>base, _crtc_helper_funcs);

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index b67ad12..e88101a 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -132,7 +132,7 @@ static void bochs_crtc_init(struct drm_device *dev)
struct drm_plane *primary;

primary = drm_primary_helper_create_plane(dev);
-   drm_crtc_init(dev, crtc, primary, _crtc_funcs);
+   drm_crtc_init(dev, crtc, primary, NULL, _crtc_funcs);
drm_mode_crtc_set_gamma_size(crtc, 256);
drm_crtc_helper_add(crtc, _helper_funcs);
 }
diff --git a/drivers/gpu/drm/cirrus/cirrus_mode.c 
b/drivers/gpu/drm/cirrus/cirrus_mode.c
index 5291d2f..04a002b 100644
--- a/drivers/gpu/drm/cirrus/cirrus_mode.c
+++ b/drivers/gpu/drm/cirrus/cirrus_mode.c
@@ -392,7 +392,8 @@ static void cirrus_crtc_init(struct drm_device *dev)
return;

primary = drm_primary_helper_create_plane(dev);
-   drm_crtc_init(dev, _crtc->base, primary, _crtc_funcs);
+   drm_crtc_init(dev, _crtc->base, primary, NULL,
+ _crtc_funcs);

drm_mode_crtc_set_gamma_size(_crtc->base, CIRRUS_LUT_SIZE);
cdev->mode_info.crtc = cirrus_crtc;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fb8e493..e6e1f39 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -683,6 +683,7 @@ EXPORT_SYMBOL(drm_framebuffer_remove);
  * @dev: DRM device
  * @crtc: CRTC object to init
  * @primary: Primary plane for CRTC
+ * @cursor: Cursor plane for CRTC; may be NULL if no hardware cursor exists
  * @funcs: callbacks for the new CRTC
  *
  * Inits a new object created as base part of a driver crtc object.
@@ -692,6 +693,7 @@ EXPORT_SYMBOL(drm_framebuffer_remove);
  */
 int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
  struct drm_plane *primary,
+ struct drm_plane *cursor,
  const struct drm_crtc_funcs *funcs)
 {
int ret;
@@ -716,6 +718,10 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc 
*crtc,
crtc->primary = primary;