Re: [PATCH] staging: drm/omap: add rotation properties

2012-07-02 Thread Ville Syrjälä
On Fri, Jun 29, 2012 at 07:17:23AM -0500, Rob Clark wrote:
 On Fri, Jun 29, 2012 at 5:46 AM, Tomi Valkeinen tomi.valkei...@ti.com wrote:
  On Wed, 2012-06-27 at 09:06 -0500, Rob Clark wrote:
  From: Rob Clark r...@ti.com
 
  Use tiled buffers for rotated/reflected scanout, with CRTC and plane
  properties as the interface for userspace to configure rotation.
 
  Signed-off-by: Rob Clark r...@ti.com
 
  +/* this should probably be in drm-core to standardize amongst drivers */
  +#define DRM_ROTATE_0 0
  +#define DRM_ROTATE_90        1
  +#define DRM_ROTATE_180       2
  +#define DRM_ROTATE_270       3
  +#define DRM_REFLECT_X        4
  +#define DRM_REFLECT_Y        5
 
  Are both reflect X and Y needed? You can get all the possible
  orientations with just one of the reflects.
 
  And I think the word mirror represents nicely what the reflect does,
  i.e. if you look at the mirror, the image you see is flipped
  horizontally.
 
 fwiw these values are aligned with what is used in userspace xrandr..
 an earlier version of this patch used just 3 bits, which where aligned
 with what the omap hw uses and can describe all possible combinations
 of mirroring and isomorphic rotation (x-invert, y-invert, and
 xy-flip).  But the advantage of the xrandr approach is you can more
 easily leave out bits for rotation/mirroring modes that your hw does
 not support.. for example if some hw supports only certain rotations
 or does not support mirror/reflect.

When I hear mirror I always think of it as the happening after
rotation, but that's not how xrandr does things. I suppose reflection
has more or less the same connotation for me, but since it's already
used by xrandr, I know I have to do the necessary mental gymnastics.

If you think about it in terms of matrix multiplication, xrandr does
things like this: x' = Rot * Ref * x, whereas for me the more natural
order (for whatever reason) would be x' = Ref * Rot * x, where x is the
source coordinates, and x' the destination coordinates.

IIRC in dss mirroring was specified in the post-rotation way, and
the direction of the rotation was also opposite to xrandr (cw in dss,
ccw in xrandr).

So FWIW, my vote goes to reflect if we want to match xrandr
semantics. Also I agree on the number of bits, six is better than
three since it allows you to describe the hw capabilities.

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] staging: drm/omap: add rotation properties

2012-06-29 Thread Tomi Valkeinen
On Wed, 2012-06-27 at 09:06 -0500, Rob Clark wrote:
 From: Rob Clark r...@ti.com
 
 Use tiled buffers for rotated/reflected scanout, with CRTC and plane
 properties as the interface for userspace to configure rotation.
 
 Signed-off-by: Rob Clark r...@ti.com

 +/* this should probably be in drm-core to standardize amongst drivers */
 +#define DRM_ROTATE_0 0
 +#define DRM_ROTATE_901
 +#define DRM_ROTATE_180   2
 +#define DRM_ROTATE_270   3
 +#define DRM_REFLECT_X4
 +#define DRM_REFLECT_Y5

Are both reflect X and Y needed? You can get all the possible
orientations with just one of the reflects.

And I think the word mirror represents nicely what the reflect does,
i.e. if you look at the mirror, the image you see is flipped
horizontally.

 Tomi



signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] staging: drm/omap: add rotation properties

2012-06-29 Thread Rob Clark
On Fri, Jun 29, 2012 at 5:46 AM, Tomi Valkeinen tomi.valkei...@ti.com wrote:
 On Wed, 2012-06-27 at 09:06 -0500, Rob Clark wrote:
 From: Rob Clark r...@ti.com

 Use tiled buffers for rotated/reflected scanout, with CRTC and plane
 properties as the interface for userspace to configure rotation.

 Signed-off-by: Rob Clark r...@ti.com

 +/* this should probably be in drm-core to standardize amongst drivers */
 +#define DRM_ROTATE_0 0
 +#define DRM_ROTATE_90        1
 +#define DRM_ROTATE_180       2
 +#define DRM_ROTATE_270       3
 +#define DRM_REFLECT_X        4
 +#define DRM_REFLECT_Y        5

 Are both reflect X and Y needed? You can get all the possible
 orientations with just one of the reflects.

 And I think the word mirror represents nicely what the reflect does,
 i.e. if you look at the mirror, the image you see is flipped
 horizontally.

