Re: [PATCH -next] drm/vmwgfx: Remove set but not used variable 'restart'
Thanks for doing this. Will include this one with next vmwgfx pull
request.
Reviewed-by: Deepak Rawat
On Thu, 2019-02-14 at 02:08 +, YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c: In function
> 'vmw_cmdbuf_work_func':
> drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c:514:7: warning:
> variable 'restart' set but not used [-Wunused-but-set-variable]
>
> It not used any more after commit dc366364c4ef ("drm/vmwgfx: Fix
> multiple
> command buffer context use")
>
> Signed-off-by: YueHaibing
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c | 6 +-
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c
> index 70dab55e7888..ed15655eacd2 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c
> @@ -511,17 +511,14 @@ static void vmw_cmdbuf_work_func(struct
> work_struct *work)
> container_of(work, struct vmw_cmdbuf_man, work);
> struct vmw_cmdbuf_header *entry, *next;
> uint32_t dummy;
> - bool restart[SVGA_CB_CONTEXT_MAX];
> bool send_fence = false;
> struct list_head restart_head[SVGA_CB_CONTEXT_MAX];
> int i;
> struct vmw_cmdbuf_context *ctx;
> bool global_block = false;
>
> - for_each_cmdbuf_ctx(man, i, ctx) {
> + for_each_cmdbuf_ctx(man, i, ctx)
> INIT_LIST_HEAD(&restart_head[i]);
> - restart[i] = false;
> - }
>
> mutex_lock(&man->error_mutex);
> spin_lock(&man->lock);
> @@ -533,7 +530,6 @@ static void vmw_cmdbuf_work_func(struct
> work_struct *work)
> const char *cmd_name;
>
> list_del_init(&entry->list);
> - restart[entry->cb_context] = true;
> global_block = true;
>
> if (!vmw_cmd_describe(header, &error_cmd_size,
> &cmd_name)) {
>
>
>
RE: [RFC 0/3] drm: page-flip with damage
>Hi, > >Many thanks that you have picked it up. >Unfortunately I didn't had time to work on it for a while. > >I am ok with what you have done, ihmo the review is going in good direction. >I will try not to miss your update of it. >From my side I can say that damage rects with frame-buffer coordinates are >perfectly fine for DisplayLink case. > >Btw I have noticed that there is also a patch from Rob Clark that is supplying >helper for legacy dirtyfb callback. >Maybe we should unify the naming of the rects. Here we have "damage_clips", >Clark's patch has 'dirty_rects' notion. >On other hand existing legacy dirtyfb callback in drm_framebuffer_funcs is >also using 'clip_rects' :). The reason to name it damage is inspired by eglSwapBuffersWithDamageKHR which user-space will be calling before submitting a page-flip. So it makes naming consistent and in sync with user-space. > >Thanks > >Łukasz Spintzyk > >On 05/04/2018 01:49, Deepak Rawat wrote: >Hi All, > >This is extension to Lukasz Spintzyk earlier draft of damage interface for drm. >Bascially a new plane property is added called "DAMAGE_CLIPS" which is simply >an array of drm_rect (exported to userspace as drm_mode_rect). The clips >represents damage in framebuffer coordinate of attached fb to the plane. > >Helper iterator is added to traverse the damage rectangles and get the damage >clips in framebuffer, plane or crtc coordinates as need by driver >implementation. Finally a helper to reset damage in case need full update is >required. Drivers interested in page-flip with damage should call this from >atomic_check hook. > >With the RFC for atomic implementation of dirtyfb ioctl I was thinking >should we need to consider dirty_fb flags, especially >DRM_MODE_FB_DIRTY_ANNOTATE_COPY to be passed with atomic >DAMAGE_CLIPS property blob? I didn't considered that untill now. If no driver >uses that in my opinion for simplicity this can be ignored? > >About overlaping of damage rectangles is also not finalized. This really >depends on driver specific implementation and can be left open-ended? > >My knowledge is limited to vmwgfx so would like to hear about other driver use >cases and this can be modified in keeping other drivers need. > >Going forward driver implementation for vmwgfx and user-space implementation >of kmscube/weston will be next step to test the changes. > >Thanks, >Deepak > >Deepak Rawat (2): >drm: Add helper iterator functions to iterate over plane damage. >drm: Add helper to validate damage during modeset_check > >Lukasz Spintzyk (1): >drm: Add DAMAGE_CLIPS property to plane > >drivers/gpu/drm/drm_atomic.c | 42 + >drivers/gpu/drm/drm_atomic_helper.c | 173 >drivers/gpu/drm/drm_mode_config.c | 5 ++ >drivers/gpu/drm/drm_plane.c | 12 +++ >include/drm/drm_atomic_helper.h | 41 + >include/drm/drm_mode_config.h | 15 >include/drm/drm_plane.h | 16 >include/uapi/drm/drm_mode.h | 15 >8 files changed, 319 insertions(+) > >-- >2.7.4 >
RE: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane
> > > > +void drm_plane_enable_damage_clips(struct drm_plane *plane)
> > > > +{
> > > > + struct drm_device *dev = plane->dev;
> > > > + struct drm_mode_config *config = &dev->mode_config;
> > > > +
> > > > + drm_object_attach_property(&plane->base, config-
> > > >prop_damage_clips, 0);
> > > > +}
> > > > diff --git a/include/drm/drm_mode_config.h
> > > b/include/drm/drm_mode_config.h
> > > > index 7569f22..d8767da 100644
> > > > --- a/include/drm/drm_mode_config.h
> > > > +++ b/include/drm/drm_mode_config.h
> > > > @@ -628,6 +628,21 @@ struct drm_mode_config {
> > > > */
> > > > struct drm_property *prop_crtc_id;
> > > > /**
> > > > +* @prop_damage_clips: Optional plane property to mark damaged
> > > regions
> > > > +* on the plane in framebuffer coordinates of the framebuffer
> > > attached
> > > > +* to the plane.
> > >
> > > Why should we make this optional? Looks like just another thing drivers
> > > might screw up, since we have multiple callbacks and things to set up for
> > > proper dirty tracking.
> >
> > Thanks Daniel for the review.
> >
> > I think not all compositor will be interested in sending damage, that was
> > the
> > reason to make this optional. Also when damage is not set that means
> > user-space need full update just like eglSwapBuffersWithDamageKHR.
> >
> > I will add better documentation.
>
> I think if we also handle this case in the helper that'd be even better:
> In the case of no damage, the helper/core code could automatically supply
> a damage rect for the entire buffer. That way drivers don't have to handle
> this case specially.
> -Daniel
Agreed.
> --
RE: [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb
> >>> 1) Expose a dirty (or content_age property) > >>> 2) Attach a clip-rect blob property. > >>> 3) Fake a page-flip by ping-ponging two frame-buffers pointing to the > >>> same > >>> underlying buffer object. > >>> > >>> Are you saying that people are already using 3) and we should keep > using > >>> that? > >> > >> I'm saying they're using 3b), flip the same buffer wrapped in the same > >> drm_framebuffer, and expect it to work. > >> > >> The only advantage dirtyfb has is that it allows you to supply the > >> optimized upload rectangles, but at the cost of a funny api (it's working > >> on the fb, not the plane/crtc you want to upload) and lack of drm_event > to > >> confirm when exactly you uploaded your stuff. But imo they should be > the > >> same underlying operation. > >> > > > > FWIW, vmwgfx has always treated a dirtyfb as a pure front-buffer like > > rendering operation without any synchronization, > > We've guaranteed that only the rects that are present are uploaded, but > only > > xf86-video-vmware has taken advantage of this to keep > > CPU- and GPU rendered content apart. > > I think we've at some point run into problems with number of cliprects, > (Old > > KDE lock screen?) and use multiple dirtyfb for the same > > update... > > Ok, if we can hit this in practices then I think it's ok to have the > limit. Just need to make sure userspace actually condenses the > cliprects down to something within the limit, since with atomic flips > you can't just do multiple updates - that would tear badly. So I think the conclusion is to have damage clip rect limit with proper documentation stating limitation of doing multiple updates. > > Wrt not syncing: I think general use pretty clearly says lots of > userspace renders into buffers with gpus (not even necessarily your > own) and then expects dirtyfb or a flip to upload all the bits. > Otherwise Rob Clark wouldn't need his patch. Given that I think we > need to make general semantics follow that requirement - I don't > expect it'll harm vmwgfx since it doesn't render into the frontbuffer > anyway? > > > IIRC the reason for working with the fb, is that it's much easier for > > user-space, which doesn't have to care where planes are scanning out and > > why. > > "Present my new content on any screen that's affected". > > Yeah, dirtyfb makes tons of sense for frontbuffer-rendering X, which > also doesn't do per-scanout pixmaps. But for atomic flips you really > want to flip on the crtc (or well plane), since otherwise with > multiple planes it comes up all teared up. vmwgfx doesn't care I guess > since you only have 1 primary plane, but all the SoC chips very much > do. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - https://urldefense.proofpoint.com/v2/url?u=http- > 3A__blog.ffwll.ch&d=DwIBaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK > 0762SxAf-cyZdStnd2NQpRu98lJP2iYGw&m=XKgN7GPElFapBWdozPSC- > 9rcfKmDvQC1QHhsFghexWc&s=SH9q5tw- > UqpUBJVrr2v1mLpRo28Aau7iJ3YWlrgbPmU&e=
RE: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.
> plane damage. > > On 04/05/2018 09:52 AM, Daniel Vetter wrote: > > > > TYPE_PLANE I have no idea who needs that. I suggest we just drop it. > > I'm assuming CRTC plane coordinates here. They are used for uploading > contents of hardware planes. Like, in the simplest case, cursor images. Yes they are CRTC plane coordinates, so is TYPE_PLANE naming confusing ? And should be named to TYPE_CRTC_PLANE but then it is confusing with TYPE_CRTC. > > /Thomas
RE: [RFC 3/3] drm: Add helper to validate damage during modeset_check
>
> On Wed, Apr 04, 2018 at 04:49:08PM -0700, Deepak Rawat wrote:
> > This patch adds a helper which should be called by driver which enable
> > damage (by calling drm_plane_enable_damage_clips) from atomic_check
> > hook. This helper for now set the damage to NULL for the planes on crtc
> > which need full modeset.
> >
> > The driver also need to check for other crtc properties which can
> > affect damage in atomic_check hook, like gamma, ctm, etc. Plane related
> > properties which affect damage can be handled in damage iterator.
> >
> > Signed-off-by: Deepak Rawat
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 47
> +
> > include/drm/drm_atomic_helper.h | 2 ++
> > 2 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> > index 355b514..23f44ab 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3987,3 +3987,50 @@ drm_atomic_helper_damage_iter_next(struct
> drm_atomic_helper_damage_iter *iter,
> > return true;
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
> > +
> > +/**
> > + * drm_atomic_helper_check_damage - validate state object for damage
> changes
> > + * @dev: DRM device
> > + * @state: the driver state object
> > + *
> > + * Check the state object if damage is required or not. In case damage is
> not
> > + * required e.g. need modeset, the damage blob is set to NULL.
>
> Why is that needed?
>
> I can imagine that drivers unconditionally upload everything for a
> modeset, and hence don't need special damage tracking. But for that it's
> imo better to have a clear_damage() helper.
Don't we need a generic helper which all drivers can use to see if anything
in drm_atomic_state will result in full update? My intention for calling that
function from atomic_check hook was because state access can
return -EDEADLK.
I think for now having drm_damage_helper_clear_damage helper and
calling it from atomic_check seems better option. In future once I have
clear idea, a generic function can be done.
>
> But even for modesets (e.g. resolution changes) I'd expect that virtual
> drivers don't want to upload unecessary amounts of data. Manual upload
> hw drivers probably need to upload everything, because the panel will have
> forgotten all the old data once power cycled.
AFAIK current vmwgfx will do full upload for resolution change.
>
> And if you think this is really the right thing, then we need to rename
> the function to tell what it does, e.g.
>
> drm_damage_helper_clear_damage_on_modeset()
>
> drm_damage_helper because I think we should stuff this all into
> drm_damage_helper.c, see previous patch.
>
> But I think a
>
> drm_damage_helper_clear_damage(crtc_state)
>
> which you can use in your crtc->atomic_check function like
>
> crtc_atomic_check(state)
> {
> if (drm_atomic_crtc_needs_modeset(state))
> drm_damage_helper_clear_damage(state);
> }
>
> is more flexible and useful for drivers. There might be other cases where
> clearing damage is the right thing to do. Also, there's the question of
> whether no damage clips == full damage or not, so maybe we should call
> this helper full_damage() instead.
In my opinion if via proper documentation it is conveyed that no damage
means full-update the clear_damage makes sense.
> -Daniel
>
> > + *
> > + * NOTE: This helper is not called by core. Those driver which enable
> damage
> > + * using drm_plane_enable_damage_clips should call this from their
> @atomic_check
> > + * hook.
> > + */
> > +int drm_atomic_helper_check_damage(struct drm_device *dev,
> > + struct drm_atomic_state *state)
> > +{
> > + struct drm_crtc *crtc;
> > + struct drm_plane *plane;
> > + struct drm_crtc_state *crtc_state;
> > + unsigned plane_mask;
> > + int i;
> > +
> > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > + continue;
> > +
> > + plane_mask = crtc_state->plane_mask;
> > + plane_mask |= crtc->state->plane_mask;
> > +
> > + drm_for_each_plane_mask(plane, dev, plane_mask) {
> > + struct drm_plane_state *plane_state =
> > + drm_atomic_get_plane_state(state, plane);
> > +
> > + if (IS_ERR(plane_state))
> > + return PTR_ERR(plane_state);
> > +
> > + if (plane_state->damage_clips) {
> > + drm_property_blob_put(plane_state-
> >damage_clips);
> > + plane_state->damage_clips = NULL;
> > + plane_state->num_clips = 0;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_check_damage);
> > diff --git a/include/drm/drm_atomic_helper.h
> b/include/d
RE: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.
>
> On Wed, Apr 04, 2018 at 04:49:07PM -0700, Deepak Rawat wrote:
> > With damage property in drm_plane_state, this patch adds helper iterator
> > to traverse the damage clips. Iterator will return the damage rectangles
> > in framebuffer, plane or crtc coordinates as need by driver
> > implementation.
> >
> > Signed-off-by: Deepak Rawat
>
> I'd really like selftest/unittests for this stuff. There's an awful lot of
> cornercases in this here (for any of the transformed iterators at least),
> and unit tests is the best way to make sure we handle them all correctly.
>
> Bonus points if you integrate the new selftests into igt so intel CI can
> run them, seel igt/tests/drm_mm.c for an example. drm_mm selftest is also
> the framework I'd copy for this stuff.
>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 122
>
> > include/drm/drm_atomic_helper.h | 39
> > 2 files changed, 161 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> > index 55b44e3..355b514 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3865,3 +3865,125 @@ void
> __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj
> *obj
> > memcpy(state, obj->state, sizeof(*state));
> > }
> > EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state);
> > +
> > +/**
> > + * drm_atomic_helper_damage_iter_init - initialize the damage iterator
> > + * @iter: The iterator to initialize.
> > + * @type: Coordinate type caller is interested in.
> > + * @state: plane_state from which to iterate the damage clips.
> > + * @hdisplay: Width of crtc on which plane is scanned out.
> > + * @vdisplay: Height of crtc on which plane is scanned out.
> > + *
> > + * Initialize an iterator that is used to translate and clip a set of
> > damage
> > + * rectangles in framebuffer coordinates to plane and crtc coordinates.
> The type
> > + * argument specify which type of coordinate to iterate in.
> > + *
> > + * Returns: 0 on success and negative error code on error. If an error code
> is
> > + * returned then it means the plane state should not update.
> > + */
> > +int
> > +drm_atomic_helper_damage_iter_init(struct
> drm_atomic_helper_damage_iter *iter,
> > + enum
> drm_atomic_helper_damage_clip_type type,
> > + const struct drm_plane_state *state,
> > + uint32_t hdisplay, uint32_t vdisplay)
> > +{
> > + if (!state || !state->crtc || !state->fb)
> > + return -EINVAL;
> > +
> > + memset(iter, 0, sizeof(*iter));
> > + iter->clips = (struct drm_rect *)state->damage_clips->data;
> > + iter->num_clips = state->num_clips;
> > + iter->type = type;
> > +
> > + /*
> > +* Full update in case of scaling or rotation. In future support for
> > +* scaling/rotating damage clips can be added
> > +*/
> > + if (state->crtc_w != (state->src_w >> 16) ||
> > + state->crtc_h != state->src_h >> 16 || state->rotation != 0) {
> > + iter->curr_clip = iter->num_clips;
> > + return 0;
>
> Given there's no user of this I have no idea how this manages to provoke a
> full clip rect. selftest code would be perfect for stuff like this.
>
> Also, I think we should provide a full clip for the case of num_clips ==
> 0, so that driver code can simply iterate over all clips and doesn't ever
> have to handle the "no clip rects provided" case as a special case itself.
The notion was if iterator does not provide any clip rect then driver need a
full update but yes I will work on providing a full clip here.
>
> > + }
> > +
> > + iter->fb_src.x1 = 0;
> > + iter->fb_src.y1 = 0;
> > + iter->fb_src.x2 = state->fb->width;
> > + iter->fb_src.y2 = state->fb->height;
> > +
> > + iter->plane_src.x1 = state->src_x >> 16;
> > + iter->plane_src.y1 = state->src_y >> 16;
> > + iter->plane_src.x2 = iter->plane_src.x1 + (state->src_w >> 16);
> > + iter->plane_src.y2 = iter->plane_src.y1 + (state->src_h >> 16);
> > + iter->translate_plane_x = -iter->plane_src.x1;
> > + iter->translate_plane_y = -iter->plane_src.y1;
> > +
> > + /* Clip plane src rect to fb dimensions */
> > + drm_rect_intersect(&iter->plane_src, &iter->fb_src);
>
> This smells like driver bug. Also, see Ville's recent efforts to improve
> the atomic plane clipping, I think drm_plane_state already has all the
> clip rects you want.
>
> > +
> > + iter->crtc_src.x1 = 0;
> > + iter->crtc_src.y1 = 0;
> > + iter->crtc_src.x2 = hdisplay;
> > + iter->crtc_src.y2 = vdisplay;
> > + iter->translate_crtc_x = -(iter->plane_src.x1 - state->crtc_x);
> > + iter->translate_crtc_x = -(iter->plane_src.y1 - state->crtc_y);
> > +
> > + /* Clip crtc src rect to plane dimensions */
> > + drm_rect_translate(&iter->crtc_src, -iter->translate_crtc_x,
> > + -iter->translate_cr
RE: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane
>
> On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > From: Lukasz Spintzyk
> >
> > Optional plane property to mark damaged regions on the plane in
> > framebuffer coordinates of the framebuffer attached to the plane.
> >
> > The layout of blob data is simply an array of drm_mode_rect with
> maximum
> > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > coordinates, damage clips are not in 16.16 fixed point.
> >
> > Damage clips are a hint to kernel as which area of framebuffer has
> > changed since last page-flip. This should be helpful for some drivers
> > especially for virtual devices where each framebuffer change needs to
> > be transmitted over network, usb, etc.
> >
> > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > plane should enable this property using drm_plane_enable_damage_clips.
> >
> > Signed-off-by: Lukasz Spintzyk
> > Signed-off-by: Deepak Rawat
>
> The property uapi section is missing, see:
>
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-
> 2Dcomposition-
> 2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762
> SxAf-
> cyZdStnd2NQpRu98lJP2iYGw&m=ELAUsNTjD0ICwUEDFjPW4jsmy2A5AkhS5Q
> z_3vlEC9Q&s=nH-HNXPczoJQMj1iwHiGfrhXz4-00b0M8-3kirjm-EY&e=
>
> Plane composition feels like the best place to put this. Please use that
> section to tie all the various bits together, including the helpers you're
> adding in the following patches for drivers to use.
>
> Bunch of nitpicks below, but overall I'm agreeing now with just going with
> fb coordinate damage rects.
>
> Like you say, the thing needed here now is userspace + driver actually
> implementing this. I'd also say the compat helper to map the legacy
> fb->dirty to this new atomic way of doing things should be included here
> (gives us a lot more testing for these new paths).
>
> Icing on the cake would be an igt to make sure kernel rejects invalid clip
> rects correctly.
>
> > ---
> > drivers/gpu/drm/drm_atomic.c| 42
> +
> > drivers/gpu/drm/drm_atomic_helper.c | 4
> > drivers/gpu/drm/drm_mode_config.c | 5 +
> > drivers/gpu/drm/drm_plane.c | 12 +++
> > include/drm/drm_mode_config.h | 15 +
> > include/drm/drm_plane.h | 16 ++
> > include/uapi/drm/drm_mode.h | 15 +
> > 7 files changed, 109 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c
> b/drivers/gpu/drm/drm_atomic.c
> > index 7d25c42..9226d24 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct
> drm_printer *p,
> > }
> >
> > /**
> > + * drm_atomic_set_damage_for_plane - sets the damage clips property
> to plane
> > + * @state: plane state
> > + * @blob: damage clips in framebuffer coordinates
> > + *
> > + * Returns:
> > + *
> > + * Zero on success, error code on failure.
> > + */
> > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state
> *state,
> > + struct drm_property_blob *blob)
> > +{
> > + if (blob == state->damage_clips)
> > + return 0;
> > +
> > + drm_property_blob_put(state->damage_clips);
> > + state->damage_clips = NULL;
> > +
> > + if (blob) {
> > + uint32_t count = blob->length/sizeof(struct drm_rect);
> > +
> > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > + return -EINVAL;
> > +
> > + state->damage_clips = drm_property_blob_get(blob);
> > + state->num_clips = count;
> > + } else {
> > + state->damage_clips = NULL;
> > + state->num_clips = 0;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > * drm_atomic_get_plane_state - get plane state
> > * @state: global atomic state object
> > * @plane: plane to get state object for
> > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
> > state->color_encoding = val;
> > } else if (property == plane->color_range_property) {
> > state->color_range = val;
> > + } else if (property == config->prop_damage_clips) {
> > + struct drm_property_blob *blob =
> > + drm_property_lookup_blob(dev, val);
> > + int ret = drm_atomic_set_damage_for_plane(state, blob);
>
> There's already a helper with size-checking built-in, see
> drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips
> I'd
> just provide a little inline helper that does the
> blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).
>
> > + drm_property_blob_put(blob);
> > + return ret;
> > } else if (plane->funcs->atomic_set_property) {
> > return plane->funcs->atomic_set_property(plane, state,
> > property, val);
> > @@ -8
RE: [RFC 0/3] drm: page-flip with damage
> > On Wed, Apr 04, 2018 at 04:49:05PM -0700, Deepak Rawat wrote: > > Hi All, > > > > This is extension to Lukasz Spintzyk earlier draft of damage interface for > drm. > > Bascially a new plane property is added called "DAMAGE_CLIPS" which is > simply > > an array of drm_rect (exported to userspace as drm_mode_rect). The clips > > represents damage in framebuffer coordinate of attached fb to the plane. > > > > Helper iterator is added to traverse the damage rectangles and get the > damage > > clips in framebuffer, plane or crtc coordinates as need by driver > > implementation. Finally a helper to reset damage in case need full update is > > required. Drivers interested in page-flip with damage should call this from > > atomic_check hook. > > > > With the RFC for atomic implementation of dirtyfb ioctl I was thinking > > should we need to consider dirty_fb flags, especially > > DRM_MODE_FB_DIRTY_ANNOTATE_COPY to be passed with atomic > > DAMAGE_CLIPS property blob? I didn't considered that untill now. If no > driver > > uses that in my opinion for simplicity this can be ignored? > > Last time I've checked no driver nor userspace really used this. I'd drop > it for the new uapi like you've done. Thanks Daniel for the review. Agreed, will drop dirty_fb flags unless someone has any objection. > > > About overlaping of damage rectangles is also not finalized. This really > > depends on driver specific implementation and can be left open-ended? > > Overlapping is userspace being stupid imo :-) I guess we should say that > drivers should at least not fall over overlapping rects, and if they can't > handle them, either coalesce into one big rect, or split them on their own > somehow. I think the best is to suggest user-space to not send overlapped rects. But as someone earlier suggested it is hard or I should say unnecessary check to enforce this in driver. > > > My knowledge is limited to vmwgfx so would like to hear about other > driver use > > cases and this can be modified in keeping other drivers need. > > > > Going forward driver implementation for vmwgfx and user-space > implementation > > of kmscube/weston will be next step to test the changes. > > I think an implementation for -modesetting and weston would be perfect. > Kmscube has very littel value for damage tracking purposes (it's not a > full compositor, just renders new frame each scene). And for kms I'm not a > fan of using vendor-specific drivers to demonstrate new generic uapi - way > too big chances you're making some vendor driver specific hardcoded > assumption that doesn't hold anywhere else. Also makes it harder for > others to test-implement your uapi in their drivers. > -Daniel Agreed implementation in modesetting and Weston and for kernel part as I read in another mail dirty_fb implementation is also a good use-case. > > > > > Thanks, > > Deepak > > > > Deepak Rawat (2): > > drm: Add helper iterator functions to iterate over plane damage. > > drm: Add helper to validate damage during modeset_check > > > > Lukasz Spintzyk (1): > > drm: Add DAMAGE_CLIPS property to plane > > > > drivers/gpu/drm/drm_atomic.c| 42 + > > drivers/gpu/drm/drm_atomic_helper.c | 173 > > > drivers/gpu/drm/drm_mode_config.c | 5 ++ > > drivers/gpu/drm/drm_plane.c | 12 +++ > > include/drm/drm_atomic_helper.h | 41 + > > include/drm/drm_mode_config.h | 15 > > include/drm/drm_plane.h | 16 > > include/uapi/drm/drm_mode.h | 15 > > 8 files changed, 319 insertions(+) > > > > -- > > 2.7.4 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > https://urldefense.proofpoint.com/v2/url?u=http- > 3A__blog.ffwll.ch&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK > 0762SxAf- > cyZdStnd2NQpRu98lJP2iYGw&m=CrefGziKAEk50MpTaNv64iDljHY7GesUlFHZc > hWpiqI&s=gFk0ko5cD_cnIyPuZOrqyAwLsmTt9PiSDbWdHoi-8cU&e=
RE: [PATCH] drm/vmwgfx: fix memory corruption with legacy/sou connectors
Thanks Rob for finding this one.
Reviewed-by: Deepak Rawat
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf
> Of Rob Clark
> Sent: Wednesday, January 17, 2018 7:16 AM
> To: dri-de...@lists.freedesktop.org
> Cc: Thomas Hellstrom ; Rob Clark
> ; David Airlie ; linux-
> ker...@vger.kernel.org; sta...@vger.kernel.org; linux-graphics-maintainer
>
> Subject: [PATCH] drm/vmwgfx: fix memory corruption with legacy/sou
> connectors
>
> From: Rob Clark
>
> It looks like in all cases 'struct vmw_connector_state' is used. But
> only in stdu connectors, was atomic_{duplicate,destroy}_state() properly
> subclassed. Leading to writes beyond the end of the allocated connector
> state block and all sorts of fun memory corruption related crashes.
>
> Fixes: d7721ca71126 "drm/vmwgfx: Connector atomic state"
> Cc:
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 4 ++--
> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> index b8a09807c5de..3824595fece1 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> @@ -266,8 +266,8 @@ static const struct drm_connector_funcs
> vmw_legacy_connector_funcs = {
> .set_property = vmw_du_connector_set_property,
> .destroy = vmw_ldu_connector_destroy,
> .reset = vmw_du_connector_reset,
> - .atomic_duplicate_state =
> drm_atomic_helper_connector_duplicate_state,
> - .atomic_destroy_state =
> drm_atomic_helper_connector_destroy_state,
> + .atomic_duplicate_state = vmw_du_connector_duplicate_state,
> + .atomic_destroy_state = vmw_du_connector_destroy_state,
> .atomic_set_property = vmw_du_connector_atomic_set_property,
> .atomic_get_property = vmw_du_connector_atomic_get_property,
> };
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> index bc5f6026573d..63a4cd794b73 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> @@ -420,8 +420,8 @@ static const struct drm_connector_funcs
> vmw_sou_connector_funcs = {
> .set_property = vmw_du_connector_set_property,
> .destroy = vmw_sou_connector_destroy,
> .reset = vmw_du_connector_reset,
> - .atomic_duplicate_state =
> drm_atomic_helper_connector_duplicate_state,
> - .atomic_destroy_state =
> drm_atomic_helper_connector_destroy_state,
> + .atomic_duplicate_state = vmw_du_connector_duplicate_state,
> + .atomic_destroy_state = vmw_du_connector_destroy_state,
> .atomic_set_property = vmw_du_connector_atomic_set_property,
> .atomic_get_property = vmw_du_connector_atomic_get_property,
> };
> --
> 2.14.3
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lists.freedesktop.org_mailman_listinfo_dri-
> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
> -
> cyZdStnd2NQpRu98lJP2iYGw&m=h6TncxsxWnJOk_7aZZClV8O_VZGw8rtrIk_
> BKtLUKM0&s=bizsNwniM18rOqG6MjlSY5t9fPsmxFrMgN_UqnI0vP0&e=
