On Mon, May 26, 2014 at 11:31:10AM +0200, Daniel Vetter wrote:
> On Sat, May 24, 2014 at 02:30:21PM -0400, Rob Clark wrote:
> > Break the mutable state of a crtc out into a separate structure
> > and use atomic properties mechanism to set crtc attributes. This
> > makes it easier to have some helpers for crtc->set_property()
> > and for checking for invalid params. The idea is that individual
> > drivers can wrap the state struct in their own struct which adds
> > driver specific parameters, for easy build-up of state across
> > multiple set_property() calls and for easy atomic commit or roll-
> > back.
> >
> > Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
>
> Same comments about interface design as for the plane patch apply here.
> One additional comment below.
>
> > ---
> > drivers/gpu/drm/armada/armada_crtc.c | 11 +-
> > drivers/gpu/drm/ast/ast_mode.c | 1 +
> > drivers/gpu/drm/cirrus/cirrus_mode.c | 1 +
> > drivers/gpu/drm/drm_atomic.c | 231 ++++++++++-
> > drivers/gpu/drm/drm_crtc.c | 598 ++++++++++++++++++-----------
> > drivers/gpu/drm/drm_fb_helper.c | 2 +-
> > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 7 +-
> > drivers/gpu/drm/gma500/cdv_intel_display.c | 1 +
> > drivers/gpu/drm/gma500/psb_intel_display.c | 1 +
> > drivers/gpu/drm/i915/intel_display.c | 1 +
> > drivers/gpu/drm/mgag200/mgag200_mode.c | 1 +
> > drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c | 6 +-
> > drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 6 +-
> > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 1 +
> > drivers/gpu/drm/nouveau/nv50_display.c | 1 +
> > drivers/gpu/drm/omapdrm/omap_crtc.c | 12 +-
> > drivers/gpu/drm/omapdrm/omap_drv.c | 2 +-
> > drivers/gpu/drm/qxl/qxl_display.c | 2 +
> > drivers/gpu/drm/radeon/radeon_display.c | 2 +
> > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +
> > drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 2 +
> > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 1 +
> > drivers/gpu/drm/udl/udl_modeset.c | 2 +
> > drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 1 +
> > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 1 +
> > include/drm/drm_atomic.h | 29 ++
> > include/drm/drm_crtc.h | 103 ++++-
> > 27 files changed, 785 insertions(+), 243 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> > index 7d3c649..6237af4 100644
> > --- a/drivers/gpu/drm/armada/armada_crtc.c
> > +++ b/drivers/gpu/drm/armada/armada_crtc.c
> > @@ -9,6 +9,7 @@
> > #include <linux/clk.h>
> > #include <drm/drmP.h>
> > #include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_atomic.h>
> > #include "armada_crtc.h"
> > #include "armada_drm.h"
> > #include "armada_fb.h"
> > @@ -966,7 +967,12 @@ armada_drm_crtc_set_property(struct drm_crtc *crtc,
> > {
> > struct armada_private *priv = crtc->dev->dev_private;
> > struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
> > + struct drm_crtc_state *cstate = drm_atomic_get_crtc_state(crtc, state);
> > bool update_csc = false;
> > + int ret = 0;
> > +
> > + if (IS_ERR(cstate))
> > + return PTR_ERR(cstate);
> >
> > if (property == priv->csc_yuv_prop) {
> > dcrtc->csc_yuv_mode = val;
> > @@ -974,6 +980,9 @@ armada_drm_crtc_set_property(struct drm_crtc *crtc,
> > } else if (property == priv->csc_rgb_prop) {
> > dcrtc->csc_rgb_mode = val;
> > update_csc = true;
> > + } else {
> > + ret = drm_crtc_set_property(crtc, cstate, property,
> > + val, blob_data);
> > }
> >
> > if (update_csc) {
> > @@ -984,7 +993,7 @@ armada_drm_crtc_set_property(struct drm_crtc *crtc,
> > writel_relaxed(val, dcrtc->base + LCD_SPU_IOPAD_CONTROL);
> > }
> >
> > - return 0;
> > + return ret;
> > }
> >
> > static struct drm_crtc_funcs armada_crtc_funcs = {
> > diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> > index 114aee9..c08e0e1 100644
> > --- a/drivers/gpu/drm/ast/ast_mode.c
> > +++ b/drivers/gpu/drm/ast/ast_mode.c
> > @@ -632,6 +632,7 @@ static const struct drm_crtc_funcs ast_crtc_funcs = {
> > .cursor_move = ast_cursor_move,
> > .reset = ast_crtc_reset,
> > .set_config = drm_crtc_helper_set_config,
> > + .set_property = drm_atomic_crtc_set_property,
> > .gamma_set = ast_crtc_gamma_set,
> > .destroy = ast_crtc_destroy,
> > };
> > diff --git a/drivers/gpu/drm/cirrus/cirrus_mode.c b/drivers/gpu/drm/cirrus/cirrus_mode.c
> > index 49332c5..3c4428c 100644
> > --- a/drivers/gpu/drm/cirrus/cirrus_mode.c
> > +++ b/drivers/gpu/drm/cirrus/cirrus_mode.c
> > @@ -366,6 +366,7 @@ static void cirrus_crtc_destroy(struct drm_crtc *crtc)
> > static const struct drm_crtc_funcs cirrus_crtc_funcs = {
> > .gamma_set = cirrus_crtc_gamma_set,
> > .set_config = drm_crtc_helper_set_config,
> > + .set_property = drm_atomic_crtc_set_property,
> > .destroy = cirrus_crtc_destroy,
> > };
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 403ffc5..863a0fe 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -48,11 +48,13 @@ struct drm_atomic_state *drm_atomic_begin(struct drm_device *dev,
> > {
> > struct drm_atomic_state *state;
> > int nplanes = dev->mode_config.num_total_plane;
> > + int ncrtcs = dev->mode_config.num_crtc;
> > int sz;
> > void *ptr;
> >
> > sz = sizeof(*state);
> > sz += (sizeof(state->planes) + sizeof(state->pstates)) * nplanes;
> > + sz += (sizeof(state->crtcs) + sizeof(state->cstates)) * ncrtcs;
> >
> > ptr = kzalloc(sz, GFP_KERNEL);
> >
> > @@ -74,6 +76,12 @@ struct drm_atomic_state *drm_atomic_begin(struct drm_device *dev,
> > state->pstates = ptr;
> > ptr = &state->pstates[nplanes];
> >
> > + state->crtcs = ptr;
> > + ptr = &state->crtcs[ncrtcs];
> > +
> > + state->cstates = ptr;
> > + ptr = &state->cstates[ncrtcs];
> > +
> > return state;
> > }
> > EXPORT_SYMBOL(drm_atomic_begin);
> > @@ -92,7 +100,18 @@ int drm_atomic_set_event(struct drm_device *dev,
> > struct drm_atomic_state *state, struct drm_mode_object *obj,
> > struct drm_pending_vblank_event *event)
> > {
> > - return -EINVAL; /* for now */
> > + switch (obj->type) {
> > + case DRM_MODE_OBJECT_CRTC: {
> > + struct drm_crtc_state *cstate =
> > + drm_atomic_get_crtc_state(obj_to_crtc(obj), state);
> > + if (IS_ERR(cstate))
> > + return PTR_ERR(cstate);
> > + cstate->event = event;
> > + return 0;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
>
> Hm, I think if we only want completion events on crtcs (which I agree on)
I don't. Unless you have a nice way of passing some kind of "fbs now
available for rendering" list back to userland in the single crtc
event. Last time I looked making the drm event stuff deal with
variable length events looked more painful than just adding per
plane events. But I must admit that I didn't really try to do it.
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