fwiw these values are aligned with what is used in userspace xrandr..
an earlier version of this patch used just 3 bits, which where aligned
with what the omap hw uses and can describe all possible combinations
of mirroring and isomorphic rotation (x-invert, y-invert, and
xy-flip).  But the advantage of the xrandr approach is you can more
easily leave out bits for rotation/mirroring modes that your hw does
not support.. for example if some hw supports only certain rotations
or does not support mirror/reflect.

BR,
-R

  Tomi

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] staging: drm/omap: add rotation properties

2012-06-27 Thread Rob Clark
On Wed, Jun 27, 2012 at 9:06 AM, Rob Clark rob.cl...@linaro.org wrote:
 From: Rob Clark r...@ti.com

 Use tiled buffers for rotated/reflected scanout, with CRTC and plane
 properties as the interface for userspace to configure rotation.

btw, I assume the potential controversial part of the patch would be
the property names/values exposed to userspace, because we probably
should try to standardize property names/values wherever possible.

There is one difference, AFAIU, from the rotation properties proposed
for the intel driver, in that in the omap case these properties are
actually instructing the hw to do the rotation, rather than just
informing the driver that rotation was done via a shadow buffer in
userspace.

BR,
-R

 Signed-off-by: Rob Clark r...@ti.com
 ---
  drivers/staging/omapdrm/omap_crtc.c      |   17 +
  drivers/staging/omapdrm/omap_dmm_tiler.c |   47 --
  drivers/staging/omapdrm/omap_dmm_tiler.h |   17 -
  drivers/staging/omapdrm/omap_drv.c       |   17 +
  drivers/staging/omapdrm/omap_drv.h       |   32 +-
  drivers/staging/omapdrm/omap_fb.c        |   99 
 +-
  drivers/staging/omapdrm/omap_gem.c       |   43 -
  drivers/staging/omapdrm/omap_plane.c     |   92 ---
  8 files changed, 318 insertions(+), 46 deletions(-)

 diff --git a/drivers/staging/omapdrm/omap_crtc.c 
 b/drivers/staging/omapdrm/omap_crtc.c
 index 8b864af..7479dcb 100644
 --- a/drivers/staging/omapdrm/omap_crtc.c
 +++ b/drivers/staging/omapdrm/omap_crtc.c
 @@ -191,10 +191,25 @@ static int omap_crtc_page_flip_locked(struct drm_crtc 
 *crtc,
        return 0;
  }

 +static int omap_crtc_set_property(struct drm_crtc *crtc,
 +               struct drm_property *property, uint64_t val)
 +{
 +       struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
 +       struct omap_drm_private *priv = crtc-dev-dev_private;
 +
 +       if (property == priv-rotation_prop) {
 +               crtc-invert_dimensions =
 +                               !!(val  ((1LL  DRM_ROTATE_90) | (1LL  
 DRM_ROTATE_270)));
 +       }
 +
 +       return omap_plane_set_property(omap_crtc-plane, property, val);
 +}
 +
  static const struct drm_crtc_funcs omap_crtc_funcs = {
        .set_config = drm_crtc_helper_set_config,
        .destroy = omap_crtc_destroy,
        .page_flip = omap_crtc_page_flip_locked,
 +       .set_property = omap_crtc_set_property,
  };

  static const struct drm_crtc_helper_funcs omap_crtc_helper_funcs = {
 @@ -231,6 +246,8 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
        drm_crtc_init(dev, crtc, omap_crtc_funcs);
        drm_crtc_helper_add(crtc, omap_crtc_helper_funcs);

 +       omap_plane_install_properties(omap_crtc-plane, crtc-base);
 +
        return crtc;

  fail:
 diff --git a/drivers/staging/omapdrm/omap_dmm_tiler.c 
 b/drivers/staging/omapdrm/omap_dmm_tiler.c
 index 9d83060..68da290 100644
 --- a/drivers/staging/omapdrm/omap_dmm_tiler.c
 +++ b/drivers/staging/omapdrm/omap_dmm_tiler.c
 @@ -401,8 +401,26 @@ int tiler_release(struct tiler_block *block)
  * Utils
  */

 -/* calculate the tiler space address of a pixel in a view orientation */
 -static u32 tiler_get_address(u32 orient, enum tiler_fmt fmt, u32 x, u32 y)
 +/* calculate the tiler space address of a pixel in a view orientation...
 + * below description copied from the display subsystem section of TRM:
 + *
 + * When the TILER is addressed, the bits:
 + *   [28:27] = 0x0 for 8-bit tiled
 + *             0x1 for 16-bit tiled
 + *             0x2 for 32-bit tiled
 + *             0x3 for page mode
 + *   [31:29] = 0x0 for 0-degree view
 + *             0x1 for 180-degree view + mirroring
 + *             0x2 for 0-degree view + mirroring
 + *             0x3 for 180-degree view
 + *             0x4 for 270-degree view + mirroring
 + *             0x5 for 270-degree view
 + *             0x6 for 90-degree view
 + *             0x7 for 90-degree view + mirroring
 + * Otherwise the bits indicated the corresponding bit address to access
 + * the SDRAM.
 + */
 +static u32 tiler_get_address(enum tiler_fmt fmt, u32 orient, u32 x, u32 y)
  {
        u32 x_bits, y_bits, tmp, x_mask, y_mask, alignment;

 @@ -414,8 +432,11 @@ static u32 tiler_get_address(u32 orient, enum tiler_fmt 
 fmt, u32 x, u32 y)
        x_mask = MASK(x_bits);
        y_mask = MASK(y_bits);

 -       if (x  0 || x  x_mask || y  0 || y  y_mask)
 +       if (x  0 || x  x_mask || y  0 || y  y_mask) {
 +               DBG(invalid coords: %u  0 || %u  %u || %u  0 || %u  %u,
 +                               x, x, x_mask, y, y, y_mask);
                return 0;
 +       }

        /* account for mirroring */
        if (orient  MASK_X_INVERT)
 @@ -436,11 +457,22 @@ dma_addr_t tiler_ssptr(struct tiler_block *block)
  {
        BUG_ON(!validfmt(block-fmt));

 -       return TILVIEW_8BIT + tiler_get_address(0, block-fmt,
 +       return TILVIEW_8BIT + tiler_get_address(block-fmt, 0,
                        

Re: [PATCH] staging: drm/omap: add rotation properties

2012-06-27 Thread Greg KH
On Wed, Jun 27, 2012 at 09:06:26AM -0500, Rob Clark wrote:
 From: Rob Clark r...@ti.com
 
 Use tiled buffers for rotated/reflected scanout, with CRTC and plane
 properties as the interface for userspace to configure rotation.
 
 Signed-off-by: Rob Clark r...@ti.com
 ---
  drivers/staging/omapdrm/omap_crtc.c  |   17 +
  drivers/staging/omapdrm/omap_dmm_tiler.c |   47 --
  drivers/staging/omapdrm/omap_dmm_tiler.h |   17 -
  drivers/staging/omapdrm/omap_drv.c   |   17 +
  drivers/staging/omapdrm/omap_drv.h   |   32 +-
  drivers/staging/omapdrm/omap_fb.c|   99 
 +-
  drivers/staging/omapdrm/omap_gem.c   |   43 -
  drivers/staging/omapdrm/omap_plane.c |   92 ---
  8 files changed, 318 insertions(+), 46 deletions(-)

That's great you are adding new features, but how goes the progress to
get this driver out of the drivers/staging/ area and moved to the real
part of the kernel?  I don't want to keep taking new features without
seeing some progress on getting the code cleaned up and moved out first.

thanks,

greg k-h
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] staging: drm/omap: add rotation properties

2012-06-27 Thread Rob Clark
On Wed, Jun 27, 2012 at 2:40 PM, Greg KH g...@kroah.com wrote:
 On Wed, Jun 27, 2012 at 09:06:26AM -0500, Rob Clark wrote:
 From: Rob Clark r...@ti.com

 Use tiled buffers for rotated/reflected scanout, with CRTC and plane
 properties as the interface for userspace to configure rotation.

 Signed-off-by: Rob Clark r...@ti.com
 ---
  drivers/staging/omapdrm/omap_crtc.c      |   17 +
  drivers/staging/omapdrm/omap_dmm_tiler.c |   47 --
  drivers/staging/omapdrm/omap_dmm_tiler.h |   17 -
  drivers/staging/omapdrm/omap_drv.c       |   17 +
  drivers/staging/omapdrm/omap_drv.h       |   32 +-
  drivers/staging/omapdrm/omap_fb.c        |   99 
 +-
  drivers/staging/omapdrm/omap_gem.c       |   43 -
  drivers/staging/omapdrm/omap_plane.c     |   92 ---
  8 files changed, 318 insertions(+), 46 deletions(-)

 That's great you are adding new features, but how goes the progress to
 get this driver out of the drivers/staging/ area and moved to the real
 part of the kernel?  I don't want to keep taking new features without
 seeing some progress on getting the code cleaned up and moved out first.

Oh, heh, well I suppose the first thing to do is send a patch to clean
up the TODO file.. I just noticed that I'd been forgetting to update
it.

BR,
-R


 thanks,

 greg k-h
 --
 To unsubscribe from this list: send the line unsubscribe linux-omap in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel