| Message ID | 1362768363-3710-3-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 08, 2013 at 10:45:46AM -0800, Jesse Barnes wrote: > And implement it on ValleyView. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_sprite.c | 11 ++++++++++- > include/uapi/drm/i915_drm.h | 1 + > 3 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 669a61c..6a7e424 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3287,6 +3287,7 @@ > #define _SPAKEYMAXVAL 0x721a0 > #define _SPATILEOFF 0x721a4 > #define _SPACONSTALPHA 0x721a8 > +#define SP_ALPHA_EN (1<<31) > #define _SPAGAMC 0x721f4 > > #define _SPBCNTR 0x72280 > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 26fee2f..a6a49f6 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -162,7 +162,8 @@ vlv_update_colorkey(struct drm_plane *dplane, > int plane = intel_plane->plane; > u32 sprctl; > > - if (key->flags & I915_SET_COLORKEY_DESTINATION) > + if (!(key->flags & > + (I915_SET_COLORKEY_SOURCE | I915_SET_COLORKEY_ALPHA))) > return -EINVAL; > > I915_WRITE(SPKEYMINVAL(pipe, plane), key->min_value); > @@ -171,8 +172,16 @@ vlv_update_colorkey(struct drm_plane *dplane, > > sprctl = I915_READ(SPCNTR(pipe, plane)); > sprctl &= ~SP_SOURCE_KEY; > + > + if (!(key->flags & I915_SET_COLORKEY_ALPHA)) > + I915_WRITE(SPCONSTALPHA(pipe, plane), 0); > + > if (key->flags & I915_SET_COLORKEY_SOURCE) > sprctl |= SP_SOURCE_KEY; > + else if (key->flags & I915_SET_COLORKEY_ALPHA) { > + I915_WRITE(SPCONSTALPHA(pipe, plane), > + SP_ALPHA_EN | key->channel_mask); > + } > I915_WRITE(SPCNTR(pipe, plane), sprctl); > > POSTING_READ(SPKEYMSK(pipe, plane)); > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 07d5941..05e61bc 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -949,6 +949,7 @@ struct drm_intel_overlay_attrs { > #define I915_SET_COLORKEY_NONE (1<<0) /* disable color key matching */ > #define I915_SET_COLORKEY_DESTINATION (1<<1) > #define I915_SET_COLORKEY_SOURCE (1<<2) > +#define I915_SET_COLORKEY_ALPHA (1<<3) We've added this driver-private ioctl before attributes everywhere was possible. I think we need to convert that before adding more stuff ... Also some discussion on dri-devel about a somewhat standardized set of plane blending properties can't hurt. Yes, I know that this will lead to a massive dri-devel bikeshed ;-) -Daniel
On Tue, 19 Mar 2013 09:42:56 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > > --- a/include/uapi/drm/i915_drm.h > > +++ b/include/uapi/drm/i915_drm.h > > @@ -949,6 +949,7 @@ struct drm_intel_overlay_attrs { > > #define I915_SET_COLORKEY_NONE (1<<0) /* disable color key matching */ > > #define I915_SET_COLORKEY_DESTINATION (1<<1) > > #define I915_SET_COLORKEY_SOURCE (1<<2) > > +#define I915_SET_COLORKEY_ALPHA (1<<3) > > We've added this driver-private ioctl before attributes everywhere was > possible. I think we need to convert that before adding more stuff ... > > Also some discussion on dri-devel about a somewhat standardized set of > plane blending properties can't hurt. > > Yes, I know that this will lead to a massive dri-devel bikeshed ;-) Yeah we need some properties for this to fit in with the atomic bits. This patch actually pre-dates that by a bit (it's very old)... I'm sure Ville has ideas on what he'd like to see.
On Tue, Mar 19, 2013 at 10:57:25AM -0700, Jesse Barnes wrote: > On Tue, 19 Mar 2013 09:42:56 +0100 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > --- a/include/uapi/drm/i915_drm.h > > > +++ b/include/uapi/drm/i915_drm.h > > > @@ -949,6 +949,7 @@ struct drm_intel_overlay_attrs { > > > #define I915_SET_COLORKEY_NONE (1<<0) /* disable color key matching */ > > > #define I915_SET_COLORKEY_DESTINATION (1<<1) > > > #define I915_SET_COLORKEY_SOURCE (1<<2) > > > +#define I915_SET_COLORKEY_ALPHA (1<<3) > > > > We've added this driver-private ioctl before attributes everywhere was > > possible. I think we need to convert that before adding more stuff ... > > > > Also some discussion on dri-devel about a somewhat standardized set of > > plane blending properties can't hurt. > > > > Yes, I know that this will lead to a massive dri-devel bikeshed ;-) > > Yeah we need some properties for this to fit in with the atomic bits. > This patch actually pre-dates that by a bit (it's very old)... > > I'm sure Ville has ideas on what he'd like to see. Yeah, I've been pondering about this kind of stuff. One idea was that we might want to make the props for color key/mask, background color, const alpha etc. always use 16bpc, and then we can just drop the least significant bits if the HW uses less precision. And also fix the channel order to some common standard. That might make it a bit easier to write generic user space code. But I'm not sure how to deal w/ RGB vs. YCbCr. Sometimes you may need to feed hardware RGB values, sometimes YCbCr. Options: 1. use one prop but the driver will interpret the values as RGB or YCbCr as needed. Userspace just has to figure out what to stuff into prop somehow. 2. one prop for RGB, another one for YCbCr. If the hw has need for both, it'll pick the right one. Again userspace just has to figure out which one it should use at a given time 3. standardize on RGB and convert to YCbCr in the driver as needed As for the per-pixel alpha, I'm not sure if relying on the pixel format alone is the best idea. At least we need a way to tell the premult and non-premult cases apart. But maybe we want to even allow disabling per-pixel alpha for ARGB formats, so that we don't need to create an XRGB fb for the same data. So maybe an enum prop w/ "no per-pixel alpha", "pre-mult", "non-premult" options.
On Wed, Mar 20, 2013 at 06:32:00PM +0200, Ville Syrjälä wrote: > On Tue, Mar 19, 2013 at 10:57:25AM -0700, Jesse Barnes wrote: > > On Tue, 19 Mar 2013 09:42:56 +0100 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > --- a/include/uapi/drm/i915_drm.h > > > > +++ b/include/uapi/drm/i915_drm.h > > > > @@ -949,6 +949,7 @@ struct drm_intel_overlay_attrs { > > > > #define I915_SET_COLORKEY_NONE (1<<0) /* disable color key matching */ > > > > #define I915_SET_COLORKEY_DESTINATION (1<<1) > > > > #define I915_SET_COLORKEY_SOURCE (1<<2) > > > > +#define I915_SET_COLORKEY_ALPHA (1<<3) > > > > > > We've added this driver-private ioctl before attributes everywhere was > > > possible. I think we need to convert that before adding more stuff ... > > > > > > Also some discussion on dri-devel about a somewhat standardized set of > > > plane blending properties can't hurt. > > > > > > Yes, I know that this will lead to a massive dri-devel bikeshed ;-) > > > > Yeah we need some properties for this to fit in with the atomic bits. > > This patch actually pre-dates that by a bit (it's very old)... > > > > I'm sure Ville has ideas on what he'd like to see. > > Yeah, I've been pondering about this kind of stuff. > > One idea was that we might want to make the props for > color key/mask, background color, const alpha etc. always use 16bpc, > and then we can just drop the least significant bits if the HW uses > less precision. And also fix the channel order to some common standard. > That might make it a bit easier to write generic user space code. > > But I'm not sure how to deal w/ RGB vs. YCbCr. Sometimes you may > need to feed hardware RGB values, sometimes YCbCr. > > Options: > 1. use one prop but the driver will interpret the values as RGB or > YCbCr as needed. Userspace just has to figure out what to stuff > into prop somehow. > 2. one prop for RGB, another one for YCbCr. If the hw has need for > both, it'll pick the right one. Again userspace just has to figure > out which one it should use at a given time > 3. standardize on RGB and convert to YCbCr in the driver as needed I'm leaning towards 3 for now, since currently no driver supports ycbcr passthrough. If conversion would be too imprecise (e.g. for color keys) we could add new attributes later on (i.e. do 2). > As for the per-pixel alpha, I'm not sure if relying on the pixel > format alone is the best idea. At least we need a way to tell the > premult and non-premult cases apart. But maybe we want to even allow > disabling per-pixel alpha for ARGB formats, so that we don't need > to create an XRGB fb for the same data. So maybe an enum prop w/ > "no per-pixel alpha", "pre-mult", "non-premult" options. Creating different fb views for xrgb and argb with the same backing storage feels like the right way for me. But yeah, we need some agreement on how to expose constant alpha and whether the argb data is pre-multiplied or not. I kinda wonder whether we shouldn't just copy&paste the gl1 texture blend state stuff ... Although iirc that passes on colorkeys. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 669a61c..6a7e424 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3287,6 +3287,7 @@ #define _SPAKEYMAXVAL 0x721a0 #define _SPATILEOFF 0x721a4 #define _SPACONSTALPHA 0x721a8 +#define SP_ALPHA_EN (1<<31) #define _SPAGAMC 0x721f4 #define _SPBCNTR 0x72280 diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 26fee2f..a6a49f6 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -162,7 +162,8 @@ vlv_update_colorkey(struct drm_plane *dplane, int plane = intel_plane->plane; u32 sprctl; - if (key->flags & I915_SET_COLORKEY_DESTINATION) + if (!(key->flags & + (I915_SET_COLORKEY_SOURCE | I915_SET_COLORKEY_ALPHA))) return -EINVAL; I915_WRITE(SPKEYMINVAL(pipe, plane), key->min_value); @@ -171,8 +172,16 @@ vlv_update_colorkey(struct drm_plane *dplane, sprctl = I915_READ(SPCNTR(pipe, plane)); sprctl &= ~SP_SOURCE_KEY; + + if (!(key->flags & I915_SET_COLORKEY_ALPHA)) + I915_WRITE(SPCONSTALPHA(pipe, plane), 0); + if (key->flags & I915_SET_COLORKEY_SOURCE) sprctl |= SP_SOURCE_KEY; + else if (key->flags & I915_SET_COLORKEY_ALPHA) { + I915_WRITE(SPCONSTALPHA(pipe, plane), + SP_ALPHA_EN | key->channel_mask); + } I915_WRITE(SPCNTR(pipe, plane), sprctl); POSTING_READ(SPKEYMSK(pipe, plane)); diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 07d5941..05e61bc 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -949,6 +949,7 @@ struct drm_intel_overlay_attrs { #define I915_SET_COLORKEY_NONE (1<<0) /* disable color key matching */ #define I915_SET_COLORKEY_DESTINATION (1<<1) #define I915_SET_COLORKEY_SOURCE (1<<2) +#define I915_SET_COLORKEY_ALPHA (1<<3) struct drm_intel_sprite_colorkey { __u32 plane_id; __u32 min_value;
And implement it on ValleyView. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_sprite.c | 11 ++++++++++- include/uapi/drm/i915_drm.h | 1 + 3 files changed, 12 insertions(+), 1 deletion(-)