| Message ID | cover.713e47b83733ed05f0c38f5ba6ef8b6c1e32805c.1515494838.git-series.maxime.ripard@free-electrons.com |
|---|---|
| Headers | show |
| Series | drm/sun4i: Support more planes, zpos and plane-wide alpha | expand |
On Tue, 9 Jan 2018 11:56:20 +0100 Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > There's a bunch of drivers that duplicate the same function to know if a > particular format embeds an alpha component or not. > > Let's create a helper to avoid duplicating that logic. > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Eric Anholt <eric@anholt.net> > Cc: Inki Dae <inki.dae@samsung.com> > Cc: Joonyoung Shim <jy0922.shim@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Mark Yao <mark.yao@rock-chips.com> > Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/gpu/drm/drm_fourcc.c | 43 +++++++++++++++++++++++++++++++++++++- > include/drm/drm_fourcc.h | 1 +- > 2 files changed, 44 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > index 9c0152df45ad..6e6227d6a46b 100644 > --- a/drivers/gpu/drm/drm_fourcc.c > +++ b/drivers/gpu/drm/drm_fourcc.c > @@ -348,3 +348,46 @@ int drm_format_plane_height(int height, uint32_t format, int plane) > return height / info->vsub; > } > EXPORT_SYMBOL(drm_format_plane_height); > + > +/** > + * drm_format_has_alpha - get whether the format embeds an alpha component > + * @format: pixel format (DRM_FORMAT_*) > + * > + * Returns: > + * true if the format embeds an alpha component, false otherwise. > + */ > +bool drm_format_has_alpha(uint32_t format) > +{ > + switch (format) { > + case DRM_FORMAT_ARGB4444: > + case DRM_FORMAT_ABGR4444: > + case DRM_FORMAT_RGBA4444: > + case DRM_FORMAT_BGRA4444: > + case DRM_FORMAT_ARGB1555: > + case DRM_FORMAT_ABGR1555: > + case DRM_FORMAT_RGBA5551: > + case DRM_FORMAT_BGRA5551: > + case DRM_FORMAT_ARGB8888: > + case DRM_FORMAT_ABGR8888: > + case DRM_FORMAT_RGBA8888: > + case DRM_FORMAT_BGRA8888: > + case DRM_FORMAT_ARGB2101010: > + case DRM_FORMAT_ABGR2101010: > + case DRM_FORMAT_RGBA1010102: > + case DRM_FORMAT_BGRA1010102: > + case DRM_FORMAT_AYUV: > + case DRM_FORMAT_XRGB8888_A8: > + case DRM_FORMAT_XBGR8888_A8: > + case DRM_FORMAT_RGBX8888_A8: > + case DRM_FORMAT_BGRX8888_A8: > + case DRM_FORMAT_RGB888_A8: > + case DRM_FORMAT_BGR888_A8: > + case DRM_FORMAT_RGB565_A8: > + case DRM_FORMAT_BGR565_A8: > + return true; > + > + default: > + return false; > + } > +} > +EXPORT_SYMBOL(drm_format_has_alpha); > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h > index 6942e84b6edd..e08fc22c5f78 100644 > --- a/include/drm/drm_fourcc.h > +++ b/include/drm/drm_fourcc.h > @@ -69,5 +69,6 @@ int drm_format_vert_chroma_subsampling(uint32_t format); > int drm_format_plane_width(int width, uint32_t format, int plane); > int drm_format_plane_height(int height, uint32_t format, int plane); > const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf); > +bool drm_format_has_alpha(uint32_t format); > > #endif /* __DRM_FOURCC_H__ */
On Tue, 9 Jan 2018 11:56:21 +0100 Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Now that the core has a drm format helper to tell if a format embeds an > alpha component in it, let's use it. > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 20 ++---------------- > 1 file changed, 3 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > index 703c2d13603f..1a9318810a29 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > @@ -194,20 +194,6 @@ static int atmel_hlcdc_format_to_plane_mode(u32 format, u32 *mode) > return 0; > } > > -static bool atmel_hlcdc_format_embeds_alpha(u32 format) > -{ > - int i; > - > - for (i = 0; i < sizeof(format); i++) { > - char tmp = (format >> (8 * i)) & 0xff; > - > - if (tmp == 'A') > - return true; > - } > - > - return false; > -} > - > static u32 heo_downscaling_xcoef[] = { > 0x11343311, > 0x000000f7, > @@ -395,7 +381,7 @@ atmel_hlcdc_plane_update_general_settings(struct atmel_hlcdc_plane *plane, > cfg |= ATMEL_HLCDC_LAYER_OVR | ATMEL_HLCDC_LAYER_ITER2BL | > ATMEL_HLCDC_LAYER_ITER; > > - if (atmel_hlcdc_format_embeds_alpha(format)) > + if (drm_format_has_alpha(format)) > cfg |= ATMEL_HLCDC_LAYER_LAEN; > else > cfg |= ATMEL_HLCDC_LAYER_GAEN | > @@ -566,7 +552,7 @@ atmel_hlcdc_plane_prepare_disc_area(struct drm_crtc_state *c_state) > ovl_state = drm_plane_state_to_atmel_hlcdc_plane_state(ovl_s); > > if (!ovl_s->fb || > - atmel_hlcdc_format_embeds_alpha(ovl_s->fb->format->format) || > + drm_format_has_alpha(ovl_s->fb->format->format) || > ovl_state->alpha != 255) > continue; > > @@ -769,7 +755,7 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p, > > if ((state->crtc_h != state->src_h || state->crtc_w != state->src_w) && > (!desc->layout.memsize || > - atmel_hlcdc_format_embeds_alpha(state->base.fb->format->format))) > + drm_format_has_alpha(state->base.fb->format->format))) > return -EINVAL; > > if (state->crtc_x < 0 || state->crtc_y < 0)
Hi Maxime, Thank you for the patch. On Tuesday, 9 January 2018 12:56:20 EET Maxime Ripard wrote: > There's a bunch of drivers that duplicate the same function to know if a > particular format embeds an alpha component or not. > > Let's create a helper to avoid duplicating that logic. > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Eric Anholt <eric@anholt.net> > Cc: Inki Dae <inki.dae@samsung.com> > Cc: Joonyoung Shim <jy0922.shim@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Mark Yao <mark.yao@rock-chips.com> > Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/gpu/drm/drm_fourcc.c | 43 +++++++++++++++++++++++++++++++++++++- > include/drm/drm_fourcc.h | 1 +- > 2 files changed, 44 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > index 9c0152df45ad..6e6227d6a46b 100644 > --- a/drivers/gpu/drm/drm_fourcc.c > +++ b/drivers/gpu/drm/drm_fourcc.c > @@ -348,3 +348,46 @@ int drm_format_plane_height(int height, uint32_t > format, int plane) return height / info->vsub; > } > EXPORT_SYMBOL(drm_format_plane_height); > + > +/** > + * drm_format_has_alpha - get whether the format embeds an alpha component > + * @format: pixel format (DRM_FORMAT_*) > + * > + * Returns: > + * true if the format embeds an alpha component, false otherwise. > + */ > +bool drm_format_has_alpha(uint32_t format) > +{ > + switch (format) { > + case DRM_FORMAT_ARGB4444: > + case DRM_FORMAT_ABGR4444: > + case DRM_FORMAT_RGBA4444: > + case DRM_FORMAT_BGRA4444: > + case DRM_FORMAT_ARGB1555: > + case DRM_FORMAT_ABGR1555: > + case DRM_FORMAT_RGBA5551: > + case DRM_FORMAT_BGRA5551: > + case DRM_FORMAT_ARGB8888: > + case DRM_FORMAT_ABGR8888: > + case DRM_FORMAT_RGBA8888: > + case DRM_FORMAT_BGRA8888: > + case DRM_FORMAT_ARGB2101010: > + case DRM_FORMAT_ABGR2101010: > + case DRM_FORMAT_RGBA1010102: > + case DRM_FORMAT_BGRA1010102: > + case DRM_FORMAT_AYUV: > + case DRM_FORMAT_XRGB8888_A8: > + case DRM_FORMAT_XBGR8888_A8: > + case DRM_FORMAT_RGBX8888_A8: > + case DRM_FORMAT_BGRX8888_A8: > + case DRM_FORMAT_RGB888_A8: > + case DRM_FORMAT_BGR888_A8: > + case DRM_FORMAT_RGB565_A8: > + case DRM_FORMAT_BGR565_A8: > + return true; > + > + default: > + return false; > + } > +} > +EXPORT_SYMBOL(drm_format_has_alpha); How about adding the information to struct drm_format_info instead ? drm_format_has_alpha() could then be implemented as bool drm_format_has_alpha(uint32_t format) { const struct drm_format_info *info; info = drm_format_info(format); return info ? info->has_alpha : false; } although drivers should really use the drm_framebuffer::format field directly in most cases, so the helper might not be needed at all. > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h > index 6942e84b6edd..e08fc22c5f78 100644 > --- a/include/drm/drm_fourcc.h > +++ b/include/drm/drm_fourcc.h > @@ -69,5 +69,6 @@ int drm_format_vert_chroma_subsampling(uint32_t format); > int drm_format_plane_width(int width, uint32_t format, int plane); > int drm_format_plane_height(int height, uint32_t format, int plane); > const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf > *buf); > +bool drm_format_has_alpha(uint32_t format); > > #endif /* __DRM_FOURCC_H__ */
On Tue, 9 Jan 2018 11:56:26 +0100 Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Now that we have support for per-plane alpha in the core, let's use it. > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 13 +--- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 89 ++---------------- > 2 files changed, 14 insertions(+), 88 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h > index 6833ee253cfa..704cac6399eb 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h > @@ -298,7 +298,6 @@ struct atmel_hlcdc_layer { > struct atmel_hlcdc_plane { > struct drm_plane base; > struct atmel_hlcdc_layer layer; > - struct atmel_hlcdc_plane_properties *properties; > }; > > static inline struct atmel_hlcdc_plane * > @@ -345,18 +344,6 @@ struct atmel_hlcdc_dc_desc { > }; > > /** > - * Atmel HLCDC Plane properties. > - * > - * This structure stores plane property definitions. > - * > - * @alpha: alpha blending (or transparency) property > - * @rotation: rotation property > - */ > -struct atmel_hlcdc_plane_properties { > - struct drm_property *alpha; > -}; > - > -/** > * Atmel HLCDC Display Controller. > * > * @desc: HLCDC Display Controller description > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > index 1a9318810a29..dbc508889e87 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > @@ -31,7 +31,6 @@ > * @src_y: y buffer position > * @src_w: buffer width > * @src_h: buffer height > - * @alpha: alpha blending of the plane > * @disc_x: x discard position > * @disc_y: y discard position > * @disc_w: discard width > @@ -54,8 +53,6 @@ struct atmel_hlcdc_plane_state { > uint32_t src_w; > uint32_t src_h; > > - u8 alpha; > - > int disc_x; > int disc_y; > int disc_w; > @@ -385,7 +382,7 @@ atmel_hlcdc_plane_update_general_settings(struct atmel_hlcdc_plane *plane, > cfg |= ATMEL_HLCDC_LAYER_LAEN; > else > cfg |= ATMEL_HLCDC_LAYER_GAEN | > - ATMEL_HLCDC_LAYER_GA(state->alpha); > + ATMEL_HLCDC_LAYER_GA(state->base.alpha); > } > > if (state->disc_h && state->disc_w) > @@ -553,7 +550,7 @@ atmel_hlcdc_plane_prepare_disc_area(struct drm_crtc_state *c_state) > > if (!ovl_s->fb || > drm_format_has_alpha(ovl_s->fb->format->format) || > - ovl_state->alpha != 255) > + ovl_s->alpha != 255) > continue; > > /* TODO: implement a smarter hidden area detection */ > @@ -829,51 +826,18 @@ static void atmel_hlcdc_plane_destroy(struct drm_plane *p) > drm_plane_cleanup(p); > } > > -static int atmel_hlcdc_plane_atomic_set_property(struct drm_plane *p, > - struct drm_plane_state *s, > - struct drm_property *property, > - uint64_t val) > -{ > - struct atmel_hlcdc_plane *plane = drm_plane_to_atmel_hlcdc_plane(p); > - struct atmel_hlcdc_plane_properties *props = plane->properties; > - struct atmel_hlcdc_plane_state *state = > - drm_plane_state_to_atmel_hlcdc_plane_state(s); > - > - if (property == props->alpha) > - state->alpha = val; > - else > - return -EINVAL; > - > - return 0; > -} > - > -static int atmel_hlcdc_plane_atomic_get_property(struct drm_plane *p, > - const struct drm_plane_state *s, > - struct drm_property *property, > - uint64_t *val) > -{ > - struct atmel_hlcdc_plane *plane = drm_plane_to_atmel_hlcdc_plane(p); > - struct atmel_hlcdc_plane_properties *props = plane->properties; > - const struct atmel_hlcdc_plane_state *state = > - container_of(s, const struct atmel_hlcdc_plane_state, base); > - > - if (property == props->alpha) > - *val = state->alpha; > - else > - return -EINVAL; > - > - return 0; > -} > - > -static int atmel_hlcdc_plane_init_properties(struct atmel_hlcdc_plane *plane, > - struct atmel_hlcdc_plane_properties *props) > +static int atmel_hlcdc_plane_init_properties(struct atmel_hlcdc_plane *plane) > { > const struct atmel_hlcdc_layer_desc *desc = plane->layer.desc; > > if (desc->type == ATMEL_HLCDC_OVERLAY_LAYER || > - desc->type == ATMEL_HLCDC_CURSOR_LAYER) > - drm_object_attach_property(&plane->base.base, > - props->alpha, 255); > + desc->type == ATMEL_HLCDC_CURSOR_LAYER) { > + int ret; > + > + ret = drm_plane_create_alpha_property(&plane->base, 255); > + if (ret) > + return ret; > + } > > if (desc->layout.xstride && desc->layout.pstride) { > int ret; > @@ -988,8 +952,8 @@ static void atmel_hlcdc_plane_reset(struct drm_plane *p) > return; > } > > - state->alpha = 255; > p->state = &state->base; > + p->state->alpha = 255; > p->state->plane = p; > } > } > @@ -1042,13 +1006,10 @@ static const struct drm_plane_funcs layer_plane_funcs = { > .reset = atmel_hlcdc_plane_reset, > .atomic_duplicate_state = atmel_hlcdc_plane_atomic_duplicate_state, > .atomic_destroy_state = atmel_hlcdc_plane_atomic_destroy_state, > - .atomic_set_property = atmel_hlcdc_plane_atomic_set_property, > - .atomic_get_property = atmel_hlcdc_plane_atomic_get_property, > }; > > static int atmel_hlcdc_plane_create(struct drm_device *dev, > - const struct atmel_hlcdc_layer_desc *desc, > - struct atmel_hlcdc_plane_properties *props) > + const struct atmel_hlcdc_layer_desc *desc) > { > struct atmel_hlcdc_dc *dc = dev->dev_private; > struct atmel_hlcdc_plane *plane; > @@ -1060,7 +1021,6 @@ static int atmel_hlcdc_plane_create(struct drm_device *dev, > return -ENOMEM; > > atmel_hlcdc_layer_init(&plane->layer, desc, dc->hlcdc->regmap); > - plane->properties = props; > > if (desc->type == ATMEL_HLCDC_BASE_LAYER) > type = DRM_PLANE_TYPE_PRIMARY; > @@ -1081,7 +1041,7 @@ static int atmel_hlcdc_plane_create(struct drm_device *dev, > &atmel_hlcdc_layer_plane_helper_funcs); > > /* Set default property values*/ > - ret = atmel_hlcdc_plane_init_properties(plane, props); > + ret = atmel_hlcdc_plane_init_properties(plane); > if (ret) > return ret; > > @@ -1090,34 +1050,13 @@ static int atmel_hlcdc_plane_create(struct drm_device *dev, > return 0; > } > > -static struct atmel_hlcdc_plane_properties * > -atmel_hlcdc_plane_create_properties(struct drm_device *dev) > -{ > - struct atmel_hlcdc_plane_properties *props; > - > - props = devm_kzalloc(dev->dev, sizeof(*props), GFP_KERNEL); > - if (!props) > - return ERR_PTR(-ENOMEM); > - > - props->alpha = drm_property_create_range(dev, 0, "alpha", 0, 255); > - if (!props->alpha) > - return ERR_PTR(-ENOMEM); > - > - return props; > -} > - > int atmel_hlcdc_create_planes(struct drm_device *dev) > { > struct atmel_hlcdc_dc *dc = dev->dev_private; > - struct atmel_hlcdc_plane_properties *props; > const struct atmel_hlcdc_layer_desc *descs = dc->desc->layers; > int nlayers = dc->desc->nlayers; > int i, ret; > > - props = atmel_hlcdc_plane_create_properties(dev); > - if (IS_ERR(props)) > - return PTR_ERR(props); > - > dc->dscrpool = dmam_pool_create("atmel-hlcdc-dscr", dev->dev, > sizeof(struct atmel_hlcdc_dma_channel_dscr), > sizeof(u64), 0); > @@ -1130,7 +1069,7 @@ int atmel_hlcdc_create_planes(struct drm_device *dev) > descs[i].type != ATMEL_HLCDC_CURSOR_LAYER) > continue; > > - ret = atmel_hlcdc_plane_create(dev, &descs[i], props); > + ret = atmel_hlcdc_plane_create(dev, &descs[i]); > if (ret) > return ret; > }
On Tue, Jan 09, 2018 at 11:56:24AM +0100, Maxime Ripard wrote: > Now that the core has a drm format helper to tell if a format embeds an > alpha component in it, let's use it. > > Cc: Eric Anholt <eric@anholt.net> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> On patches 1-5: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/vc4/vc4_plane.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c > index 423a23ed8fc2..2c0e25128dcd 100644 > --- a/drivers/gpu/drm/vc4/vc4_plane.c > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > @@ -85,40 +85,39 @@ static const struct hvs_format { > u32 drm; /* DRM_FORMAT_* */ > u32 hvs; /* HVS_FORMAT_* */ > u32 pixel_order; > - bool has_alpha; > bool flip_cbcr; > } hvs_formats[] = { > { > .drm = DRM_FORMAT_XRGB8888, .hvs = HVS_PIXEL_FORMAT_RGBA8888, > - .pixel_order = HVS_PIXEL_ORDER_ABGR, .has_alpha = false, > + .pixel_order = HVS_PIXEL_ORDER_ABGR, > }, > { > .drm = DRM_FORMAT_ARGB8888, .hvs = HVS_PIXEL_FORMAT_RGBA8888, > - .pixel_order = HVS_PIXEL_ORDER_ABGR, .has_alpha = true, > + .pixel_order = HVS_PIXEL_ORDER_ABGR, > }, > { > .drm = DRM_FORMAT_ABGR8888, .hvs = HVS_PIXEL_FORMAT_RGBA8888, > - .pixel_order = HVS_PIXEL_ORDER_ARGB, .has_alpha = true, > + .pixel_order = HVS_PIXEL_ORDER_ARGB, > }, > { > .drm = DRM_FORMAT_XBGR8888, .hvs = HVS_PIXEL_FORMAT_RGBA8888, > - .pixel_order = HVS_PIXEL_ORDER_ARGB, .has_alpha = false, > + .pixel_order = HVS_PIXEL_ORDER_ARGB, > }, > { > .drm = DRM_FORMAT_RGB565, .hvs = HVS_PIXEL_FORMAT_RGB565, > - .pixel_order = HVS_PIXEL_ORDER_XRGB, .has_alpha = false, > + .pixel_order = HVS_PIXEL_ORDER_XRGB, > }, > { > .drm = DRM_FORMAT_BGR565, .hvs = HVS_PIXEL_FORMAT_RGB565, > - .pixel_order = HVS_PIXEL_ORDER_XBGR, .has_alpha = false, > + .pixel_order = HVS_PIXEL_ORDER_XBGR, > }, > { > .drm = DRM_FORMAT_ARGB1555, .hvs = HVS_PIXEL_FORMAT_RGBA5551, > - .pixel_order = HVS_PIXEL_ORDER_ABGR, .has_alpha = true, > + .pixel_order = HVS_PIXEL_ORDER_ABGR, > }, > { > .drm = DRM_FORMAT_XRGB1555, .hvs = HVS_PIXEL_FORMAT_RGBA5551, > - .pixel_order = HVS_PIXEL_ORDER_ABGR, .has_alpha = false, > + .pixel_order = HVS_PIXEL_ORDER_ABGR, > }, > { > .drm = DRM_FORMAT_YUV422, > @@ -601,7 +600,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane, > /* Position Word 2: Source Image Size, Alpha Mode */ > vc4_state->pos2_offset = vc4_state->dlist_count; > vc4_dlist_write(vc4_state, > - VC4_SET_FIELD(format->has_alpha ? > + VC4_SET_FIELD(drm_format_has_alpha(format->drm) ? > SCALER_POS2_ALPHA_MODE_PIPELINE : > SCALER_POS2_ALPHA_MODE_FIXED, > SCALER_POS2_ALPHA_MODE) | > -- > git-series 0.9.1 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Maxime, Thank you for the patch. On Tuesday, 9 January 2018 12:56:27 EET Maxime Ripard wrote: > Now that we have support for per-plane alpha in the core, let's use it. > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 +- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 5 +--- > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 15 +++------ > drivers/gpu/drm/rcar-du/rcar_du_plane.h | 2 +- > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 42 ++------------------------ > drivers/gpu/drm/rcar-du/rcar_du_vsp.h | 2 +- > 6 files changed, 9 insertions(+), 58 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h > b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index f8cd79488ece..aff04adaae53 > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h > @@ -89,7 +89,6 @@ struct rcar_du_device { > struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS]; > > struct { > - struct drm_property *alpha; > struct drm_property *colorkey; > } props; > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 566d1a948c8f..e1b5a7b460cc > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > @@ -417,11 +417,6 @@ static int rcar_du_encoders_init(struct rcar_du_device > *rcdu) > > static int rcar_du_properties_init(struct rcar_du_device *rcdu) > { > - rcdu->props.alpha = > - drm_property_create_range(rcdu->ddev, 0, "alpha", 0, 255); > - if (rcdu->props.alpha == NULL) > - return -ENOMEM; > - > /* > * The color key is expressed as an RGB888 triplet stored in a 32-bit > * integer in XRGB8888 format. Bit 24 is used as a flag to disable (0) > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c > b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index 61833cc1c699..5b34e8092c8b > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c > @@ -423,7 +423,7 @@ static void rcar_du_plane_setup_mode(struct > rcar_du_group *rgrp, rcar_du_plane_write(rgrp, index, PnALPHAR, > PnALPHAR_ABIT_0); > else > rcar_du_plane_write(rgrp, index, PnALPHAR, > - PnALPHAR_ABIT_X | state->alpha); > + PnALPHAR_ABIT_X | state->state.alpha); > > pnmr = PnMR_BM_MD | state->format->pnmr; > > @@ -667,11 +667,11 @@ static void rcar_du_plane_reset(struct drm_plane > *plane) > > state->hwindex = -1; > state->source = RCAR_DU_PLANE_MEMORY; > - state->alpha = 255; > state->colorkey = RCAR_DU_COLORKEY_NONE; > state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1; > > plane->state = &state->state; > + plane->state->alpha = 255; > plane->state->plane = plane; > } > > @@ -683,9 +683,7 @@ static int rcar_du_plane_atomic_set_property(struct > drm_plane *plane, struct rcar_du_plane_state *rstate = > to_rcar_plane_state(state); struct rcar_du_device *rcdu = > to_rcar_plane(plane)->group->dev; > > - if (property == rcdu->props.alpha) > - rstate->alpha = val; > - else if (property == rcdu->props.colorkey) > + if (property == rcdu->props.colorkey) > rstate->colorkey = val; > else > return -EINVAL; > @@ -701,9 +699,7 @@ static int rcar_du_plane_atomic_get_property(struct > drm_plane *plane, container_of(state, const struct rcar_du_plane_state, > state); > struct rcar_du_device *rcdu = to_rcar_plane(plane)->group->dev; > > - if (property == rcdu->props.alpha) > - *val = rstate->alpha; > - else if (property == rcdu->props.colorkey) > + if (property == rcdu->props.colorkey) > *val = rstate->colorkey; > else > return -EINVAL; > @@ -772,10 +768,9 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp) > continue; > > drm_object_attach_property(&plane->plane.base, > - rcdu->props.alpha, 255); > - drm_object_attach_property(&plane->plane.base, > rcdu->props.colorkey, > RCAR_DU_COLORKEY_NONE); > + drm_plane_create_alpha_property(&plane->plane, 255); > drm_plane_create_zpos_property(&plane->plane, 1, 1, 7); > } > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.h > b/drivers/gpu/drm/rcar-du/rcar_du_plane.h index f62e09f195de..2dc793ebd1a2 > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.h > +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.h > @@ -50,7 +50,6 @@ static inline struct rcar_du_plane *to_rcar_plane(struct > drm_plane *plane) * @state: base DRM plane state > * @format: information about the pixel format used by the plane > * @hwindex: 0-based hardware plane index, -1 means unused > - * @alpha: value of the plane alpha property > * @colorkey: value of the plane colorkey property > */ > struct rcar_du_plane_state { > @@ -60,7 +59,6 @@ struct rcar_du_plane_state { > int hwindex; > enum rcar_du_plane_source source; > > - unsigned int alpha; > unsigned int colorkey; > }; > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 2c96147bc444..ee85f6fdffad > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > @@ -54,6 +54,7 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) > }; > struct rcar_du_plane_state state = { > .state = { > + .alpha = 255, > .crtc = &crtc->crtc, > .crtc_x = 0, > .crtc_y = 0, > @@ -67,7 +68,6 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) > }, > .format = rcar_du_format_info(DRM_FORMAT_ARGB8888), > .source = RCAR_DU_PLANE_VSPD1, > - .alpha = 255, > .colorkey = 0, > }; > > @@ -173,7 +173,7 @@ static void rcar_du_vsp_plane_setup(struct > rcar_du_vsp_plane *plane) struct vsp1_du_atomic_config cfg = { > .pixelformat = 0, > .pitch = fb->pitches[0], > - .alpha = state->alpha, > + .alpha = state->state.alpha, > .zpos = state->state.zpos, > }; > unsigned int i; > @@ -351,44 +351,13 @@ static void rcar_du_vsp_plane_reset(struct drm_plane > *plane) if (state == NULL) > return; > > - state->alpha = 255; > + state->state.alpha = 255; > state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1; > > plane->state = &state->state; > plane->state->plane = plane; > } > > -static int rcar_du_vsp_plane_atomic_set_property(struct drm_plane *plane, > - struct drm_plane_state *state, struct drm_property *property, > - uint64_t val) > -{ > - struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state); > - struct rcar_du_device *rcdu = to_rcar_vsp_plane(plane)->vsp->dev; > - > - if (property == rcdu->props.alpha) > - rstate->alpha = val; > - else > - return -EINVAL; > - > - return 0; > -} > - > -static int rcar_du_vsp_plane_atomic_get_property(struct drm_plane *plane, > - const struct drm_plane_state *state, struct drm_property *property, > - uint64_t *val) > -{ > - const struct rcar_du_vsp_plane_state *rstate = > - container_of(state, const struct rcar_du_vsp_plane_state, state); > - struct rcar_du_device *rcdu = to_rcar_vsp_plane(plane)->vsp->dev; > - > - if (property == rcdu->props.alpha) > - *val = rstate->alpha; > - else > - return -EINVAL; > - > - return 0; > -} > - > static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { > .update_plane = drm_atomic_helper_update_plane, > .disable_plane = drm_atomic_helper_disable_plane, > @@ -396,8 +365,6 @@ static const struct drm_plane_funcs > rcar_du_vsp_plane_funcs = { .destroy = drm_plane_cleanup, > .atomic_duplicate_state = rcar_du_vsp_plane_atomic_duplicate_state, > .atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state, > - .atomic_set_property = rcar_du_vsp_plane_atomic_set_property, > - .atomic_get_property = rcar_du_vsp_plane_atomic_get_property, > }; > > int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, > @@ -454,8 +421,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct > device_node *np, if (type == DRM_PLANE_TYPE_PRIMARY) > continue; > > - drm_object_attach_property(&plane->plane.base, > - rcdu->props.alpha, 255); > + drm_plane_create_alpha_property(&plane->plane, 255); > drm_plane_create_zpos_property(&plane->plane, 1, 1, > vsp->num_planes - 1); > } > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h index f876c512163c..8b19914761e4 > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h > @@ -44,7 +44,6 @@ static inline struct rcar_du_vsp_plane > *to_rcar_vsp_plane(struct drm_plane *p) * @state: base DRM plane state > * @format: information about the pixel format used by the plane > * @sg_tables: scatter-gather tables for the frame buffer memory > - * @alpha: value of the plane alpha property > * @zpos: value of the plane zpos property > */ > struct rcar_du_vsp_plane_state { > @@ -53,7 +52,6 @@ struct rcar_du_vsp_plane_state { > const struct rcar_du_format_info *format; > struct sg_table sg_tables[3]; > > - unsigned int alpha; > unsigned int zpos; > };
Hi Laurent, On Tue, Jan 09, 2018 at 02:29:58PM +0200, Laurent Pinchart wrote: > On Tuesday, 9 January 2018 12:56:20 EET Maxime Ripard wrote: > > There's a bunch of drivers that duplicate the same function to know if a > > particular format embeds an alpha component or not. > > > > Let's create a helper to avoid duplicating that logic. > > > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > > Cc: Eric Anholt <eric@anholt.net> > > Cc: Inki Dae <inki.dae@samsung.com> > > Cc: Joonyoung Shim <jy0922.shim@samsung.com> > > Cc: Kyungmin Park <kyungmin.park@samsung.com> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Mark Yao <mark.yao@rock-chips.com> > > Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > drivers/gpu/drm/drm_fourcc.c | 43 +++++++++++++++++++++++++++++++++++++- > > include/drm/drm_fourcc.h | 1 +- > > 2 files changed, 44 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > > index 9c0152df45ad..6e6227d6a46b 100644 > > --- a/drivers/gpu/drm/drm_fourcc.c > > +++ b/drivers/gpu/drm/drm_fourcc.c > > @@ -348,3 +348,46 @@ int drm_format_plane_height(int height, uint32_t > > format, int plane) return height / info->vsub; > > } > > EXPORT_SYMBOL(drm_format_plane_height); > > + > > +/** > > + * drm_format_has_alpha - get whether the format embeds an alpha component > > + * @format: pixel format (DRM_FORMAT_*) > > + * > > + * Returns: > > + * true if the format embeds an alpha component, false otherwise. > > + */ > > +bool drm_format_has_alpha(uint32_t format) > > +{ > > + switch (format) { > > + case DRM_FORMAT_ARGB4444: > > + case DRM_FORMAT_ABGR4444: > > + case DRM_FORMAT_RGBA4444: > > + case DRM_FORMAT_BGRA4444: > > + case DRM_FORMAT_ARGB1555: > > + case DRM_FORMAT_ABGR1555: > > + case DRM_FORMAT_RGBA5551: > > + case DRM_FORMAT_BGRA5551: > > + case DRM_FORMAT_ARGB8888: > > + case DRM_FORMAT_ABGR8888: > > + case DRM_FORMAT_RGBA8888: > > + case DRM_FORMAT_BGRA8888: > > + case DRM_FORMAT_ARGB2101010: > > + case DRM_FORMAT_ABGR2101010: > > + case DRM_FORMAT_RGBA1010102: > > + case DRM_FORMAT_BGRA1010102: > > + case DRM_FORMAT_AYUV: > > + case DRM_FORMAT_XRGB8888_A8: > > + case DRM_FORMAT_XBGR8888_A8: > > + case DRM_FORMAT_RGBX8888_A8: > > + case DRM_FORMAT_BGRX8888_A8: > > + case DRM_FORMAT_RGB888_A8: > > + case DRM_FORMAT_BGR888_A8: > > + case DRM_FORMAT_RGB565_A8: > > + case DRM_FORMAT_BGR565_A8: > > + return true; > > + > > + default: > > + return false; > > + } > > +} > > +EXPORT_SYMBOL(drm_format_has_alpha); > > How about adding the information to struct drm_format_info instead ? > drm_format_has_alpha() could then be implemented as > > bool drm_format_has_alpha(uint32_t format) > { > const struct drm_format_info *info; > > info = drm_format_info(format); > return info ? info->has_alpha : false; > } I considered it, and wasn't too sure about if adding more fields to drm_format_info was ok. I can definitely do it that way. > although drivers should really use the drm_framebuffer::format field directly > in most cases, so the helper might not be needed at all. The drivers converted in my serie shouldn't be too hard to convert to use drm_format_info directly, so that can be removed as well. Maxime
Maxime Ripard <maxime.ripard@free-electrons.com> writes: > Now that the core has a drm format helper to tell if a format embeds an > alpha component in it, let's use it. > > Cc: Eric Anholt <eric@anholt.net> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Reviewed-by: Eric Anholt <eric@anholt.net>
2018년 01월 09일 19:56에 Maxime Ripard 이(가) 쓴 글: > Now that the core has a drm format helper to tell if a format embeds an > alpha component in it, let's use it. > > Cc: Inki Dae <inki.dae@samsung.com> > Cc: Joonyoung Shim <jy0922.shim@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Acked-by: Inki Dae <inki.dae@samsung.com> Thanks, Inki Dae > --- > drivers/gpu/drm/exynos/exynos_mixer.c | 14 +------------- > 1 file changed, 1 insertion(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c > index dc5d79465f9b..d7339a6d072c 100644 > --- a/drivers/gpu/drm/exynos/exynos_mixer.c > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c > @@ -179,18 +179,6 @@ static const u8 filter_cr_horiz_tap4[] = { > 70, 59, 48, 37, 27, 19, 11, 5, > }; > > -static inline bool is_alpha_format(unsigned int pixel_format) > -{ > - switch (pixel_format) { > - case DRM_FORMAT_ARGB8888: > - case DRM_FORMAT_ARGB1555: > - case DRM_FORMAT_ARGB4444: > - return true; > - default: > - return false; > - } > -} > - > static inline u32 vp_reg_read(struct mixer_context *ctx, u32 reg_id) > { > return readl(ctx->vp_regs + reg_id); > @@ -625,7 +613,7 @@ static void mixer_graph_buffer(struct mixer_context *ctx, > mixer_reg_write(ctx, MXR_GRAPHIC_BASE(win), dma_addr); > > mixer_cfg_layer(ctx, win, priority, true); > - mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->format->format)); > + mixer_cfg_gfx_blend(ctx, win, drm_format_has_alpha(fb->format->format)); > > /* layer update mandatory for mixer 16.0.33.0 */ > if (ctx->mxr_ver == MXR_VER_16_0_33_0 || >
On Tue, Jan 09, 2018 at 02:28:33PM +0100, Maxime Ripard wrote: > Hi Laurent, > > On Tue, Jan 09, 2018 at 02:29:58PM +0200, Laurent Pinchart wrote: > > On Tuesday, 9 January 2018 12:56:20 EET Maxime Ripard wrote: > > > There's a bunch of drivers that duplicate the same function to know if a > > > particular format embeds an alpha component or not. > > > > > > Let's create a helper to avoid duplicating that logic. > > > > > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > > > Cc: Eric Anholt <eric@anholt.net> > > > Cc: Inki Dae <inki.dae@samsung.com> > > > Cc: Joonyoung Shim <jy0922.shim@samsung.com> > > > Cc: Kyungmin Park <kyungmin.park@samsung.com> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Cc: Mark Yao <mark.yao@rock-chips.com> > > > Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > --- > > > drivers/gpu/drm/drm_fourcc.c | 43 +++++++++++++++++++++++++++++++++++++- > > > include/drm/drm_fourcc.h | 1 +- > > > 2 files changed, 44 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > > > index 9c0152df45ad..6e6227d6a46b 100644 > > > --- a/drivers/gpu/drm/drm_fourcc.c > > > +++ b/drivers/gpu/drm/drm_fourcc.c > > > @@ -348,3 +348,46 @@ int drm_format_plane_height(int height, uint32_t > > > format, int plane) return height / info->vsub; > > > } > > > EXPORT_SYMBOL(drm_format_plane_height); > > > + > > > +/** > > > + * drm_format_has_alpha - get whether the format embeds an alpha component > > > + * @format: pixel format (DRM_FORMAT_*) > > > + * > > > + * Returns: > > > + * true if the format embeds an alpha component, false otherwise. > > > + */ > > > +bool drm_format_has_alpha(uint32_t format) > > > +{ > > > + switch (format) { > > > + case DRM_FORMAT_ARGB4444: > > > + case DRM_FORMAT_ABGR4444: > > > + case DRM_FORMAT_RGBA4444: > > > + case DRM_FORMAT_BGRA4444: > > > + case DRM_FORMAT_ARGB1555: > > > + case DRM_FORMAT_ABGR1555: > > > + case DRM_FORMAT_RGBA5551: > > > + case DRM_FORMAT_BGRA5551: > > > + case DRM_FORMAT_ARGB8888: > > > + case DRM_FORMAT_ABGR8888: > > > + case DRM_FORMAT_RGBA8888: > > > + case DRM_FORMAT_BGRA8888: > > > + case DRM_FORMAT_ARGB2101010: > > > + case DRM_FORMAT_ABGR2101010: > > > + case DRM_FORMAT_RGBA1010102: > > > + case DRM_FORMAT_BGRA1010102: > > > + case DRM_FORMAT_AYUV: > > > + case DRM_FORMAT_XRGB8888_A8: > > > + case DRM_FORMAT_XBGR8888_A8: > > > + case DRM_FORMAT_RGBX8888_A8: > > > + case DRM_FORMAT_BGRX8888_A8: > > > + case DRM_FORMAT_RGB888_A8: > > > + case DRM_FORMAT_BGR888_A8: > > > + case DRM_FORMAT_RGB565_A8: > > > + case DRM_FORMAT_BGR565_A8: > > > + return true; > > > + > > > + default: > > > + return false; > > > + } > > > +} > > > +EXPORT_SYMBOL(drm_format_has_alpha); > > > > How about adding the information to struct drm_format_info instead ? > > drm_format_has_alpha() could then be implemented as > > > > bool drm_format_has_alpha(uint32_t format) > > { > > const struct drm_format_info *info; > > > > info = drm_format_info(format); > > return info ? info->has_alpha : false; > > } > > I considered it, and wasn't too sure about if adding more fields to > drm_format_info was ok. I can definitely do it that way. Are you going to send an updated patch with the change mentioned here. Or should I update my patch (https://patchwork.kernel.org/patch/10161023/) and change the type of '.alpha' to boolean to denote if the color format has an alpha channel or not. > > although drivers should really use the drm_framebuffer::format field directly > > in most cases, so the helper might not be needed at all. > > The drivers converted in my serie shouldn't be too hard to convert to > use drm_format_info directly, so that can be removed as well. > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
在 2018/1/9 18:56, Maxime Ripard 写道: > Now that the core has a drm format helper to tell if a format embeds an > alpha component in it, let's use it. > > Cc: Mark Yao <mark.yao@rock-chips.com> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > index 19128b4dea54..cfc4d4909185 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > @@ -253,17 +253,6 @@ static bool is_yuv_support(uint32_t format) > } > } > > -static bool is_alpha_support(uint32_t format) > -{ > - switch (format) { > - case DRM_FORMAT_ARGB8888: > - case DRM_FORMAT_ABGR8888: > - return true; > - default: > - return false; > - } > -} > - > static uint16_t scl_vop_cal_scale(enum scale_mode mode, uint32_t src, > uint32_t dst, bool is_horizontal, > int vsu_mode, int *vskiplines) > @@ -790,7 +779,7 @@ static void vop_plane_atomic_update(struct drm_plane *plane, > rb_swap = has_rb_swapped(fb->format->format); > VOP_WIN_SET(vop, win, rb_swap, rb_swap); > > - if (is_alpha_support(fb->format->format)) { > + if (drm_format_has_alpha(fb->format->format)) { > VOP_WIN_SET(vop, win, dst_alpha_ctl, > DST_FACTOR_M0(ALPHA_SRC_INVERSE)); > val = SRC_ALPHA_EN(1) | SRC_COLOR_M0(ALPHA_SRC_PRE_MUL) | > remove dead email: Mark Yao <mark.yao@rock-chips.com> Acked-by: Sandy huang <hjc@rock-chips.com>
On Tue, Jan 09, 2018 at 02:28:33PM +0100, Maxime Ripard wrote: > Hi Laurent, > > On Tue, Jan 09, 2018 at 02:29:58PM +0200, Laurent Pinchart wrote: > > On Tuesday, 9 January 2018 12:56:20 EET Maxime Ripard wrote: > > > There's a bunch of drivers that duplicate the same function to know if a > > > particular format embeds an alpha component or not. > > > > > > Let's create a helper to avoid duplicating that logic. > > > > > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > > > Cc: Eric Anholt <eric@anholt.net> > > > Cc: Inki Dae <inki.dae@samsung.com> > > > Cc: Joonyoung Shim <jy0922.shim@samsung.com> > > > Cc: Kyungmin Park <kyungmin.park@samsung.com> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Cc: Mark Yao <mark.yao@rock-chips.com> > > > Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > --- > > > drivers/gpu/drm/drm_fourcc.c | 43 +++++++++++++++++++++++++++++++++++++- > > > include/drm/drm_fourcc.h | 1 +- > > > 2 files changed, 44 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > > > index 9c0152df45ad..6e6227d6a46b 100644 > > > --- a/drivers/gpu/drm/drm_fourcc.c > > > +++ b/drivers/gpu/drm/drm_fourcc.c > > > @@ -348,3 +348,46 @@ int drm_format_plane_height(int height, uint32_t > > > format, int plane) return height / info->vsub; > > > } > > > EXPORT_SYMBOL(drm_format_plane_height); > > > + > > > +/** > > > + * drm_format_has_alpha - get whether the format embeds an alpha component > > > + * @format: pixel format (DRM_FORMAT_*) > > > + * > > > + * Returns: > > > + * true if the format embeds an alpha component, false otherwise. > > > + */ > > > +bool drm_format_has_alpha(uint32_t format) > > > +{ > > > + switch (format) { > > > + case DRM_FORMAT_ARGB4444: > > > + case DRM_FORMAT_ABGR4444: > > > + case DRM_FORMAT_RGBA4444: > > > + case DRM_FORMAT_BGRA4444: > > > + case DRM_FORMAT_ARGB1555: > > > + case DRM_FORMAT_ABGR1555: > > > + case DRM_FORMAT_RGBA5551: > > > + case DRM_FORMAT_BGRA5551: > > > + case DRM_FORMAT_ARGB8888: > > > + case DRM_FORMAT_ABGR8888: > > > + case DRM_FORMAT_RGBA8888: > > > + case DRM_FORMAT_BGRA8888: > > > + case DRM_FORMAT_ARGB2101010: > > > + case DRM_FORMAT_ABGR2101010: > > > + case DRM_FORMAT_RGBA1010102: > > > + case DRM_FORMAT_BGRA1010102: > > > + case DRM_FORMAT_AYUV: > > > + case DRM_FORMAT_XRGB8888_A8: > > > + case DRM_FORMAT_XBGR8888_A8: > > > + case DRM_FORMAT_RGBX8888_A8: > > > + case DRM_FORMAT_BGRX8888_A8: > > > + case DRM_FORMAT_RGB888_A8: > > > + case DRM_FORMAT_BGR888_A8: > > > + case DRM_FORMAT_RGB565_A8: > > > + case DRM_FORMAT_BGR565_A8: > > > + return true; > > > + > > > + default: > > > + return false; > > > + } > > > +} > > > +EXPORT_SYMBOL(drm_format_has_alpha); > > > > How about adding the information to struct drm_format_info instead ? > > drm_format_has_alpha() could then be implemented as > > > > bool drm_format_has_alpha(uint32_t format) > > { > > const struct drm_format_info *info; > > > > info = drm_format_info(format); > > return info ? info->has_alpha : false; > > } > > I considered it, and wasn't too sure about if adding more fields to > drm_format_info was ok. I can definitely do it that way.\ Are you going to send an updated patch with the change mentioned here. Or should I update my patch (https://patchwork.kernel.org/patch/10161023/) and change the type of '.alpha' to boolean to denote if the color format has an alpha channel or not. > > > although drivers should really use the drm_framebuffer::format field directly > > in most cases, so the helper might not be needed at all. > > The drivers converted in my serie shouldn't be too hard to convert to > use drm_format_info directly, so that can be removed as well. > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Ayan, On Mon, Jan 15, 2018 at 03:47:44PM +0000, Ayan Halder wrote: > On Tue, Jan 09, 2018 at 02:28:33PM +0100, Maxime Ripard wrote: > > On Tue, Jan 09, 2018 at 02:29:58PM +0200, Laurent Pinchart wrote: > > > On Tuesday, 9 January 2018 12:56:20 EET Maxime Ripard wrote: > > > > There's a bunch of drivers that duplicate the same function to know if a > > > > particular format embeds an alpha component or not. > > > > > > > > Let's create a helper to avoid duplicating that logic. > > > > > > > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > Cc: Eric Anholt <eric@anholt.net> > > > > Cc: Inki Dae <inki.dae@samsung.com> > > > > Cc: Joonyoung Shim <jy0922.shim@samsung.com> > > > > Cc: Kyungmin Park <kyungmin.park@samsung.com> > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Cc: Mark Yao <mark.yao@rock-chips.com> > > > > Cc: Seung-Woo Kim <sw0312.kim@samsung.com> > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > > --- > > > > drivers/gpu/drm/drm_fourcc.c | 43 +++++++++++++++++++++++++++++++++++++- > > > > include/drm/drm_fourcc.h | 1 +- > > > > 2 files changed, 44 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > > > > index 9c0152df45ad..6e6227d6a46b 100644 > > > > --- a/drivers/gpu/drm/drm_fourcc.c > > > > +++ b/drivers/gpu/drm/drm_fourcc.c > > > > @@ -348,3 +348,46 @@ int drm_format_plane_height(int height, uint32_t > > > > format, int plane) return height / info->vsub; > > > > } > > > > EXPORT_SYMBOL(drm_format_plane_height); > > > > + > > > > +/** > > > > + * drm_format_has_alpha - get whether the format embeds an alpha component > > > > + * @format: pixel format (DRM_FORMAT_*) > > > > + * > > > > + * Returns: > > > > + * true if the format embeds an alpha component, false otherwise. > > > > + */ > > > > +bool drm_format_has_alpha(uint32_t format) > > > > +{ > > > > + switch (format) { > > > > + case DRM_FORMAT_ARGB4444: > > > > + case DRM_FORMAT_ABGR4444: > > > > + case DRM_FORMAT_RGBA4444: > > > > + case DRM_FORMAT_BGRA4444: > > > > + case DRM_FORMAT_ARGB1555: > > > > + case DRM_FORMAT_ABGR1555: > > > > + case DRM_FORMAT_RGBA5551: > > > > + case DRM_FORMAT_BGRA5551: > > > > + case DRM_FORMAT_ARGB8888: > > > > + case DRM_FORMAT_ABGR8888: > > > > + case DRM_FORMAT_RGBA8888: > > > > + case DRM_FORMAT_BGRA8888: > > > > + case DRM_FORMAT_ARGB2101010: > > > > + case DRM_FORMAT_ABGR2101010: > > > > + case DRM_FORMAT_RGBA1010102: > > > > + case DRM_FORMAT_BGRA1010102: > > > > + case DRM_FORMAT_AYUV: > > > > + case DRM_FORMAT_XRGB8888_A8: > > > > + case DRM_FORMAT_XBGR8888_A8: > > > > + case DRM_FORMAT_RGBX8888_A8: > > > > + case DRM_FORMAT_BGRX8888_A8: > > > > + case DRM_FORMAT_RGB888_A8: > > > > + case DRM_FORMAT_BGR888_A8: > > > > + case DRM_FORMAT_RGB565_A8: > > > > + case DRM_FORMAT_BGR565_A8: > > > > + return true; > > > > + > > > > + default: > > > > + return false; > > > > + } > > > > +} > > > > +EXPORT_SYMBOL(drm_format_has_alpha); > > > > > > How about adding the information to struct drm_format_info instead ? > > > drm_format_has_alpha() could then be implemented as > > > > > > bool drm_format_has_alpha(uint32_t format) > > > { > > > const struct drm_format_info *info; > > > > > > info = drm_format_info(format); > > > return info ? info->has_alpha : false; > > > } > > > > I considered it, and wasn't too sure about if adding more fields to > > drm_format_info was ok. I can definitely do it that way. > > Are you going to send an updated patch with the change mentioned here. > Or should I update my patch (https://patchwork.kernel.org/patch/10161023/) > and change the type of '.alpha' to boolean to denote if the color > format has an alpha channel or not. Yes, I already had those patches ready. I'm just waiting for the discussion on the alpha property to settle before sending a v2. Maxime
Hi, This serie aims at enhancing the support for our planes in the current drm driver on the first generation of Allwinner's display engine. This also introduces a few generic stuff, as well as some conversion for some other drivers. This series basically implements three things that look orthogonal, but due to the way the hardware works are kind of related. The main feature is that instead of implementing 2 planes per backend, we are now able to use the planes that are available in hardware. This was unsupported before because of the way the composition works in the hardware. Indeed, the planes are first grouped into 2 pipes that are doing a basic composition, in case of overlapping planes, it just takes whatever plane has the highest priority (=> zpos). Then, the alpha blending is done between the two pipes. This was simplified so far by only using two planes, one for each pipe, which was allowing us to have an illusion of proper alpha blending. This is further complicated by the bug/feature that the lowest plane must not have any alpha at all, otherwise the pixel will turn black, no matter what the value of alpha is. This basically means that we can have a plane with alpha only in the second pipe. However, as we have more and more blocks being worked on, 2 planes are getting really limited and we need to support all 4 of them. This is mostly possible by extending our atomic_check and to make sure that we enforce those constraints, and assign the pipes automatically. This is done by looking at the number of planes using an alpha component, and we then end up in various scenarios: - 0 plane with alpha => we don't care for the pipes at all. All the planes are assigned to the first pipe - 1 plane with alpha => we assign all the planes without alpha below the plane with alpha to the first pipe, and then all the remaining planes to the second pipe. The plane with alpha will be the lowest plane on that pipe, which means that whatever plane is above it will have precedence, but the alpha component will remain and will be used on pixels that are not overlapping - 2-4 planes with alpha => we can't operate that way, we just reject the configuration. In addition to the formats that embed an alpha component, we also add support for plane-wide alpha property, and in order to tweak the configuration the way we want to, we also add support for configurable zpos. Let me know what you think, Maxime Maxime Ripard (19): drm/fourcc: Add a function to tell if the format embeds alpha drm/atmel-hlcdc: Use the alpha format helper drm/exynos: Use the alpha format helper drm/rockchip: Use the alpha format helper drm/vc4: Use the alpha format helper drm/blend: Add a generic alpha property drm/atmel-hclcdc: Convert to the new generic alpha property drm/rcar-du: Convert to the new generic alpha property drm/sun4i: backend: Fix structure indentation drm/sun4i: backend: Fix define typo drm/sun4i: framebuffer: Add a custom atomic_check drm/sun4i: backend: Move the coord function in the shared part drm/sun4i: backend: Set a default zpos in our reset hook drm/sun4i: backend: Add support for zpos drm/sun4i: backend: Check for the number of alpha planes drm/sun4i: backend: Assign the pipes automatically drm/sun4i: backend: Make zpos configurable drm/sun4i: Add support for plane alpha drm/sun4i: backend: Remove ARGB spoofing Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 13 +-- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 109 ++------------- drivers/gpu/drm/drm_atomic.c | 4 +- drivers/gpu/drm/drm_atomic_helper.c | 1 +- drivers/gpu/drm/drm_blend.c | 32 ++++- drivers/gpu/drm/drm_fourcc.c | 43 ++++++- drivers/gpu/drm/exynos/exynos_mixer.c | 14 +-- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 +- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 5 +- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 15 +-- drivers/gpu/drm/rcar-du/rcar_du_plane.h | 2 +- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 42 +------ drivers/gpu/drm/rcar-du/rcar_du_vsp.h | 2 +- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 13 +-- drivers/gpu/drm/sun4i/sun4i_backend.c | 126 +++++++++++++++-- drivers/gpu/drm/sun4i/sun4i_backend.h | 11 +- drivers/gpu/drm/sun4i/sun4i_framebuffer.c | 18 +- drivers/gpu/drm/sun4i/sun4i_layer.c | 84 ++--------- drivers/gpu/drm/sun4i/sun4i_layer.h | 1 +- drivers/gpu/drm/vc4/vc4_plane.c | 19 +-- include/drm/drm_blend.h | 1 +- include/drm/drm_fourcc.h | 1 +- include/drm/drm_plane.h | 6 +- 24 files changed, 289 insertions(+), 276 deletions(-) base-commit: 53b519deaf2e507b121b64abcb4ba0da075bd6a7