Re: [PATCH v2 1/2] drm/vmwgfx: add local DRM_AUTH check for PRIME TO/FROM HANDLE
Hi Emil,
Thanks for doing this. Looks good to me. By the way I think Thomas had
a patch to get rid of legacy locking mechanism. I don't know when it
will go upstream. With that we no need for the below check.
Reviewed-by: Deepak Rawat
On Fri, 2019-08-02 at 18:01 +0100, Emil Velikov wrote:
> From: Emil Velikov
>
> Realistically no drivers, but vmwgfx care about the DRM_AUTH flag
> here.
>
> Follow-up work in this driver will properly isolate primary clients
> from
> different master realms, thus we'll no longer need to parse _any_
> ioctl
> flags.
>
> Until that work lands, add a local workaround.
>
> v2: Use bitwise or (Deepak)
>
> Cc: VMware Graphics
> Cc: Thomas Hellstrom
> Cc: Deepak Singh Rawat
> Signed-off-by: Emil Velikov
> ---
> I'd like to merge this and the next patch hrough the drm-misc tree.
> Ack
> and rb are appreciated.
>
> Thanks
> Emil
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index cd0d49d8a8da..53afb1d597e8 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1134,6 +1134,15 @@ static long vmw_generic_ioctl(struct file
> *filp, unsigned int cmd,
> } else if (!drm_ioctl_flags(nr, &flags))
> return -EINVAL;
>
> + /*
> + * Little workaround until the vmwgfx patches providing
> isolation of
> + * primary clients from different master realms land.
> + * With that work, we'll no longer need to parse _any_ ioctl
> flags.
> + */
> + if (nr == 0x2d /* DRM_IOCTL_PRIME_HANDLE_TO_FD */ ||
> + nr == 0x2e /* DRM_IOCTL_PRIME_FD_TO_HANDLE */)
> + flags |= DRM_AUTH;
> +
> vmaster = vmw_master_check(dev, file_priv, flags);
> if (IS_ERR(vmaster)) {
> ret = PTR_ERR(vmaster);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Linux-graphics-maintainer] [PATCH 2/3] drm/vmwgfx: add local DRM_AUTH check for PRIME TO/FROM HANDLE
On Mon, 2019-07-22 at 18:40 +0100, Emil Velikov wrote:
> From: Emil Velikov
>
> Realistically no drivers, but vmwgfx care about the DRM_AUTH flag
> here.
>
> Follow-up work in this driver will properly isolate primary clients
> from
> different master realms, thus we'll no longer need to parse _any_
> ioctl
> flags.
>
> Until that work lands, add a local workaround.
>
> Cc: VMware Graphics
> Cc: Thomas Hellstrom
> Signed-off-by: Emil Velikov
> ---
> I'd like to merge this through the drm-misc tree. Ack and rb are
> appreciated.
>
> Thanks
> Emil
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 275d90fe2a25..32c18bb482a6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1131,6 +1131,15 @@ static long vmw_generic_ioctl(struct file
> *filp, unsigned int cmd,
> } else if (!drm_ioctl_flags(nr, &flags))
> return -EINVAL;
>
> + /*
> + * Little workaround until the vmwgfx patches providing
> isolation of
> + * primary clients from different master realms lands.
> + * With that work, we'll no longer need to parse _any_ ioctl
> flags.
> + */
> + if (nr == 0x2d /* DRM_IOCTL_PRIME_HANDLE_TO_FD */ ||
> + nr == 0x2e /* DRM_IOCTL_PRIME_FD_TO_HANDLE */)
> + flags != DRM_AUTH;
Do you mean bitwise OR assignment? In current form this is no-op.
> +
> vmaster = vmw_master_check(dev, file_priv, flags);
> if (IS_ERR(vmaster)) {
> ret = PTR_ERR(vmaster);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/vmwgfx: add local DRM_AUTH check for PRIME TO/FROM HANDLE
> Hi Deepak, > > As far as I can tell Thomas is on holidays for another 2+ weeks. > > Is there anyone else in the team who can review the VMWare patches of > this series? I tested the lot quickly, but additional confirmation > would > be appreciated. > > You can find the series via the "VMware Graphics" alias, or in the > patchwork link below. Hi Emil, I can look into your patches and I did had a cursory look at those and to be honest I don't really know this area and also since it deals with security I thought a RB from Thomas would be nice. I will devote some more time on your patches. Thanks for doing this. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH][next] drm/vmwgfx: remove redundant assignment to sub_res
Hi Colin,
Reviewed-by: Deepak Rawat
Thanks,
Deepak
On Mon, 2019-06-24 at 23:44 +0100, Colin King wrote:
> From: Colin Ian King
>
> Variable sub_res is initialized to a value that is never read and it
> is re-assigned later in a for-loop. The initialization is redundant
> and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index 862ca44680ca..3257ba689d93 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> @@ -1914,7 +1914,7 @@ static void
> vmw_surface_tex_dirty_range_add(struct vmw_resource *res,
> } else {
> /* Dirty range covers multiple sub-resources */
> struct svga3dsurface_loc loc_min, loc_max;
> - u32 sub_res = loc1.sub_resource;
> + u32 sub_res;
>
> svga3dsurface_max_loc(cache, loc1.sub_resource,
> &loc_max);
> vmw_subres_dirty_add(dirty, &loc1, &loc_max);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/2] drm/vmwgfx: drop use of drmP.h
Hi Sam, Thanks for doing this. Reviewed-by: Deepak Rawat I did some compile/basic run test on your patchs. Things look fine. Will include this for vmwgfx-next so can run more automated test. On Sun, 2019-06-23 at 12:23 +0200, Sam Ravnborg wrote: > In two steps drop the use of drmP.h > First patch remove drmP.h from header files and fixes fallout. > Second patch remove drmP.h from the remaining files. > > While touching the list of include files divide them in blocks > and sort include files within the blocks. > > Patches made on top of drm-misc-next, and checked that > they apply to vmwgfx-fixes-5.2 in > git://people.freedesktop.org/~thomash/linux > > Build tested with various configs with several architectures. > > v2: > - fix warning in i386 build reported by 0-day > (missing include) > > Sam > > Sam Ravnborg (2): > drm/vmwgfx: drop use of drmP.h in header files > drm/vmwgfx: drop reminaing users of drmP.h > > drivers/gpu/drm/vmwgfx/ttm_lock.h | 2 +- > drivers/gpu/drm/vmwgfx/ttm_object.h| 7 --- > drivers/gpu/drm/vmwgfx/vmwgfx_binding.h| 3 ++- > drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 1 - > drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c | 3 +++ > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c| 17 +++-- > drivers/gpu/drm/vmwgfx/vmwgfx_drv.h| 30 > +- > drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 8 > drivers/gpu/drm/vmwgfx/vmwgfx_fence.c | 3 ++- > drivers/gpu/drm/vmwgfx/vmwgfx_fence.h | 5 - > drivers/gpu/drm/vmwgfx/vmwgfx_fifo.c | 6 -- > drivers/gpu/drm/vmwgfx/vmwgfx_gmr.c| 4 ++-- > drivers/gpu/drm/vmwgfx/vmwgfx_irq.c| 3 ++- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c| 10 +++--- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.h| 2 +- > drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c| 6 -- > drivers/gpu/drm/vmwgfx/vmwgfx_mob.c| 2 ++ > drivers/gpu/drm/vmwgfx/vmwgfx_msg.c| 11 +-- > drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c| 6 ++ > drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 5 ++--- > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 6 -- > drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 9 ++--- > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 1 - > drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 3 ++- > 24 files changed, 95 insertions(+), 58 deletions(-) > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vmwgfx: fix memory leak when too many retries have occurred
Hi Colin,
Thanks for doing this.
Reviewed-by: Deepak Rawat
Do you want me to include this in vmwgfx-next or will you submit this
via drm-mics?
On Fri, 2019-06-21 at 23:35 +0100, Colin King wrote:
> From: Colin Ian King
>
> Currently when too many retries have occurred there is a memory
> leak on the allocation for reply on the error return path. Fix
> this by kfree'ing reply before returning.
>
> Addresses-Coverity: ("Resource leak")
> Fixes: a9cd9c044aa9 ("drm/vmwgfx: Add a check to handle host message
> failure")
> Signed-off-by: Colin Ian King
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> index 8b9270f31409..8b61f16f50cf 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> @@ -301,8 +301,10 @@ static int vmw_recv_msg(struct rpc_channel
> *channel, void **msg,
> break;
> }
>
> - if (retries == RETRIES)
> + if (retries == RETRIES) {
> + kfree(reply);
> return -EINVAL;
> + }
>
> *msg_len = reply_len;
> *msg = reply;
Re: [PATCH] drm/vmwgfx: Don't look at state->allow_modeset
Reviewed-by: Deepak Rawat
On Tue, 2019-05-21 at 00:35 +0200, Daniel Vetter wrote:
> That's purely for the uapi layer to implement the ALLOW_MODESET flag.
>
> Drivers should instead look at the state, e.g. through
> drm_atomic_crtc_needs_modeset(), which vmwgfx already does. Also
> remove
> the confusing comment, since checking allow_modeset is at best a
> micro
> optimization.
>
> v2: Rebase
>
> Signed-off-by: Daniel Vetter
> Cc: VMware Graphics
> Cc: Thomas Hellstrom
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index b97bc8e5944b..34284f0f5084 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -1704,14 +1704,6 @@ vmw_kms_atomic_check_modeset(struct drm_device
> *dev,
> if (ret)
> return ret;
>
> - if (!state->allow_modeset)
> - return ret;
> -
> - /*
> - * Legacy path do not set allow_modeset properly like
> - * @drm_atomic_helper_update_plane, This will result in
> unnecessary call
> - * to vmw_kms_check_topology. So extra set of check.
> - */
> for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> if (drm_atomic_crtc_needs_modeset(crtc_state))
> need_modeset = true;
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: linux-next: Signed-off-by missing for commit in the drm tree
On Wed, 2019-04-24 at 11:12 +1000, Stephen Rothwell wrote:
> Hi Deepak,
>
> On Wed, 24 Apr 2019 00:36:19 +0000 Deepak Singh Rawat <
> dra...@vmware.com> wrote:
> >
> > On Wed, 2019-04-24 at 10:22 +1000, Stephen Rothwell wrote:
> > >
> > > Commit
> > >
> > > a9f58c456e9d ("drm/vmwgfx: Be more restrictive when dirtying
> > > resources")
> >
> > Actully the Signed-off-by: is not missing see
> >
> >
https://cgit.freedesktop.org/drm/drm/commit/drivers/gpu/drm/vmwgfx?id=a9f58c456e9dde6f272e7be4d6bed607fd7008aa
>
> There is no Signed-off-by from you as the committer (just a
> Reviewed-by from you and a Signed-off-by from the author).
Sorry missed that part and missed your email context about commiter and
not the author. I think at this point nothing can be done?
>
> > By the way there is the patter "---" before Signed-off-by:(which is
> > same as used by patch to separate description from code) and I
> > think
> > the script is reading only till that point. I think "---" should
> > not be
> > used in patch description.
>
> All the tags should be *before* the "-- " or "---" separator.
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: linux-next: Signed-off-by missing for commit in the drm tree
On Wed, 2019-04-24 at 10:22 +1000, Stephen Rothwell wrote:
> Hi all,
>
> Commit
>
> a9f58c456e9d ("drm/vmwgfx: Be more restrictive when dirtying
> resources")
Hi Stephen,
Actully the Signed-off-by: is not missing see
https://cgit.freedesktop.org/drm/drm/commit/drivers/gpu/drm/vmwgfx?id=a9f58c456e9dde6f272e7be4d6bed607fd7008aa
By the way there is the patter "---" before Signed-off-by:(which is
same as used by patch to separate description from code) and I think
the script is reading only till that point. I think "---" should not be
used in patch description.
>
> is missing a Signed-off-by from its committer.
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [GIT PULL] drm/vmwgfx: vmwgfx-next
On Wed, 2019-04-24 at 06:55 +1000, Dave Airlie wrote: > I'll merge it, but it appears patchwork failed to find this pull > request. > > What version of git was used to generate it? git version 2.17.1 > > Dave. > > On Sat, 20 Apr 2019 at 03:36, Deepak Singh Rawat > wrote: > > > > It seems this got missed, If no one has any objection I will submit > > the > > patches via drm-mics route. > > > > Deepak > > > > On Tue, 2019-04-09 at 12:49 -0700, Deepak Singh Rawat wrote: > > > Hi Daniel/Dave, > > > > > > The vmwgfx-next changes for 5.2: > > > Resource dirtying improvement by Thomas, > > > user-space error logging improvement and > > > some other minor fixes. > > > > > > The following changes since commit > > > 14d2bd53a47a7e1cb3e03d00a6b952734cf90f3f: > > > > > > Merge tag 'drm-misc-next-2019-04-04' of > > > git://anongit.freedesktop.org/drm/drm-misc into drm-next (2019- > > > 04-05 > > > 11:38:02 +1000) > > > > > > are available in the Git repository at: > > > > > > > > > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrawat%2Flinux.git&data=02%7C01%7Cdrawat%40vmware.com%7C9de1ab6df162408f97bb08d6c82e183e%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636916497680322201&sdata=2H3UYxG0oP0Xm1M8OEFS9EL6YJzxzBS%2FxK2XRGVJ%2Fmg%3D&reserved=0 > > > vmwgfx-next > > > > > > for you to fetch changes up to > > > c601b12fb634e2d0c2669706b38dba98a3c3a832: > > > > > > drm/vmwgfx: Remove set but not used variable 'fb_offset, > > > fb_depth' > > > (2019-04-08 10:29:05 -0700) > > > > > > > > > Chengguang Xu (1): > > > drm/vmwgfx: remove redundant unlikely annotation > > > > > > Deepak Rawat (8): > > > drm/vmwgfx: Use preprocessor macro to get valid context > > > node > > > drm/vmwgfx: Use preprocessor macro for cmd struct > > > drm/vmwgfx: Add a new define for vmwgfx user-space > > > debugging > > > drm/vmwgfx: Print message when command verifier returns > > > with > > > error > > > drm/vmwgfx: Clean up some debug messages in > > > vmwgfx_execbuf.c > > > drm/vmwgfx: Use VMW_DEBUG_USER for device command buffer > > > errors > > > drm/vmwgfx: Fix formatting and spaces in vmwgfx_execbuf.c > > > drm/vmwgfx: Use preprocessor macro for FIFO allocation > > > > > > Nathan Chancellor (1): > > > drm/vmwgfx: Zero initialize handle in vmw_execbuf_process > > > > > > Thomas Hellstrom (1): > > > drm/vmwgfx: Be more restrictive when dirtying resources > > > > > > YueHaibing (2): > > > drm/vmwgfx: Remove set but not used variable 'restart' > > > drm/vmwgfx: Remove set but not used variable 'fb_offset, > > > fb_depth' > > > > > > drivers/gpu/drm/vmwgfx/vmwgfx_binding.c | 98 + > > > drivers/gpu/drm/vmwgfx/vmwgfx_binding.h |2 + > > > drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c | 24 ++- > > > drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 59 ++ > > > drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c | 23 +-- > > > drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 29 ++- > > > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 1505 > > > +++ > > > > > > --- > > > > > > > > > drivers/gpu/drm/vmwgfx/vmwgfx_fb.c |4 +- > > > drivers/gpu/drm/vmwgfx/vmwgfx_fifo.c| 27 +-- > > > drivers/gpu/drm/vmwgfx/vmwgfx_gmr.c |9 +- > > > drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 12 +- > > > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 28 ++- > > > drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c |6 +- > > > drivers/gpu/drm/vmwgfx/vmwgfx_mob.c | 25 +-- > > > drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c |4 +- > > > drivers/gpu/drm/vmwgfx/vmwgfx_resource.c| 28 ++- > > > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c| 23 +-- > > > drivers/gpu/drm/vmwgfx/vmwgfx_shader.c | 44 ++--- > > > dr
Re: [PATCH 9/9] drm/vmwgfx: Add surface dirty-tracking callbacks
Minor nits below
Reviewed-by: Deepak Rawat
On Fri, 2019-04-12 at 16:04 +, Thomas Hellstrom wrote:
> Add the callbacks necessary to implement emulated coherent memory for
> surfaces. Add a flag to the gb_surface_create ioctl to indicate that
> surface memory should be coherent.
> Also bump the drm minor version to signal the availability of
> coherent
> surfaces.
>
> Signed-off-by: Thomas Hellstrom
> ---
> .../device_include/svga3d_surfacedefs.h | 209 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 4 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 390
> +-
> include/uapi/drm/vmwgfx_drm.h | 4 +-
> 4 files changed, 600 insertions(+), 7 deletions(-)
>
> diff --git
> a/drivers/gpu/drm/vmwgfx/device_include/svga3d_surfacedefs.h
> b/drivers/gpu/drm/vmwgfx/device_include/svga3d_surfacedefs.h
> index f2bfd3d80598..d901206c04e3 100644
> --- a/drivers/gpu/drm/vmwgfx/device_include/svga3d_surfacedefs.h
> +++ b/drivers/gpu/drm/vmwgfx/device_include/svga3d_surfacedefs.h
> @@ -1280,7 +1280,6 @@
> svga3dsurface_get_pixel_offset(SVGA3dSurfaceFormat format,
> return offset;
> }
>
> -
> static inline u32
> svga3dsurface_get_image_offset(SVGA3dSurfaceFormat format,
> surf_size_struct baseLevelSize,
> @@ -1375,4 +1374,212 @@
> svga3dsurface_is_screen_target_format(SVGA3dSurfaceFormat format)
> return svga3dsurface_is_dx_screen_target_format(format);
> }
>
> +/**
> + * struct svga3dsurface_mip - Mimpmap level information
> + * @bytes: Bytes required in the backing store of this mipmap level.
> + * @img_stride: Byte stride per image.
> + * @row_stride: Byte stride per block row.
> + * @size: The size of the mipmap.
> + */
> +struct svga3dsurface_mip {
> + size_t bytes;
> + size_t img_stride;
> + size_t row_stride;
> + struct drm_vmw_size size;
> +
> +};
> +
> +/**
> + * struct svga3dsurface_cache - Cached surface information
> + * @desc: Pointer to the surface descriptor
> + * @mip: Array of mipmap level information. Valid size is
> @num_mip_levels.
> + * @mip_chain_bytes: Bytes required in the backing store for the
> whole chain
> + * of mip levels.
> + * @num_mip_levels: Valid size of the @mip array. Number of mipmap
> levels in
> + * a chain.
> + * @num_layers: Number of slices in an array texture or number of
> faces in
> + * a cubemap texture.
> + */
> +struct svga3dsurface_cache {
> + const struct svga3d_surface_desc *desc;
> + struct svga3dsurface_mip mip[DRM_VMW_MAX_MIP_LEVELS];
> + size_t mip_chain_bytes;
> + u32 num_mip_levels;
> + u32 num_layers;
> +};
> +
> +/**
> + * struct svga3dsurface_loc - Surface location
> + * @sub_resource: Surface subresource. Defined as layer *
> num_mip_levels +
> + * mip_level.
> + * @x: X coordinate.
> + * @y: Y coordinate.
> + * @z: Z coordinate.
> + */
> +struct svga3dsurface_loc {
> + u32 sub_resource;
> + u32 x, y, z;
> +};
> +
> +/**
> + * svga3dsurface_subres - Compute the subresource from layer and
> mipmap.
> + * @cache: Surface layout data.
> + * @mip_level: The mipmap level.
> + * @layer: The surface layer (face or array slice).
> + *
> + * Return: The subresource.
> + */
> +static inline u32 svga3dsurface_subres(const struct
> svga3dsurface_cache *cache,
> +u32 mip_level, u32 layer)
> +{
> + return cache->num_mip_levels * layer + mip_level;
> +}
> +
> +/**
> + * svga3dsurface_setup_cache - Build a surface cache entry
> + * @size: The surface base level dimensions.
> + * @format: The surface format.
> + * @num_mip_levels: Number of mipmap levels.
> + * @num_layers: Number of layers.
> + * @cache: Pointer to a struct svga3dsurface_cach object to be
> filled in.
> + */
> +static inline void svga3dsurface_setup_cache(const struct
> drm_vmw_size *size,
> + SVGA3dSurfaceFormat
> format,
> + u32 num_mip_levels,
> + u32 num_layers,
> + u32 num_samples,
> + struct svga3dsurface_cache
> *cache)
> +{
> + const struct svga3d_surface_desc *desc;
> + u32 i;
> +
> + memset(cache, 0, sizeof(*cache));
> + cache->desc = desc = svga3dsurface_get_desc(format);
> + cache->num_mip_levels = num_mip_levels;
> + cache->num_layers = num_layers;
> + for (i = 0; i < cache->num_mip_levels; i++) {
> + struct svga3dsurface_mip *mip = &cache->mip[i];
> +
> + mip->size = svga3dsurface_get_mip_size(*size, i);
> + mip->bytes = svga3dsurface_get_image_buffer_size
> + (desc, &mip->size, 0) * num_samples;
> + mip->row_stride =
> + __KERNEL_DIV_ROUND_UP(mip->size.width,
> + desc->block_size.width) *
> + desc->bytes_per_block * num_samp
Re: [PATCH 8/9] drm/vmwgfx: Implement an infrastructure for read-coherent resources
Minor nits below, otherwise
Reviewed-by: Deepak Rawat
On Fri, 2019-04-12 at 09:04 -0700, Thomas Hellstrom wrote:
> Similar to write-coherent resources, make sure that from the user-
> space
> point of view, GPU rendered contents is automatically available for
> reading by the CPU.
>
> Signed-off-by: Thomas Hellstrom
> ---
> drivers/gpu/drm/ttm/ttm_bo_vm.c | 1 +
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 8 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c| 69 +++-
> drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 102
> +-
> drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h | 2 +
> drivers/gpu/drm/vmwgfx/vmwgfx_validation.c| 3 +-
> 6 files changed, 176 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index 3bd28fb97124..0065b138f450 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -42,6 +42,7 @@
> #include
> #include
>
> +
> static vm_fault_t ttm_bo_vm_fault_idle(struct ttm_buffer_object *bo,
> struct vm_fault *vmf)
> {
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 81ebcd668038..00794415335e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -96,6 +96,7 @@ struct vmw_fpriv {
> * @map: Kmap object for semi-persistent mappings
> * @res_prios: Eviction priority counts for attached resources
> * @dirty: structure for user-space dirty-tracking
> + * @cleaning: Current validation sequence is cleaning.
> */
> struct vmw_buffer_object {
> struct ttm_buffer_object base;
> @@ -690,7 +691,8 @@ extern void vmw_resource_unreference(struct
> vmw_resource **p_res);
> extern struct vmw_resource *vmw_resource_reference(struct
> vmw_resource *res);
> extern struct vmw_resource *
> vmw_resource_reference_unless_doomed(struct vmw_resource *res);
> -extern int vmw_resource_validate(struct vmw_resource *res, bool
> intr);
> +extern int vmw_resource_validate(struct vmw_resource *res, bool
> intr,
> + bool dirtying);
> extern int vmw_resource_reserve(struct vmw_resource *res, bool
> interruptible,
> bool no_backup);
> extern bool vmw_resource_needs_backup(const struct vmw_resource
> *res);
> @@ -734,6 +736,8 @@ void vmw_resource_mob_attach(struct vmw_resource
> *res);
> void vmw_resource_mob_detach(struct vmw_resource *res);
> void vmw_resource_dirty_update(struct vmw_resource *res, pgoff_t
> start,
> pgoff_t end);
> +int vmw_resources_clean(struct vmw_buffer_object *vbo, pgoff_t
> start,
> + pgoff_t end, pgoff_t *num_prefault);
>
> /**
> * vmw_resource_mob_attached - Whether a resource currently has a
> mob attached
> @@ -1428,6 +1432,8 @@ int vmw_bo_dirty_add(struct vmw_buffer_object
> *vbo);
> void vmw_bo_dirty_transfer_to_res(struct vmw_resource *res);
> void vmw_bo_dirty_clear_res(struct vmw_resource *res);
> void vmw_bo_dirty_release(struct vmw_buffer_object *vbo);
> +void vmw_bo_dirty_unmap(struct vmw_buffer_object *vbo,
> + pgoff_t start, pgoff_t end);
> vm_fault_t vmw_bo_vm_fault(struct vm_fault *vmf);
> vm_fault_t vmw_bo_vm_mkwrite(struct vm_fault *vmf);
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> index 87e4a73b1175..773ff30a4b60 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> @@ -153,7 +153,6 @@ static void vmw_bo_dirty_scan_mkwrite(struct
> vmw_buffer_object *vbo)
> }
> }
>
> -
> /**
> * vmw_bo_dirty_scan - Scan for dirty pages and add them to the
> dirty
> * tracking structure
> @@ -171,6 +170,51 @@ void vmw_bo_dirty_scan(struct vmw_buffer_object
> *vbo)
> vmw_bo_dirty_scan_mkwrite(vbo);
> }
>
> +/**
> + * vmw_bo_dirty_pre_unmap - write-protect and pick up dirty pages
> before
> + * an unmap_mapping_range operation.
> + * @vbo: The buffer object,
> + * @start: First page of the range within the buffer object.
> + * @end: Last page of the range within the buffer object + 1.
> + *
> + * If we're using the _PAGETABLE scan method, we may leak dirty
> pages
> + * when calling unmap_mapping_range(). This function makes sure we
> pick
> + * up all dirty pages.
> + */
> +static void vmw_bo_dirty_pre_unmap(struct vmw_buffer_object *vbo,
> +pgoff_t start, pgoff_t end)
> +{
> + struct vmw_bo_dirty *dirty = vbo->dirty;
> + unsigned long offset = drm_vma_node_start(&vbo->base.vma_node);
> + struct address_space *mapping = vbo->base.bdev->dev_mapping;
> +
> + if (dirty->method != VMW_BO_DIRTY_PAGETABLE || start >= end)
> + return;
> +
> + apply_as_wrprotect(mapping, start + offset, end - start);
> + apply_as_clean(mapping, start + offset
Re: [PATCH 7/9] drm/vmwgfx: Use an RBtree instead of linked list for MOB resources
Reviewed-by: Deepak Rawat
On Fri, 2019-04-12 at 09:04 -0700, Thomas Hellstrom wrote:
> With emulated coherent memory we need to be able to quickly look up
> a resource from the MOB offset. Instead of traversing a linked list
> with
> O(n) worst case, use an RBtree with O(log n) worst case complexity.
>
> Signed-off-by: Thomas Hellstrom
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 5 ++--
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 10 +++
> drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 33 +-
> --
> 3 files changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index 90ca866640fe..e8bc7a7ac031 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> @@ -464,6 +464,7 @@ void vmw_bo_bo_free(struct ttm_buffer_object *bo)
> struct vmw_buffer_object *vmw_bo = vmw_buffer_object(bo);
>
> WARN_ON(vmw_bo->dirty);
> + WARN_ON(!RB_EMPTY_ROOT(&vmw_bo->res_tree));
> vmw_bo_unmap(vmw_bo);
> kfree(vmw_bo);
> }
> @@ -480,6 +481,7 @@ static void vmw_user_bo_destroy(struct
> ttm_buffer_object *bo)
> struct vmw_buffer_object *vbo = &vmw_user_bo->vbo;
>
> WARN_ON(vbo->dirty);
> + WARN_ON(!RB_EMPTY_ROOT(&vbo->res_tree));
> vmw_bo_unmap(vbo);
> ttm_prime_object_kfree(vmw_user_bo, prime);
> }
> @@ -515,8 +517,7 @@ int vmw_bo_init(struct vmw_private *dev_priv,
> memset(vmw_bo, 0, sizeof(*vmw_bo));
> BUILD_BUG_ON(TTM_MAX_BO_PRIORITY <= 3);
> vmw_bo->base.priority = 3;
> -
> - INIT_LIST_HEAD(&vmw_bo->res_list);
> + vmw_bo->res_tree = RB_ROOT;
>
> ret = ttm_bo_init(bdev, &vmw_bo->base, size,
> ttm_bo_type_device, placement,
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index f05fce52fbb4..81ebcd668038 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -90,7 +90,7 @@ struct vmw_fpriv {
> /**
> * struct vmw_buffer_object - TTM buffer object with vmwgfx
> additions
> * @base: The TTM buffer object
> - * @res_list: List of resources using this buffer object as a
> backing MOB
> + * @res_tree: RB tree of resources using this buffer object as a
> backing MOB
> * @pin_count: pin depth
> * @dx_query_ctx: DX context if this buffer object is used as a DX
> query MOB
> * @map: Kmap object for semi-persistent mappings
> @@ -99,7 +99,7 @@ struct vmw_fpriv {
> */
> struct vmw_buffer_object {
> struct ttm_buffer_object base;
> - struct list_head res_list;
> + struct rb_root res_tree;
> s32 pin_count;
> /* Not ref-counted. Protected by binding_mutex */
> struct vmw_resource *dx_query_ctx;
> @@ -147,8 +147,8 @@ struct vmw_res_func;
> * pin-count greater than zero. It is not on the resource LRU lists
> and its
> * backup buffer is pinned. Hence it can't be evicted.
> * @func: Method vtable for this resource. Immutable.
> + * @mob_node; Node for the MOB backup rbtree. Protected by @backup
> reserved.
> * @lru_head: List head for the LRU list. Protected by
> @dev_priv::resource_lock.
> - * @mob_head: List head for the MOB backup list. Protected by
> @backup reserved.
> * @binding_head: List head for the context binding list. Protected
> by
> * the @dev_priv::binding_mutex
> * @res_free: The resource destructor.
> @@ -169,8 +169,8 @@ struct vmw_resource {
> unsigned long backup_offset;
> unsigned long pin_count;
> const struct vmw_res_func *func;
> + struct rb_node mob_node;
> struct list_head lru_head;
> - struct list_head mob_head;
> struct list_head binding_head;
> struct vmw_resource_dirty *dirty;
> void (*res_free) (struct vmw_resource *res);
> @@ -743,7 +743,7 @@ void vmw_resource_dirty_update(struct
> vmw_resource *res, pgoff_t start,
> */
> static inline bool vmw_resource_mob_attached(const struct
> vmw_resource *res)
> {
> - return !list_empty(&res->mob_head);
> + return !RB_EMPTY_NODE(&res->mob_node);
> }
>
> /**
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> index d35f4bd32cd9..ff9fe5650468 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> @@ -41,11 +41,24 @@
> void vmw_resource_mob_attach(struct vmw_resource *res)
> {
> struct vmw_buffer_object *backup = res->backup;
> + struct rb_node **new = &backup->res_tree.rb_node, *parent =
> NULL;
>
> lockdep_assert_held(&backup->base.resv->lock.base);
> res->used_prio = (res->res_dirty) ? res->func->dirty_prio :
> res->func->prio;
> - list_add_tail(&res->mob_head, &backup->res_list);
> +
> + while (*new) {
> + struct vmw_resource *this =
> + container_of(*new, struct vmw_resource,
> mob_node);
> +
> + parent = *
Re: [PATCH 6/9] drm/vmwgfx: Implement an infrastructure for write-coherent resources
Hi Thomas,
With minor comments below
Reviewed-by: Deepak Rawat
On Fri, 2019-04-12 at 09:04 -0700, Thomas Hellstrom wrote:
> This infrastructure will, for coherent resources, make sure that
> from the user-space point of view, data written by the CPU is
> immediately
> automatically available to the GPU at resource validation time.
>
> Signed-off-by: Thomas Hellstrom
> ---
> drivers/gpu/drm/vmwgfx/Kconfig| 1 +
> drivers/gpu/drm/vmwgfx/Makefile | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_bo.c| 5 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 5 +
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 26 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 1 -
> drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c| 410
> ++
> drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 57 +++
> drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h | 11 +
> drivers/gpu/drm/vmwgfx/vmwgfx_validation.c| 74
> drivers/gpu/drm/vmwgfx/vmwgfx_validation.h| 16 +-
> 11 files changed, 588 insertions(+), 20 deletions(-)
> create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
>
> diff --git a/drivers/gpu/drm/vmwgfx/Kconfig
> b/drivers/gpu/drm/vmwgfx/Kconfig
> index 6b28a326f8bb..d5fd81a521f6 100644
> --- a/drivers/gpu/drm/vmwgfx/Kconfig
> +++ b/drivers/gpu/drm/vmwgfx/Kconfig
> @@ -8,6 +8,7 @@ config DRM_VMWGFX
> select FB_CFB_IMAGEBLIT
> select DRM_TTM
> select FB
> + select AS_DIRTY_HELPERS
> # Only needed for the transitional use of drm_crtc_init - can
> be removed
> # again once vmwgfx sets up the primary plane itself.
> select DRM_KMS_HELPER
> diff --git a/drivers/gpu/drm/vmwgfx/Makefile
> b/drivers/gpu/drm/vmwgfx/Makefile
> index 8841bd30e1e5..c877a21a0739 100644
> --- a/drivers/gpu/drm/vmwgfx/Makefile
> +++ b/drivers/gpu/drm/vmwgfx/Makefile
> @@ -8,7 +8,7 @@ vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o
> vmwgfx_kms.o vmwgfx_drv.o \
> vmwgfx_cmdbuf_res.o vmwgfx_cmdbuf.o vmwgfx_stdu.o \
> vmwgfx_cotable.o vmwgfx_so.o vmwgfx_binding.o vmwgfx_msg.o
> \
> vmwgfx_simple_resource.o vmwgfx_va.o vmwgfx_blit.o \
> - vmwgfx_validation.o \
> + vmwgfx_validation.o vmwgfx_page_dirty.o \
> ttm_object.o ttm_lock.o
>
> obj-$(CONFIG_DRM_VMWGFX) := vmwgfx.o
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index c0829d50eecc..90ca866640fe 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> @@ -463,6 +463,7 @@ void vmw_bo_bo_free(struct ttm_buffer_object *bo)
> {
> struct vmw_buffer_object *vmw_bo = vmw_buffer_object(bo);
>
> + WARN_ON(vmw_bo->dirty);
> vmw_bo_unmap(vmw_bo);
> kfree(vmw_bo);
> }
> @@ -476,8 +477,10 @@ void vmw_bo_bo_free(struct ttm_buffer_object
> *bo)
> static void vmw_user_bo_destroy(struct ttm_buffer_object *bo)
> {
> struct vmw_user_buffer_object *vmw_user_bo =
> vmw_user_buffer_object(bo);
> + struct vmw_buffer_object *vbo = &vmw_user_bo->vbo;
>
> - vmw_bo_unmap(&vmw_user_bo->vbo);
> + WARN_ON(vbo->dirty);
Is it possible for user-space to exploit this WARN? If yes then you
might want to change the logic?
> + vmw_bo_unmap(vbo);
> ttm_prime_object_kfree(vmw_user_bo, prime);
> }
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 6165fe2c4504..74e94138877e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -857,6 +857,11 @@ static int vmw_driver_load(struct drm_device
> *dev, unsigned long chipset)
> DRM_ERROR("Failed initializing TTM buffer object
> driver.\n");
> goto out_no_bdev;
> }
> + dev_priv->vm_ops = *dev_priv->bdev.vm_ops;
> + dev_priv->vm_ops.fault = vmw_bo_vm_fault;
> + dev_priv->vm_ops.pfn_mkwrite = vmw_bo_vm_mkwrite;
> + dev_priv->vm_ops.page_mkwrite = vmw_bo_vm_mkwrite;
> + dev_priv->bdev.vm_ops = &dev_priv->vm_ops;
>
> /*
>* Enable VRAM, but initially don't use it until SVGA is
> enabled and
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index bd6919b90519..f05fce52fbb4 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -95,6 +95,7 @@ struct vmw_fpriv {
> * @dx_query_ctx: DX context if this buffer object is used as a DX
> query MOB
> * @map: Kmap object for semi-persistent mappings
> * @res_prios: Eviction priority counts for attached resources
> + * @dirty: structure for user-space dirty-tracking
> */
> struct vmw_buffer_object {
> struct ttm_buffer_object base;
> @@ -105,6 +106,7 @@ struct vmw_buffer_object {
> /* Protected by reservation */
> struct ttm_bo_kmap_obj map;
> u32 res_prios[TTM_MAX_BO_PRIORITY];
> + struct vmw_bo_dirty *dirty;
> };
>
> /**
> @@ -135,7 +137,8 @@ struct vm
Re: [GIT PULL] drm/vmwgfx: vmwgfx-next
It seems this got missed, If no one has any objection I will submit the patches via drm-mics route. Deepak On Tue, 2019-04-09 at 12:49 -0700, Deepak Singh Rawat wrote: > Hi Daniel/Dave, > > The vmwgfx-next changes for 5.2: > Resource dirtying improvement by Thomas, > user-space error logging improvement and > some other minor fixes. > > The following changes since commit > 14d2bd53a47a7e1cb3e03d00a6b952734cf90f3f: > > Merge tag 'drm-misc-next-2019-04-04' of > git://anongit.freedesktop.org/drm/drm-misc into drm-next (2019-04-05 > 11:38:02 +1000) > > are available in the Git repository at: > > https://gitlab.freedesktop.org/drawat/linux.git vmwgfx-next > > for you to fetch changes up to > c601b12fb634e2d0c2669706b38dba98a3c3a832: > > drm/vmwgfx: Remove set but not used variable 'fb_offset, fb_depth' > (2019-04-08 10:29:05 -0700) > > > Chengguang Xu (1): > drm/vmwgfx: remove redundant unlikely annotation > > Deepak Rawat (8): > drm/vmwgfx: Use preprocessor macro to get valid context node > drm/vmwgfx: Use preprocessor macro for cmd struct > drm/vmwgfx: Add a new define for vmwgfx user-space debugging > drm/vmwgfx: Print message when command verifier returns with > error > drm/vmwgfx: Clean up some debug messages in vmwgfx_execbuf.c > drm/vmwgfx: Use VMW_DEBUG_USER for device command buffer errors > drm/vmwgfx: Fix formatting and spaces in vmwgfx_execbuf.c > drm/vmwgfx: Use preprocessor macro for FIFO allocation > > Nathan Chancellor (1): > drm/vmwgfx: Zero initialize handle in vmw_execbuf_process > > Thomas Hellstrom (1): > drm/vmwgfx: Be more restrictive when dirtying resources > > YueHaibing (2): > drm/vmwgfx: Remove set but not used variable 'restart' > drm/vmwgfx: Remove set but not used variable 'fb_offset, > fb_depth' > > drivers/gpu/drm/vmwgfx/vmwgfx_binding.c | 98 + > drivers/gpu/drm/vmwgfx/vmwgfx_binding.h |2 + > drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c | 24 ++- > drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 59 ++ > drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c | 23 +-- > drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 29 ++- > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 1505 > +++ > --- > > drivers/gpu/drm/vmwgfx/vmwgfx_fb.c |4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_fifo.c| 27 +-- > drivers/gpu/drm/vmwgfx/vmwgfx_gmr.c |9 +- > drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 12 +- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 28 ++- > drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c |6 +- > drivers/gpu/drm/vmwgfx/vmwgfx_mob.c | 25 +-- > drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c |4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_resource.c| 28 ++- > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c| 23 +-- > drivers/gpu/drm/vmwgfx/vmwgfx_shader.c | 44 ++--- > drivers/gpu/drm/vmwgfx/vmwgfx_simple_resource.c | 12 +- > drivers/gpu/drm/vmwgfx/vmwgfx_so.c | 45 +++-- > drivers/gpu/drm/vmwgfx/vmwgfx_so.h |1 + > drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c| 47 ++--- > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 80 +++- > drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 61 -- > drivers/gpu/drm/vmwgfx/vmwgfx_validation.h |7 + > 25 files changed, 972 insertions(+), 1231 deletions(-) > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Expose "FB_DAMAGE_CLIPS" property to atomic aware user-space only
> > > > If you mean ssh on annarchy.freedesktop.org, I already have one > > with > > username - drawat > > You're added. Please check out the dim toturial and pls ask any > questions here or on irc or wherever - drm(-misc) maintainers are > happy to help out. > -Daniel > Hi I followed the instruction at https://drm.pages.freedesktop.org/maintainer-tools/getting-started.html and submitted my patch, I see that drm-tip has a lot of commits with myself as author(https://cgit.freedesktop.org/drm/drm-tip/log/). Is this expected? Or did I messed up something. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Expose "FB_DAMAGE_CLIPS" property to atomic aware user-space only
On Tue, 2019-04-16 at 20:15 +0200, Daniel Vetter wrote:
> On Tue, Apr 16, 2019 at 6:21 PM Deepak Singh Rawat > wrote:
> >
> > On Tue, 2019-04-16 at 09:42 +0200, Daniel Vetter wrote:
> > > On Mon, Apr 15, 2019 at 05:28:05PM +, Deepak Singh Rawat
> > > wrote:
> > > > Plane property "FB_DAMAGE_CLIPS" can only be used by atomic
> > > > aware
> > > > user-space, so no point exposing it otherwise.
> > > >
> > > > Signed-off-by: Deepak Rawat
> > > > Fixes: d3b21767821e ("drm: Add a new plane property to send
> > > > damage
> > > > during plane update")
> > >
> > > Makes sense, probably good if we add Cc: sta...@vger.kernel.org
> > > too
> >
> > Hi Daniel, thanks for the review. Sure.
> >
> > >
> > > Reviewed-by: Daniel Vetter
> > >
> > > btw, want drm-misc commit rights to push this? Assuming you'd
> > > then
> > > push
> > > vmware drm patches in general ofc. See
> >
> > Yes I think it would be a good idea if I can have drm-mics commit
> > rights. I think for occasional vmwgfx patches it would be ideal for
> > us
> > to use drm-mics on the other hand if had some major changes better
> > to
> > follow pull request way.
> >
> > How to apply for drm-mics commit rights?
>
>
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.freedesktop.org%2Fwiki%2FAccountRequests%2F&data=02%7C01%7Cdrawat%40vmware.com%7C8e0fc2a84e8d43a6ee7d08d6c2978af4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636910353461126728&sdata=T4RrZ7vdhhu3t0sn2jhU2axEVl7cr5x2JgOgJzVoEl8%3D&reserved=0
>
> you need an ssh account for drm-misc. Reply with the bug report here
> and I'll ack - drm-misc maintainers already acked.
> -Daniel
If you mean ssh on annarchy.freedesktop.org, I already have one with
username - drawat
>
> >
> > >
> > >
> >
> >
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdrm.pages.freedesktop.org%2Fmaintainer-tools%2Fgetting-started.html&data=02%7C01%7Cdrawat%40vmware.com%7C8e0fc2a84e8d43a6ee7d08d6c2978af4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636910353461126728&sdata=ZLOBkqM%2BFVKlutvu4u3kFyW8b%2FKp2fV5UFtfaW0wVZc%3D&reserved=0
> > >
> > > Cheers, Daniel
> > >
> > > > ---
> > > > drivers/gpu/drm/drm_mode_config.c | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_mode_config.c
> > > > b/drivers/gpu/drm/drm_mode_config.c
> > > > index 4a1c2023ccf0..1a346ae1599d 100644
> > > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > > @@ -297,8 +297,9 @@ static int
> > > > drm_mode_create_standard_properties(struct drm_device *dev)
> > > > return -ENOMEM;
> > > > dev->mode_config.prop_crtc_id = prop;
> > > >
> > > > - prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > > > "FB_DAMAGE_CLIPS",
> > > > - 0);
> > > > + prop = drm_property_create(dev,
> > > > + DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB,
> > > > + "FB_DAMAGE_CLIPS", 0);
> > > > if (!prop)
> > > > return -ENOMEM;
> > > > dev->mode_config.prop_fb_damage_clips = prop;
> > > > --
> > > > 2.21.0
> > > >
> > > > ___
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > >
> >
> >
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cdrawat%40vmware.com%7C8e0fc2a84e8d43a6ee7d08d6c2978af4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636910353461126728&sdata=6zQnD6klo9k3%2FvVP0mOUQeuwgoSZ3q7o0OChNNaoG%2BQ%3D&reserved=0
> > >
> > >
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Expose "FB_DAMAGE_CLIPS" property to atomic aware user-space only
On Tue, 2019-04-16 at 09:42 +0200, Daniel Vetter wrote:
> On Mon, Apr 15, 2019 at 05:28:05PM +0000, Deepak Singh Rawat wrote:
> > Plane property "FB_DAMAGE_CLIPS" can only be used by atomic aware
> > user-space, so no point exposing it otherwise.
> >
> > Signed-off-by: Deepak Rawat
> > Fixes: d3b21767821e ("drm: Add a new plane property to send damage
> > during plane update")
>
> Makes sense, probably good if we add Cc: sta...@vger.kernel.org too
Hi Daniel, thanks for the review. Sure.
>
> Reviewed-by: Daniel Vetter
>
> btw, want drm-misc commit rights to push this? Assuming you'd then
> push
> vmware drm patches in general ofc. See
Yes I think it would be a good idea if I can have drm-mics commit
rights. I think for occasional vmwgfx patches it would be ideal for us
to use drm-mics on the other hand if had some major changes better to
follow pull request way.
How to apply for drm-mics commit rights?
>
>
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdrm.pages.freedesktop.org%2Fmaintainer-tools%2Fgetting-started.html&data=02%7C01%7Cdrawat%40vmware.com%7C6c643d95324e44d8783108d6c23f0938%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636909973336030830&sdata=PMdonBtvx%2B2bMweQJTRu1b7IjmL0lSd2KKSIBV1ngTk%3D&reserved=0
>
> Cheers, Daniel
>
> > ---
> > drivers/gpu/drm/drm_mode_config.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mode_config.c
> > b/drivers/gpu/drm/drm_mode_config.c
> > index 4a1c2023ccf0..1a346ae1599d 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -297,8 +297,9 @@ static int
> > drm_mode_create_standard_properties(struct drm_device *dev)
> > return -ENOMEM;
> > dev->mode_config.prop_crtc_id = prop;
> >
> > - prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > "FB_DAMAGE_CLIPS",
> > - 0);
> > + prop = drm_property_create(dev,
> > + DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB,
> > + "FB_DAMAGE_CLIPS", 0);
> > if (!prop)
> > return -ENOMEM;
> > dev->mode_config.prop_fb_damage_clips = prop;
> > --
> > 2.21.0
> >
> > ___
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> >
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cdrawat%40vmware.com%7C6c643d95324e44d8783108d6c23f0938%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636909973336030830&sdata=QJCrLbtFUv7%2FiRAYOb2R%2Bab4w3l5CWae2K7t9Ya6dZw%3D&reserved=0
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: Expose "FB_DAMAGE_CLIPS" property to atomic aware user-space only
Plane property "FB_DAMAGE_CLIPS" can only be used by atomic aware
user-space, so no point exposing it otherwise.
Signed-off-by: Deepak Rawat
Fixes: d3b21767821e ("drm: Add a new plane property to send damage during plane
update")
---
drivers/gpu/drm/drm_mode_config.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_mode_config.c
b/drivers/gpu/drm/drm_mode_config.c
index 4a1c2023ccf0..1a346ae1599d 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -297,8 +297,9 @@ static int drm_mode_create_standard_properties(struct
drm_device *dev)
return -ENOMEM;
dev->mode_config.prop_crtc_id = prop;
- prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, "FB_DAMAGE_CLIPS",
- 0);
+ prop = drm_property_create(dev,
+ DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_BLOB,
+ "FB_DAMAGE_CLIPS", 0);
if (!prop)
return -ENOMEM;
dev->mode_config.prop_fb_damage_clips = prop;
--
2.21.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[GIT PULL] drm/vmwgfx: vmwgfx-next
Hi Daniel/Dave, The vmwgfx-next changes for 5.2: Resource dirtying improvement by Thomas, user-space error logging improvement and some other minor fixes. The following changes since commit 14d2bd53a47a7e1cb3e03d00a6b952734cf90f3f: Merge tag 'drm-misc-next-2019-04-04' of git://anongit.freedesktop.org/drm/drm-misc into drm-next (2019-04-05 11:38:02 +1000) are available in the Git repository at: https://gitlab.freedesktop.org/drawat/linux.git vmwgfx-next for you to fetch changes up to c601b12fb634e2d0c2669706b38dba98a3c3a832: drm/vmwgfx: Remove set but not used variable 'fb_offset, fb_depth' (2019-04-08 10:29:05 -0700) Chengguang Xu (1): drm/vmwgfx: remove redundant unlikely annotation Deepak Rawat (8): drm/vmwgfx: Use preprocessor macro to get valid context node drm/vmwgfx: Use preprocessor macro for cmd struct drm/vmwgfx: Add a new define for vmwgfx user-space debugging drm/vmwgfx: Print message when command verifier returns with error drm/vmwgfx: Clean up some debug messages in vmwgfx_execbuf.c drm/vmwgfx: Use VMW_DEBUG_USER for device command buffer errors drm/vmwgfx: Fix formatting and spaces in vmwgfx_execbuf.c drm/vmwgfx: Use preprocessor macro for FIFO allocation Nathan Chancellor (1): drm/vmwgfx: Zero initialize handle in vmw_execbuf_process Thomas Hellstrom (1): drm/vmwgfx: Be more restrictive when dirtying resources YueHaibing (2): drm/vmwgfx: Remove set but not used variable 'restart' drm/vmwgfx: Remove set but not used variable 'fb_offset, fb_depth' drivers/gpu/drm/vmwgfx/vmwgfx_binding.c | 98 + drivers/gpu/drm/vmwgfx/vmwgfx_binding.h |2 + drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c | 24 ++- drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 59 ++ drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c | 23 +-- drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 29 ++- drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 1505 +++--- drivers/gpu/drm/vmwgfx/vmwgfx_fb.c |4 +- drivers/gpu/drm/vmwgfx/vmwgfx_fifo.c| 27 +-- drivers/gpu/drm/vmwgfx/vmwgfx_gmr.c |9 +- drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 12 +- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 28 ++- drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c |6 +- drivers/gpu/drm/vmwgfx/vmwgfx_mob.c | 25 +-- drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c |4 +- drivers/gpu/drm/vmwgfx/vmwgfx_resource.c| 28 ++- drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c| 23 +-- drivers/gpu/drm/vmwgfx/vmwgfx_shader.c | 44 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_simple_resource.c | 12 +- drivers/gpu/drm/vmwgfx/vmwgfx_so.c | 45 +++-- drivers/gpu/drm/vmwgfx/vmwgfx_so.h |1 + drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c| 47 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 80 +++- drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 61 -- drivers/gpu/drm/vmwgfx/vmwgfx_validation.h |7 + 25 files changed, 972 insertions(+), 1231 deletions(-) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 10/11] drm/vmwgfx: Fix formatting and spaces in vmwgfx_execbuf.c
No functional change with this change, just fixing formatting and
spaces.
v2: Rebase.
Signed-off-by: Deepak Rawat
Reviewed-by: Thomas Hellstrom
---
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 687 +++-
1 file changed, 302 insertions(+), 385 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index e7c93f422a7e..0d703f431f1f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -54,7 +54,7 @@
__type body; \
} __var
-/*
+/**
* struct vmw_relocation - Buffer object relocation
*
* @head: List head for the command submission context's relocation list
@@ -78,9 +78,8 @@ struct vmw_relocation {
* command stream is replaced with the actual id after validation.
* @vmw_res_rel_nop: NOP relocation. The command is unconditionally replaced
* with a NOP.
- * @vmw_res_rel_cond_nop: Conditional NOP relocation. If the resource id
- * after validation is -1, the command is replaced with a NOP. Otherwise no
- * action.
+ * @vmw_res_rel_cond_nop: Conditional NOP relocation. If the resource id after
+ * validation is -1, the command is replaced with a NOP. Otherwise no action.
*/
enum vmw_resource_relocation_type {
vmw_res_rel_normal,
@@ -94,8 +93,8 @@ enum vmw_resource_relocation_type {
*
* @head: List head for the software context's relocation list.
* @res: Non-ref-counted pointer to the resource.
- * @offset: Offset of single byte entries into the command buffer where the
- * id that needs fixup is located.
+ * @offset: Offset of single byte entries into the command buffer where the id
+ * that needs fixup is located.
* @rel_type: Type of relocation.
*/
struct vmw_resource_relocation {
@@ -105,8 +104,9 @@ struct vmw_resource_relocation {
enum vmw_resource_relocation_type rel_type:3;
};
-/*
+/**
* struct vmw_ctx_validation_info - Extra validation metadata for contexts
+ *
* @head: List head of context list
* @ctx: The context resource
* @cur: The context's persistent binding state
@@ -161,9 +161,10 @@ static size_t vmw_ptr_diff(void *a, void *b)
/**
* vmw_execbuf_bindings_commit - Commit modified binding state
+ *
* @sw_context: The command submission context
- * @backoff: Whether this is part of the error path and binding state
- * changes should be ignored
+ * @backoff: Whether this is part of the error path and binding state changes
+ * should be ignored
*/
static void vmw_execbuf_bindings_commit(struct vmw_sw_context *sw_context,
bool backoff)
@@ -173,6 +174,7 @@ static void vmw_execbuf_bindings_commit(struct
vmw_sw_context *sw_context,
list_for_each_entry(entry, &sw_context->ctx_list, head) {
if (!backoff)
vmw_binding_state_commit(entry->cur, entry->staged);
+
if (entry->staged != sw_context->staged_bindings)
vmw_binding_state_free(entry->staged);
else
@@ -185,6 +187,7 @@ static void vmw_execbuf_bindings_commit(struct
vmw_sw_context *sw_context,
/**
* vmw_bind_dx_query_mob - Bind the DX query MOB if referenced
+ *
* @sw_context: The command submission context
*/
static void vmw_bind_dx_query_mob(struct vmw_sw_context *sw_context)
@@ -195,8 +198,8 @@ static void vmw_bind_dx_query_mob(struct vmw_sw_context
*sw_context)
}
/**
- * vmw_cmd_ctx_first_setup - Perform the setup needed when a context is
- * added to the validate list.
+ * vmw_cmd_ctx_first_setup - Perform the setup needed when a context is added
to
+ * the validate list.
*
* @dev_priv: Pointer to the device private:
* @sw_context: The command submission context
@@ -214,8 +217,7 @@ static int vmw_cmd_ctx_first_setup(struct vmw_private
*dev_priv,
goto out_err;
if (!sw_context->staged_bindings) {
- sw_context->staged_bindings =
- vmw_binding_state_alloc(dev_priv);
+ sw_context->staged_bindings = vmw_binding_state_alloc(dev_priv);
if (IS_ERR(sw_context->staged_bindings)) {
ret = PTR_ERR(sw_context->staged_bindings);
sw_context->staged_bindings = NULL;
@@ -240,19 +242,20 @@ static int vmw_cmd_ctx_first_setup(struct vmw_private
*dev_priv,
list_add_tail(&node->head, &sw_context->ctx_list);
return 0;
+
out_err:
return ret;
}
/**
- * vmw_execbuf_res_size - calculate extra size fore the resource validation
- * node
+ * vmw_execbuf_res_size - calculate extra size fore the resource validation
node
+ *
* @dev_priv: Pointer to the device private struct.
* @res_type: The resource type.
*
- * Guest-backed contexts and DX contexts require extra size to store
- * execbuf private information in the validation node. Typically the
- * binding manager associated data structures.
+
[PATCH 04/11] drm/vmwgfx: Use preprocessor macro to get valid context node
Several command verifier function check if context node is present or
not and if not present print an error and return. Use a preprocessor
macro to print the message.
v2: Name-space distinction for preprocessor macro
Signed-off-by: Deepak Rawat
Reviewed-by: Thomas Hellstrom
---
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 102 +++-
1 file changed, 45 insertions(+), 57 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index 4955a48a9d86..4f5445c53111 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -35,6 +35,19 @@
#define VMW_RES_HT_ORDER 12
+/*
+ * Helper macro to get dx_ctx_node if available otherwise print an error
+ * message. This is for use in command verifier function where if dx_ctx_node
+ * is not set then command is invalid.
+ */
+#define VMW_GET_CTX_NODE(__sw_context)\
+({\
+ __sw_context->dx_ctx_node ? __sw_context->dx_ctx_node : ({\
+ DRM_ERROR("SM context is not set at %s\n", __func__); \
+ __sw_context->dx_ctx_node;\
+ }); \
+})
+
/*
* struct vmw_relocation - Buffer object relocation
*
@@ -774,13 +787,11 @@ static int vmw_view_bindings_add(struct vmw_sw_context
*sw_context,
uint32 view_ids[], u32 num_views,
u32 first_slot)
{
- struct vmw_ctx_validation_info *ctx_node = sw_context->dx_ctx_node;
+ struct vmw_ctx_validation_info *ctx_node = VMW_GET_CTX_NODE(sw_context);
u32 i;
- if (!ctx_node) {
- DRM_ERROR("DX Context not set.\n");
+ if (!ctx_node)
return -EINVAL;
- }
for (i = 0; i < num_views; ++i) {
struct vmw_ctx_bindinfo_view binding;
@@ -1280,14 +1291,11 @@ static int vmw_cmd_dx_define_query(struct vmw_private
*dev_priv,
} *cmd;
intret;
- struct vmw_ctx_validation_info *ctx_node = sw_context->dx_ctx_node;
+ struct vmw_ctx_validation_info *ctx_node = VMW_GET_CTX_NODE(sw_context);
struct vmw_resource *cotable_res;
-
- if (ctx_node == NULL) {
- DRM_ERROR("DX Context not set for query.\n");
+ if (!ctx_node)
return -EINVAL;
- }
cmd = container_of(header, struct vmw_dx_define_query_cmd, header);
@@ -2270,14 +2278,12 @@ vmw_cmd_dx_set_single_constant_buffer(struct
vmw_private *dev_priv,
SVGA3dCmdDXSetSingleConstantBuffer body;
} *cmd;
struct vmw_resource *res = NULL;
- struct vmw_ctx_validation_info *ctx_node = sw_context->dx_ctx_node;
+ struct vmw_ctx_validation_info *ctx_node = VMW_GET_CTX_NODE(sw_context);
struct vmw_ctx_bindinfo_cb binding;
int ret;
- if (unlikely(ctx_node == NULL)) {
- DRM_ERROR("DX Context not set.\n");
+ if (!ctx_node)
return -EINVAL;
- }
cmd = container_of(header, typeof(*cmd), header);
ret = vmw_cmd_res_check(dev_priv, sw_context, vmw_res_surface,
@@ -2358,14 +2364,12 @@ static int vmw_cmd_dx_set_shader(struct vmw_private
*dev_priv,
SVGA3dCmdDXSetShader body;
} *cmd;
struct vmw_resource *res = NULL;
- struct vmw_ctx_validation_info *ctx_node = sw_context->dx_ctx_node;
+ struct vmw_ctx_validation_info *ctx_node = VMW_GET_CTX_NODE(sw_context);
struct vmw_ctx_bindinfo_shader binding;
int ret = 0;
- if (unlikely(ctx_node == NULL)) {
- DRM_ERROR("DX Context not set.\n");
+ if (!ctx_node)
return -EINVAL;
- }
cmd = container_of(header, typeof(*cmd), header);
@@ -2411,7 +2415,7 @@ static int vmw_cmd_dx_set_vertex_buffers(struct
vmw_private *dev_priv,
struct vmw_sw_context *sw_context,
SVGA3dCmdHeader *header)
{
- struct vmw_ctx_validation_info *ctx_node = sw_context->dx_ctx_node;
+ struct vmw_ctx_validation_info *ctx_node = VMW_GET_CTX_NODE(sw_context);
struct vmw_ctx_bindinfo_vb binding;
struct vmw_resource *res;
struct {
@@ -2421,10 +2425,8 @@ static int vmw_cmd_dx_set_vertex_buffers(struct
vmw_private *dev_priv,
} *cmd;
int i, ret, num;
- if (unlikely(ctx_node == NULL)) {
- DRM_ERROR("DX Context not set.\n");
+ if (!ctx_node)
return -EINVAL;
- }
cmd = container_of(header, typeof(*cmd), header);
num = (cmd->header.size - sizeof(cmd->body)) /
@@ -2469,7 +2471,7 @@ static int vmw_cmd_dx_set_index_buffer(struct vmw_private
*dev_priv,
[PATCH 07/11] drm/vmwgfx: Print message when command verifier returns with error
Whenever command verifier function returns with an error, print a debug
message using VMW_DEBUG_USER. This will make sure failing commands can
be easily tracked for debugging purpose.
Signed-off-by: Deepak Rawat
Reviewed-by: Thomas Hellstrom
---
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index 9362670af03c..9df334340146 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -3287,8 +3287,11 @@ static int vmw_cmd_check(struct vmw_private *dev_priv,
goto out_new;
ret = entry->func(dev_priv, sw_context, header);
- if (unlikely(ret != 0))
- goto out_invalid;
+ if (unlikely(ret != 0)) {
+ VMW_DEBUG_USER("SVGA3D command: %d failed with error %d\n",
+ cmd_id + SVGA_3D_CMD_BASE, ret);
+ return ret;
+ }
return 0;
out_invalid:
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 06/11] drm/vmwgfx: Add a new define for vmwgfx user-space debugging
Error messages or debugging message reported during user-space command
submission should not be printed to dmesg by default. So add a new
preprocessor define called VMW_DEBUG_USER which translates to
DRM_DEBUG_DRIVER.
v2: Use VMW_DEBUG_USER instead of using DRM_DEBUG_DRIVER directly.
Signed-off-by: Deepak Rawat
Reviewed-by: Thomas Hellstrom
---
drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 14 ++
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 140 +-
drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 12 +-
drivers/gpu/drm/vmwgfx/vmwgfx_shader.c| 7 +-
.../gpu/drm/vmwgfx/vmwgfx_simple_resource.c | 12 +-
drivers/gpu/drm/vmwgfx/vmwgfx_so.c| 9 +-
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 32 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c| 3 +-
9 files changed, 119 insertions(+), 112 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
index 694fabafaeee..39e96bb86329 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
@@ -751,7 +751,7 @@ static int vmw_context_define(struct drm_device *dev, void
*data,
int ret;
if (!dev_priv->has_dx && dx) {
- DRM_ERROR("DX contexts not supported by device.\n");
+ VMW_DEBUG_USER("DX contexts not supported by device.\n");
return -EINVAL;
}
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index abe975b7ea89..92367d4ebdf3 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -1313,6 +1313,20 @@ int vmw_host_get_guestinfo(const char *guest_info_param,
char *buffer, size_t *length);
int vmw_host_log(const char *log);
+/* VMW logging */
+
+/**
+ * VMW_DEBUG_USER - Debug output for user-space debugging.
+ *
+ * @fmt: printf() like format string.
+ *
+ * This macro is for logging user-space error and debugging messages for e.g.
+ * command buffer execution errors due to malformed commands, invalid context,
+ * etc.
+ */
+#define VMW_DEBUG_USER(fmt, ...) \
+ DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__)
+
/**
* Inline helper functions
*/
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index 2c92744817ec..9362670af03c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -43,7 +43,7 @@
#define VMW_GET_CTX_NODE(__sw_context)\
({\
__sw_context->dx_ctx_node ? __sw_context->dx_ctx_node : ({\
- DRM_ERROR("SM context is not set at %s\n", __func__); \
+ VMW_DEBUG_USER("SM context is not set at %s\n", __func__);\
__sw_context->dx_ctx_node;\
}); \
})
@@ -217,8 +217,7 @@ static int vmw_cmd_ctx_first_setup(struct vmw_private
*dev_priv,
sw_context->staged_bindings =
vmw_binding_state_alloc(dev_priv);
if (IS_ERR(sw_context->staged_bindings)) {
- DRM_ERROR("Failed to allocate context binding "
- "information.\n");
+ VMW_DEBUG_USER("Failed to alloc ctx binding info.\n");
ret = PTR_ERR(sw_context->staged_bindings);
sw_context->staged_bindings = NULL;
goto out_err;
@@ -228,8 +227,7 @@ static int vmw_cmd_ctx_first_setup(struct vmw_private
*dev_priv,
if (sw_context->staged_bindings_inuse) {
node->staged = vmw_binding_state_alloc(dev_priv);
if (IS_ERR(node->staged)) {
- DRM_ERROR("Failed to allocate context binding "
- "information.\n");
+ VMW_DEBUG_USER("Failed to alloc ctx binding info.\n");
ret = PTR_ERR(node->staged);
node->staged = NULL;
goto out_err;
@@ -424,7 +422,7 @@ vmw_view_id_val_add(struct vmw_sw_context *sw_context,
int ret;
if (!ctx_node) {
- DRM_ERROR("DX Context not set.\n");
+ VMW_DEBUG_USER("DX Context not set.\n");
return ERR_PTR(-EINVAL);
}
@@ -522,7 +520,7 @@ static int vmw_resource_relocation_add(struct
vmw_sw_context *sw_context,
rel = vmw_validation_mem_alloc(sw_context->ctx, sizeof(*rel));
if (unlikely(!rel)) {
- DRM_ERROR("Failed to allocate a resource relocation.\n");
+ VMW_DEBUG_USER("Failed to allocate a resource relocation.\n");
[PATCH 05/11] drm/vmwgfx: Use preprocessor macro for cmd struct
Use preprocessor macro for repetitive device command struct format.
v2: Name-space distinction for preprocessor macro.
v3: Struct name as macro parameter and rebase.
Signed-off-by: Deepak Rawat
Reviewed-by: Thomas Hellstrom
---
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 405
1 file changed, 125 insertions(+), 280 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index 4f5445c53111..2c92744817ec 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -48,6 +48,12 @@
}); \
})
+#define VMW_DECLARE_CMD_VAR(__var, __type)\
+ struct { \
+ SVGA3dCmdHeader header; \
+ __type body; \
+ } __var
+
/*
* struct vmw_relocation - Buffer object relocation
*
@@ -710,11 +716,7 @@ static int vmw_rebind_all_dx_query(struct vmw_resource
*ctx_res)
{
struct vmw_private *dev_priv = ctx_res->dev_priv;
struct vmw_buffer_object *dx_query_mob;
- struct {
- SVGA3dCmdHeader header;
- SVGA3dCmdDXBindAllQuery body;
- } *cmd;
-
+ VMW_DECLARE_CMD_VAR(*cmd, SVGA3dCmdDXBindAllQuery);
dx_query_mob = vmw_context_get_dx_query_mob(ctx_res);
@@ -831,15 +833,12 @@ static int vmw_cmd_cid_check(struct vmw_private *dev_priv,
struct vmw_sw_context *sw_context,
SVGA3dCmdHeader *header)
{
- struct vmw_cid_cmd {
- SVGA3dCmdHeader header;
- uint32_t cid;
- } *cmd;
+ VMW_DECLARE_CMD_VAR(*cmd, uint32_t) =
+ container_of(header, typeof(*cmd), header);
- cmd = container_of(header, struct vmw_cid_cmd, header);
return vmw_cmd_res_check(dev_priv, sw_context, vmw_res_context,
VMW_RES_DIRTY_SET, user_context_converter,
-&cmd->cid, NULL);
+&cmd->body, NULL);
}
/**
@@ -874,15 +873,12 @@ static int vmw_cmd_set_render_target_check(struct
vmw_private *dev_priv,
struct vmw_sw_context *sw_context,
SVGA3dCmdHeader *header)
{
- struct vmw_sid_cmd {
- SVGA3dCmdHeader header;
- SVGA3dCmdSetRenderTarget body;
- } *cmd;
+ VMW_DECLARE_CMD_VAR(*cmd, SVGA3dCmdSetRenderTarget);
struct vmw_resource *ctx;
struct vmw_resource *res;
int ret;
- cmd = container_of(header, struct vmw_sid_cmd, header);
+ cmd = container_of(header, typeof(*cmd), header);
if (cmd->body.type >= SVGA3D_RT_MAX) {
DRM_ERROR("Illegal render target type %u.\n",
@@ -924,13 +920,10 @@ static int vmw_cmd_surface_copy_check(struct vmw_private
*dev_priv,
struct vmw_sw_context *sw_context,
SVGA3dCmdHeader *header)
{
- struct vmw_sid_cmd {
- SVGA3dCmdHeader header;
- SVGA3dCmdSurfaceCopy body;
- } *cmd;
+ VMW_DECLARE_CMD_VAR(*cmd, SVGA3dCmdSurfaceCopy);
int ret;
- cmd = container_of(header, struct vmw_sid_cmd, header);
+ cmd = container_of(header, typeof(*cmd), header);
ret = vmw_cmd_res_check(dev_priv, sw_context, vmw_res_surface,
VMW_RES_DIRTY_NONE, user_surface_converter,
@@ -947,10 +940,7 @@ static int vmw_cmd_buffer_copy_check(struct vmw_private
*dev_priv,
struct vmw_sw_context *sw_context,
SVGA3dCmdHeader *header)
{
- struct {
- SVGA3dCmdHeader header;
- SVGA3dCmdDXBufferCopy body;
- } *cmd;
+ VMW_DECLARE_CMD_VAR(*cmd, SVGA3dCmdDXBufferCopy);
int ret;
cmd = container_of(header, typeof(*cmd), header);
@@ -969,10 +959,7 @@ static int vmw_cmd_pred_copy_check(struct vmw_private
*dev_priv,
struct vmw_sw_context *sw_context,
SVGA3dCmdHeader *header)
{
- struct {
- SVGA3dCmdHeader header;
- SVGA3dCmdDXPredCopyRegion body;
- } *cmd;
+ VMW_DECLARE_CMD_VAR(*cmd, SVGA3dCmdDXPredCopyRegion);
int ret;
cmd = container_of(header, typeof(*cmd), header);
@@ -991,13 +978,10 @@ static int vmw_cmd_stretch_blt_check(struct vmw_private
*dev_priv,
struct vmw_sw_context *sw_context,
SVGA3dCmdHeader *header)
{
- struct vmw_sid_cmd {
- SVGA3dCmdHeader hea
[PATCH 09/11] drm/vmwgfx: Use VMW_DEBUG_USER for device command buffer errors
DRM_ERROR overwhelms dmesgi so use VMW_DEBUG_USER instead. Any malformed
command should not really go to device so WARN_ONCE to spot this.
Signed-off-by: Deepak Rawat
Reviewed-by: Thomas Hellstrom
---
drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c
index ed15655eacd2..56979e412ca8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c
@@ -393,6 +393,7 @@ static void vmw_cmdbuf_ctx_process(struct vmw_cmdbuf_man
*man,
__vmw_cmdbuf_header_free(entry);
break;
case SVGA_CB_STATUS_COMMAND_ERROR:
+ WARN_ONCE(true, "Command buffer error.\n");
entry->cb_header->status = SVGA_CB_STATUS_NONE;
list_add_tail(&entry->list, &man->error);
schedule_work(&man->work);
@@ -533,19 +534,20 @@ static void vmw_cmdbuf_work_func(struct work_struct *work)
global_block = true;
if (!vmw_cmd_describe(header, &error_cmd_size, &cmd_name)) {
- DRM_ERROR("Unknown command causing device error.\n");
- DRM_ERROR("Command buffer offset is %lu\n",
- (unsigned long) cb_hdr->errorOffset);
+ VMW_DEBUG_USER("Unknown command causing device
error.\n");
+ VMW_DEBUG_USER("Command buffer offset is %lu\n",
+ (unsigned long) cb_hdr->errorOffset);
__vmw_cmdbuf_header_free(entry);
send_fence = true;
continue;
}
- DRM_ERROR("Command \"%s\" causing device error.\n", cmd_name);
- DRM_ERROR("Command buffer offset is %lu\n",
- (unsigned long) cb_hdr->errorOffset);
- DRM_ERROR("Command size is %lu\n",
- (unsigned long) error_cmd_size);
+ VMW_DEBUG_USER("Command \"%s\" causing device error.\n",
+ cmd_name);
+ VMW_DEBUG_USER("Command buffer offset is %lu\n",
+ (unsigned long) cb_hdr->errorOffset);
+ VMW_DEBUG_USER("Command size is %lu\n",
+ (unsigned long) error_cmd_size);
new_start_offset = cb_hdr->errorOffset + error_cmd_size;
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 08/11] drm/vmwgfx: Clean up some debug messages in vmwgfx_execbuf.c
Now that vmw_cmd_check prints debug message whenever a command verifier
fails, some of debug statements are unnecessary. Also rearranged some
debug print-out with this patch.
Signed-off-by: Deepak Rawat
Reviewed-by: Thomas Hellstrom
---
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 20
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index 9df334340146..e7c93f422a7e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -217,7 +217,6 @@ static int vmw_cmd_ctx_first_setup(struct vmw_private
*dev_priv,
sw_context->staged_bindings =
vmw_binding_state_alloc(dev_priv);
if (IS_ERR(sw_context->staged_bindings)) {
- VMW_DEBUG_USER("Failed to alloc ctx binding info.\n");
ret = PTR_ERR(sw_context->staged_bindings);
sw_context->staged_bindings = NULL;
goto out_err;
@@ -227,7 +226,6 @@ static int vmw_cmd_ctx_first_setup(struct vmw_private
*dev_priv,
if (sw_context->staged_bindings_inuse) {
node->staged = vmw_binding_state_alloc(dev_priv);
if (IS_ERR(node->staged)) {
- VMW_DEBUG_USER("Failed to alloc ctx binding info.\n");
ret = PTR_ERR(node->staged);
node->staged = NULL;
goto out_err;
@@ -327,8 +325,10 @@ static int vmw_execbuf_res_noref_val_add(struct
vmw_sw_context *sw_context,
if (priv_size && first_usage) {
ret = vmw_cmd_ctx_first_setup(dev_priv, sw_context, res,
ctx_info);
- if (ret)
+ if (ret) {
+ VMW_DEBUG_USER("Failed first usage context setup.\n");
return ret;
+ }
}
vmw_execbuf_rcache_update(rcache, res, ctx_info);
@@ -421,10 +421,8 @@ vmw_view_id_val_add(struct vmw_sw_context *sw_context,
struct vmw_resource *view;
int ret;
- if (!ctx_node) {
- VMW_DEBUG_USER("DX Context not set.\n");
+ if (!ctx_node)
return ERR_PTR(-EINVAL);
- }
view = vmw_view_lookup(sw_context->man, view_type, id);
if (IS_ERR(view))
@@ -723,10 +721,8 @@ static int vmw_rebind_all_dx_query(struct vmw_resource
*ctx_res)
cmd = vmw_fifo_reserve_dx(dev_priv, sizeof(*cmd), ctx_res->id);
- if (cmd == NULL) {
- VMW_DEBUG_USER("Failed to rebind queries.\n");
+ if (cmd == NULL)
return -ENOMEM;
- }
cmd->header.id = SVGA_3D_CMD_DX_BIND_ALL_QUERY;
cmd->header.size = sizeof(cmd->body);
@@ -761,8 +757,10 @@ static int vmw_rebind_contexts(struct vmw_sw_context
*sw_context)
}
ret = vmw_rebind_all_dx_query(val->ctx);
- if (ret != 0)
+ if (ret != 0) {
+ VMW_DEBUG_USER("Failed to rebind queries.\n");
return ret;
+ }
}
return 0;
@@ -2713,8 +2711,6 @@ static int vmw_cmd_dx_destroy_shader(struct vmw_private
*dev_priv,
ret = vmw_shader_remove(sw_context->man, cmd->body.shaderId, 0,
&sw_context->staged_cmd_res);
- if (ret)
- VMW_DEBUG_USER("Could not find shader to remove.\n");
return ret;
}
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 11/11] drm/vmwgfx: Use preprocessor macro for FIFO allocation
Whenever FIFO allocation fails an error message is printed to dmesg.
Since this is common operation a lot of similar messages are scattered
everywhere. Use preprocessor macro to remove this cluttering.
Signed-off-by: Deepak Rawat
Reviewed-by: Thomas Hellstrom
---
drivers/gpu/drm/vmwgfx/vmwgfx_binding.c | 72
drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 55 ++
drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c | 23 +++-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 13 -
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 12 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_fifo.c | 27 ++---
drivers/gpu/drm/vmwgfx/vmwgfx_gmr.c | 9 ++-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 23 +++-
drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 6 +-
drivers/gpu/drm/vmwgfx/vmwgfx_mob.c | 25 +++-
drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c | 4 +-
drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 7 +--
drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 20 ++-
drivers/gpu/drm/vmwgfx/vmwgfx_shader.c | 37
drivers/gpu/drm/vmwgfx/vmwgfx_so.c | 12 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 35
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 48 +---
17 files changed, 138 insertions(+), 290 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_binding.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_binding.c
index ef1469c4e91f..66e14e38d5e8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_binding.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_binding.c
@@ -499,12 +499,9 @@ static int vmw_binding_scrub_shader(struct
vmw_ctx_bindinfo *bi, bool rebind)
SVGA3dCmdSetShader body;
} *cmd;
- cmd = vmw_fifo_reserve(dev_priv, sizeof(*cmd));
- if (unlikely(cmd == NULL)) {
- DRM_ERROR("Failed reserving FIFO space for shader "
- "unbinding.\n");
+ cmd = VMW_FIFO_RESERVE(dev_priv, sizeof(*cmd));
+ if (unlikely(cmd == NULL))
return -ENOMEM;
- }
cmd->header.id = SVGA_3D_CMD_SET_SHADER;
cmd->header.size = sizeof(cmd->body);
@@ -534,12 +531,9 @@ static int vmw_binding_scrub_render_target(struct
vmw_ctx_bindinfo *bi,
SVGA3dCmdSetRenderTarget body;
} *cmd;
- cmd = vmw_fifo_reserve(dev_priv, sizeof(*cmd));
- if (unlikely(cmd == NULL)) {
- DRM_ERROR("Failed reserving FIFO space for render target "
- "unbinding.\n");
+ cmd = VMW_FIFO_RESERVE(dev_priv, sizeof(*cmd));
+ if (unlikely(cmd == NULL))
return -ENOMEM;
- }
cmd->header.id = SVGA_3D_CMD_SETRENDERTARGET;
cmd->header.size = sizeof(cmd->body);
@@ -576,12 +570,9 @@ static int vmw_binding_scrub_texture(struct
vmw_ctx_bindinfo *bi,
} body;
} *cmd;
- cmd = vmw_fifo_reserve(dev_priv, sizeof(*cmd));
- if (unlikely(cmd == NULL)) {
- DRM_ERROR("Failed reserving FIFO space for texture "
- "unbinding.\n");
+ cmd = VMW_FIFO_RESERVE(dev_priv, sizeof(*cmd));
+ if (unlikely(cmd == NULL))
return -ENOMEM;
- }
cmd->header.id = SVGA_3D_CMD_SETTEXTURESTATE;
cmd->header.size = sizeof(cmd->body);
@@ -610,12 +601,10 @@ static int vmw_binding_scrub_dx_shader(struct
vmw_ctx_bindinfo *bi, bool rebind)
SVGA3dCmdDXSetShader body;
} *cmd;
- cmd = vmw_fifo_reserve_dx(dev_priv, sizeof(*cmd), bi->ctx->id);
- if (unlikely(cmd == NULL)) {
- DRM_ERROR("Failed reserving FIFO space for DX shader "
- "unbinding.\n");
+ cmd = VMW_FIFO_RESERVE_DX(dev_priv, sizeof(*cmd), bi->ctx->id);
+ if (unlikely(cmd == NULL))
return -ENOMEM;
- }
+
cmd->header.id = SVGA_3D_CMD_DX_SET_SHADER;
cmd->header.size = sizeof(cmd->body);
cmd->body.type = binding->shader_slot + SVGA3D_SHADERTYPE_MIN;
@@ -641,12 +630,9 @@ static int vmw_binding_scrub_cb(struct vmw_ctx_bindinfo
*bi, bool rebind)
SVGA3dCmdDXSetSingleConstantBuffer body;
} *cmd;
- cmd = vmw_fifo_reserve_dx(dev_priv, sizeof(*cmd), bi->ctx->id);
- if (unlikely(cmd == NULL)) {
- DRM_ERROR("Failed reserving FIFO space for DX shader "
- "unbinding.\n");
+ cmd = VMW_FIFO_RESERVE_DX(dev_priv, sizeof(*cmd), bi->ctx->id);
+ if (unlikely(cmd == NULL))
return -ENOMEM;
- }
cmd->header.id = SVGA_3D_CMD_DX_SET_SINGLE_CONSTANT_BUFFER;
cmd->header.size = sizeof(cmd->body);
@@ -768,12 +754,9 @@ static int vmw_emit_set_sr(struct vmw_ctx_binding_state
*cbs,
view_id_size = cbs->bind_cmd_count*sizeof(uint32);
cmd_size = sizeof(*cmd) + view_id_size;
- cmd = vmw_fifo_reserve_dx(ctx->dev_priv, cmd_size, ctx->id);
- if (unlikely(cmd == NULL)) {
- DRM_ERROR("Failed reserving FIFO spac
[PATCH 02/11] drm/vmwgfx: Remove set but not used variable 'restart'
From: YueHaibing
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
Reviewed-by: Deepak Rawat
Signed-off-by: Deepak Rawat
Fixes: dc366364c4ef ("drm/vmwgfx: Fix multiple command buffer context use")
---
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)) {
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 03/11] drm/vmwgfx: remove redundant unlikely annotation
From: Chengguang Xu
unlikely has already included in IS_ERR(), so just
remove redundant unlikely annotation.
Signed-off-by: Chengguang Xu
Reviewed-by: Deepak Rawat
Signed-off-by: Deepak Rawat
---
drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
index 14bd760a62fd..694fabafaeee 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
@@ -210,7 +210,7 @@ static int vmw_gb_context_init(struct vmw_private *dev_priv,
for (i = 0; i < SVGA_COTABLE_DX10_MAX; ++i) {
uctx->cotables[i] = vmw_cotable_alloc(dev_priv,
&uctx->res, i);
- if (unlikely(IS_ERR(uctx->cotables[i]))) {
+ if (IS_ERR(uctx->cotables[i])) {
ret = PTR_ERR(uctx->cotables[i]);
goto out_cotables;
}
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index dc5698fbb654..4955a48a9d86 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -660,7 +660,7 @@ vmw_cmd_res_check(struct vmw_private *dev_priv,
res = vmw_user_resource_noref_lookup_handle
(dev_priv, sw_context->fp->tfile, *id_loc, converter);
- if (unlikely(IS_ERR(res))) {
+ if (IS_ERR(res)) {
DRM_ERROR("Could not find or use resource 0x%08x.\n",
(unsigned int) *id_loc);
return PTR_ERR(res);
@@ -3835,7 +3835,7 @@ static int vmw_execbuf_tie_context(struct vmw_private
*dev_priv,
res = vmw_user_resource_noref_lookup_handle
(dev_priv, sw_context->fp->tfile, handle,
user_context_converter);
- if (unlikely(IS_ERR(res))) {
+ if (IS_ERR(res)) {
DRM_ERROR("Could not find or user DX context 0x%08x.\n",
(unsigned) handle);
return PTR_ERR(res);
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 01/11] drm/vmwgfx: Be more restrictive when dirtying resources
From: Thomas Hellstrom
Currently we flag resources as dirty (GPU contents not yet read back to
the backing MOB) whenever they have been part of a command stream.
Obviously many resources can't be dirty and others can only be dirty when
written to by the GPU. That is when they are either bound to the context as
render-targets, depth-stencil, copy / clear destinations and
stream-output targets, or similarly when there are corresponding views into
them.
So mark resources dirty only in these special cases. Context- and cotable
resources are always marked dirty when referenced.
This is important for upcoming emulated coherent memory, since we can avoid
issuing automatic readbacks to non-dirty resources when the CPU tries to
access part of the backing MOB.
Testing: Unigine Heaven with max GPU memory set to 256MB resulting in
heavy resource thrashing.
---
v2: Addressed review comments by Deepak Rawat.
v3: Added some documentation
Signed-off-by: Thomas Hellstrom
Reviewed-by: Deepak Rawat
---
drivers/gpu/drm/vmwgfx/vmwgfx_binding.c| 26
drivers/gpu/drm/vmwgfx/vmwgfx_binding.h| 2 +
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h| 2 +
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c| 166 +
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c| 5 +-
drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 21 ++-
drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 3 +-
drivers/gpu/drm/vmwgfx/vmwgfx_so.c | 24 +++
drivers/gpu/drm/vmwgfx/vmwgfx_so.h | 1 +
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 12 +-
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 58 +--
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 7 +
12 files changed, 234 insertions(+), 93 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_binding.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_binding.c
index 0b9ee7fb45d6..ef1469c4e91f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_binding.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_binding.c
@@ -1269,6 +1269,32 @@ void vmw_binding_state_reset(struct
vmw_ctx_binding_state *cbs)
vmw_binding_drop(entry);
}
+/**
+ * vmw_binding_dirtying - Return whether a binding type is dirtying its
resource
+ * @binding_type: The binding type
+ *
+ * Each time a resource is put on the validation list as the result of a
+ * context binding referencing it, we need to determine whether that resource
+ * will be dirtied (written to by the GPU) as a result of the corresponding
+ * GPU operation. Currently rendertarget-, depth-stencil-, and
+ * stream-output-target bindings are capable of dirtying its resource.
+ *
+ * Return: Whether the binding type dirties the resource its binding points to.
+ */
+u32 vmw_binding_dirtying(enum vmw_ctx_binding_type binding_type)
+{
+ static u32 is_binding_dirtying[vmw_ctx_binding_max] = {
+ [vmw_ctx_binding_rt] = VMW_RES_DIRTY_SET,
+ [vmw_ctx_binding_dx_rt] = VMW_RES_DIRTY_SET,
+ [vmw_ctx_binding_ds] = VMW_RES_DIRTY_SET,
+ [vmw_ctx_binding_so] = VMW_RES_DIRTY_SET,
+ };
+
+ /* Review this function as new bindings are added. */
+ BUILD_BUG_ON(vmw_ctx_binding_max != 11);
+ return is_binding_dirtying[binding_type];
+}
+
/*
* This function is unused at run-time, and only used to hold various build
* asserts important for code optimization assumptions.
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_binding.h
b/drivers/gpu/drm/vmwgfx/vmwgfx_binding.h
index 6a2a9d69043b..f6ab79d23923 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_binding.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_binding.h
@@ -205,5 +205,7 @@ extern void vmw_binding_state_free(struct
vmw_ctx_binding_state *cbs);
extern struct list_head *
vmw_binding_state_list(struct vmw_ctx_binding_state *cbs);
extern void vmw_binding_state_reset(struct vmw_ctx_binding_state *cbs);
+extern u32 vmw_binding_dirtying(enum vmw_ctx_binding_type binding_type);
+
#endif
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 6302c12c2298..abe975b7ea89 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -699,6 +699,8 @@ extern int vmw_user_stream_lookup(struct vmw_private
*dev_priv,
uint32_t *inout_id,
struct vmw_resource **out);
extern void vmw_resource_unreserve(struct vmw_resource *res,
+ bool dirty_set,
+ bool dirty,
bool switch_backup,
struct vmw_buffer_object *new_backup,
unsigned long new_backup_offset);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index 88b8178d4687..dc5698fbb654 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -272,13 +272,15 @@ static void vmw_execbuf_rcache_update(struct
vmw_res_cache_en
Re: [PATCH -next] drm/vmwgfx: Remove set but not used variable 'fb_offset, fb_depth'
Thanks for doing this. Reviewed-by: Deepak Rawat On Sat, 2019-03-23 at 02:46 +, YueHaibing wrote: > Fixes gcc '-Wunused-but-set-variable' warning: > > drivers/gpu/drm/vmwgfx/vmwgfx_fb.c: In function 'vmw_fb_init': > drivers/gpu/drm/vmwgfx/vmwgfx_fb.c:645:29: warning: > variable 'fb_offset' set but not used [-Wunused-but-set-variable] > > drivers/gpu/drm/vmwgfx/vmwgfx_fb.c:645:19: warning: > variable 'fb_depth' set but not used [-Wunused-but-set-variable] > > They're not used any more, so can be removed. > > Signed-off-by: YueHaibing > --- > drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c > index 2a9112515f46..86efb91f5034 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c > @@ -642,12 +642,11 @@ int vmw_fb_init(struct vmw_private *vmw_priv) > struct vmw_fb_par *par; > struct fb_info *info; > unsigned fb_width, fb_height; > - unsigned fb_bpp, fb_depth, fb_offset, fb_pitch, fb_size; > + unsigned fb_bpp, fb_pitch, fb_size; > struct drm_display_mode *init_mode; > int ret; > > fb_bpp = 32; > - fb_depth = 24; > > /* XXX As shouldn't these be as well. */ > fb_width = min(vmw_priv->fb_max_width, (unsigned)2048); > @@ -655,7 +654,6 @@ int vmw_fb_init(struct vmw_private *vmw_priv) > > fb_pitch = fb_width * fb_bpp / 8; > fb_size = fb_pitch * fb_height; > - fb_offset = vmw_read(vmw_priv, SVGA_REG_FB_OFFSET); > > info = framebuffer_alloc(sizeof(*par), device); > if (!info) > > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH, RESEND] drm/vmwgfx: Don't double-free the mode stored in par->set_mode
Hi Thomas,
Thanks for doing this and somehow I missed the last patch, sorry about
that. Have some questions below otherwise the patch looks good to me.
Reviewed-by: Deepak Rawat
I will include your changes in vmwgfx-next and run tests.
On Mon, 2019-03-18 at 15:47 +0100, Thomas Zimmermann wrote:
> When calling vmw_fb_set_par(), the mode stored in par->set_mode gets
> free'd
> twice. The first free is in vmw_fb_kms_detach(), the second is near
> the
> end of vmw_fb_set_par() under the name of 'old_mode'. The mode-
> setting code
> only works correctly if the mode doesn't actually change.
You mean to say that without your patch vmwgfx fb driver fail to change
the mode?
> Removing 'old_mode'
> in favor of using par->set_mode directly fixes the problem.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 12 +++-
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> index b913a56f3426..2a9112515f46 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> @@ -564,11 +564,9 @@ static int vmw_fb_set_par(struct fb_info *info)
> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC)
> };
> - struct drm_display_mode *old_mode;
> struct drm_display_mode *mode;
> int ret;
>
> - old_mode = par->set_mode;
> mode = drm_mode_duplicate(vmw_priv->dev, &new_mode);
> if (!mode) {
> DRM_ERROR("Could not create new fb mode.\n");
> @@ -579,11 +577,7 @@ static int vmw_fb_set_par(struct fb_info *info)
> mode->vdisplay = var->yres;
> vmw_guess_mode_timing(mode);
>
> - if (old_mode && drm_mode_equal(old_mode, mode)) {
> - drm_mode_destroy(vmw_priv->dev, mode);
> - mode = old_mode;
> - old_mode = NULL;
I am having hard time understanding original intention for this piece
of code. Was there a restriction to send pointer to old mode if mode
were same and that restriction don't hold anymore. Sorry I am not
familiar with this code area.
> - } else if (!vmw_kms_validate_mode_vram(vmw_priv,
> + if (!vmw_kms_validate_mode_vram(vmw_priv,
> mode->hdisplay *
> DIV_ROUND_UP(var-
> >bits_per_pixel, 8),
> mode->vdisplay)) {
> @@ -620,8 +614,8 @@ static int vmw_fb_set_par(struct fb_info *info)
> schedule_delayed_work(&par->local_work, 0);
>
> out_unlock:
> - if (old_mode)
> - drm_mode_destroy(vmw_priv->dev, old_mode);
> + if (par->set_mode)
> + drm_mode_destroy(vmw_priv->dev, par->set_mode);
> par->set_mode = mode;
>
> mutex_unlock(&par->bo_mutex);
> --
> 2.20.1
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
>
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cdrawat%40vmware.com%7Cb1508247a3954fb0b08b08d6abb0bcd0%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636885172908359973&sdata=AN6UTrMzxcVK7MC6bX3OxNbcyq0j4HdKt0dk1yyHOHc%3D&reserved=0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vmwgfx: Zero initialize handle in vmw_execbuf_process
Hi Nathan, Thanks for doing this. I will include this in vmwgfx-next. Reviewed-by: Deepak Rawat On Thu, 2019-03-07 at 15:26 -0700, Nathan Chancellor wrote: > When building with -Wsometimes-uninitialized, Clang warns: > > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:3964:7: warning: variable > 'handle' is used uninitialized whenever '?:' condition is false > [-Wsometimes-uninitialized] > > It's not wrong; however, in practice, this is never an issue because > the value of handle isn't used when user_fence_rep is NULL because > vmw_execbuf_copy_fence_user returns immediately when that is the > case. > Just zero initialize this variable so that Clang no longer warns. > > Link: > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FClangBuiltLinux%2Flinux%2Fissues%2F397&data=02%7C01%7Cdrawat%40vmware.com%7C6b9c429d16d844a5740c08d6a34bedb4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636875943916412703&sdata=Yf7%2BzrbZS96yNJQewYQQ7YsHUDtJSvPaomyTJFZjsDk%3D&reserved=0 > Signed-off-by: Nathan Chancellor > --- > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > index 88b8178d4687..4ba06c2828a1 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > @@ -3829,7 +3829,7 @@ int vmw_execbuf_process(struct drm_file > *file_priv, > struct vmw_sw_context *sw_context = &dev_priv->ctx; > struct vmw_fence_obj *fence = NULL; > struct vmw_cmdbuf_header *header; > - uint32_t handle; > + uint32_t handle = 0; > int ret; > int32_t out_fence_fd = -1; > struct sync_file *sync_file = NULL; ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vmwgfx: remove redundant unlikely annotation
Hi Chengguang,
Thanks for doing this. Will include this for vmwgfx-next
Reviewed-by: Deepak Rawat
On Thu, 2019-02-21 at 10:09 +0800, Chengguang Xu wrote:
> unlikely has already included in IS_ERR(), so just
> remove redundant unlikely annotation.
>
> Signed-off-by: Chengguang Xu
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
> index 14bd760a62fd..694fabafaeee 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
> @@ -210,7 +210,7 @@ static int vmw_gb_context_init(struct vmw_private
> *dev_priv,
> for (i = 0; i < SVGA_COTABLE_DX10_MAX; ++i) {
> uctx->cotables[i] = vmw_cotable_alloc(dev_priv,
> &uctx-
> >res, i);
> - if (unlikely(IS_ERR(uctx->cotables[i]))) {
> + if (IS_ERR(uctx->cotables[i])) {
> ret = PTR_ERR(uctx->cotables[i]);
> goto out_cotables;
> }
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> index 88b8178d4687..1cde07cf3067 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> @@ -638,7 +638,7 @@ vmw_cmd_res_check(struct vmw_private *dev_priv,
>
> res = vmw_user_resource_noref_lookup_handle
> (dev_priv, sw_context->fp->tfile, *id_loc,
> converter);
> - if (unlikely(IS_ERR(res))) {
> + if (IS_ERR(res)) {
> DRM_ERROR("Could not find or use resource
> 0x%08x.\n",
> (unsigned int) *id_loc);
> return PTR_ERR(res);
> @@ -3799,7 +3799,7 @@ static int vmw_execbuf_tie_context(struct
> vmw_private *dev_priv,
> res = vmw_user_resource_noref_lookup_handle
> (dev_priv, sw_context->fp->tfile, handle,
>user_context_converter);
> - if (unlikely(IS_ERR(res))) {
> + if (IS_ERR(res)) {
> DRM_ERROR("Could not find or user DX context
> 0x%08x.\n",
> (unsigned) handle);
> return PTR_ERR(res);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
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)) {
>
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/vmwgfx: Fix an uninitialized fence handle value
Reviewed-by: Deepak Rawat
On Thu, 2019-01-31 at 10:52 +0100, Thomas Hellstrom wrote:
> if vmw_execbuf_fence_commands() fails, The handle value will be
> uninitialized and a bogus fence handle might be copied to user-space.
>
> Fixes: 2724b2d54cda: ("drm/vmwgfx: Use new validation interface for
> the modesetting code v2")
> Reported-by: Dan Carpenter
> Signed-off-by: Thomas Hellstrom
> Reviewed-by: Brian Paul #v1
> Reviewed-by: Sinclair Yeh #v1
> ---
> v2: Also initialize the ret local variable, to silence compilatior
> warnings.
> Call vmw_execbuf_copy_fence_user regardless of the value of ret, to
> propagate
> the correct error code to user-space.
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index b351fb5214d3..5e257a600cea 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -2554,8 +2554,8 @@ void vmw_kms_helper_validation_finish(struct
> vmw_private *dev_priv,
> user_fence_rep)
> {
> struct vmw_fence_obj *fence = NULL;
> - uint32_t handle;
> - int ret;
> + uint32_t handle = 0;
> + int ret = 0;
>
> if (file_priv || user_fence_rep || vmw_validation_has_bos(ctx)
> ||
> out_fence)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vmwgfx: Return error code from vmw_execbuf_copy_fence_user
Reviewed-by: Deepak Rawat
On Thu, 2019-01-31 at 11:07 +0100, Thomas Hellstrom wrote:
> The function was unconditionally returning 0, and a caller would have
> to
> rely on the returned fence pointer being NULL to detect errors.
> However,
> the function vmw_execbuf_copy_fence_user() would expect a non-zero
> error
> code in that case and would BUG otherwise.
>
> So make sure we return a proper non-zero error code if the fence
> pointer
> returned is NULL.
>
> Fixes: ae2a104058e2: ("vmwgfx: Implement fence objects")
> Signed-off-by: Thomas Hellstrom
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> index f2d13a72c05d..88b8178d4687 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> @@ -3570,7 +3570,7 @@ int vmw_execbuf_fence_commands(struct drm_file
> *file_priv,
> *p_fence = NULL;
> }
>
> - return 0;
> + return ret;
> }
>
> /**
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/vmwgfx: Fix setting of dma masks
For the series
Reviewed-by: Deepak Rawat
On Tue, 2019-01-29 at 14:35 +0100, Thomas Hellstrom wrote:
> Previously we set only the dma mask and not the coherent mask. Fix
> that.
> Also, for clarity, make sure both are initially set to 64 bits.
>
> Fixes: 0d00c488f3de: ("drm/vmwgfx: Fix the driver for large dma
> addresses")
> Signed-off-by: Thomas Hellstrom
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 3e2bcff34032..ae9df4432bfc 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -600,13 +600,16 @@ static int vmw_dma_select_mode(struct
> vmw_private *dev_priv)
> static int vmw_dma_masks(struct vmw_private *dev_priv)
> {
> struct drm_device *dev = dev_priv->dev;
> + int ret = 0;
>
> - if (intel_iommu_enabled &&
> + ret = dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(64));
> + if (dev_priv->map_mode != vmw_dma_phys &&
> (sizeof(unsigned long) == 4 || vmw_restrict_dma_mask)) {
> DRM_INFO("Restricting DMA addresses to 44 bits.\n");
> - return dma_set_mask(dev->dev, DMA_BIT_MASK(44));
> + return dma_set_mask_and_coherent(dev->dev,
> DMA_BIT_MASK(44));
> }
> - return 0;
> +
> + return ret;
> }
>
> static int vmw_driver_load(struct drm_device *dev, unsigned long
> chipset)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/4] drm/tinydrm: Use damage helper for dirtyfb
On Wed, 2019-01-09 at 18:49 +0100, Noralf Trønnes wrote: > Hi, > > I was really pleased to see that the damage helper had landed. Now I > can > do framebuffer flushing solely through the display pipe functions. > This > prepares the ground for the removal of struct tinydrm_device in my > next > series. Hi Noralf, Not an expert of tinydrm but I followed the code and everything looked alright to me especially the usage of damage iterator. Yes the code to clear damage on plane state destroy was missing earlier and can be removed from the new helper you have. It is better to have damage in plane state as something else might need it. Thanks, Deepak > > Note: > The damage helper isn't in drm-misc-next yet, it will show up when > -rc1 > arrives there. > > Noralf. > > Noralf Trønnes (4): > drm/gem-fb-helper: Add drm_gem_fb_create_with_dirty() > drm/damage-helper: Add drm_atomic_helper_damage_merged() > drm/tinydrm: Use struct drm_rect > drm/tinydrm: Use damage helper for dirtyfb > > drivers/gpu/drm/drm_damage_helper.c | 43 ++ > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 47 +- > drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 21 +-- > .../gpu/drm/tinydrm/core/tinydrm-helpers.c| 100 + > drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 30 > drivers/gpu/drm/tinydrm/hx8357d.c | 2 +- > drivers/gpu/drm/tinydrm/ili9225.c | 138 +++- > -- > drivers/gpu/drm/tinydrm/ili9341.c | 2 +- > drivers/gpu/drm/tinydrm/mi0283qt.c| 2 +- > drivers/gpu/drm/tinydrm/mipi-dbi.c| 87 +++ > drivers/gpu/drm/tinydrm/repaper.c | 42 +++--- > drivers/gpu/drm/tinydrm/st7586.c | 73 - > drivers/gpu/drm/tinydrm/st7735r.c | 2 +- > include/drm/drm_damage_helper.h | 3 + > include/drm/drm_gem_framebuffer_helper.h | 3 + > include/drm/tinydrm/mipi-dbi.h| 5 +- > include/drm/tinydrm/tinydrm-helpers.h | 20 +-- > include/drm/tinydrm/tinydrm.h | 26 > 18 files changed, 281 insertions(+), 365 deletions(-) > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Put damage blob when destroy plane state
On Tue, 2018-12-25 at 07:01 -0800, Thomas Hellstrom wrote: > On Mon, 2018-12-24 at 11:49 +0100, Daniel Vetter wrote: > > On Fri, Dec 21, 2018 at 8:56 PM Thomas Hellstrom < > > thellst...@vmware.com> wrote: > > > Reviewed-by: Thomas Hellstrom > > > > > > Daniel, Dave, could you help try to get this patch in -next > > > before > > > the > > > merge window. Otherwise people will start to experience random > > > kernel > > > crashes. I figure we don't want to wait until first -fixes pull > > > with > > > this. > > > > Afaiui this will only blow up with new userspace on new kernels, > > that > > doesn't feel like rushing an updated -next out the door material to > > me. > > Hmm, this was discovered when the drm-read igt test hit an OOPS, > I guess when damage_clips is unintentionally Non-Null. > > > > More concerning is why this fell through the cracks: > > I agree. Sinc I'm on vacation a very quick answer. Will investigate > more thogroughly when we get back. > > > - We have an igt, do those testcases not hit this bug? > > The drm-read igt intermittently hit the bug. > > > > - Were the tests not run before you've sent out the pull? > > No, and I guess that't the main culprit. The igt doesn't work well > with > vmwgfx, mostly because many tests are assuming gem functionality, > there > fore we doesn't run them regularly, but mostly a pre-commit testsuite > that focuses more on rendering correctness than modesetting > functionality. That said, Deepak has over time put quite an effort > into > making at least part of that test suite run well with vmwgfx, but I > didn't consider it for this pull request. Will make sure we do in the > future for core drm changes. > > > - The merged version doesn't seem to match any of the versions I've > > found on dri-devel, I guess should have been resend when there was > > conflicts? > > Yes, I think there was at least one trivial merge conflict and a > couple > of minor checkpatch.pl fixups in the pull request. Typically for > vmwgfx > we don't resend the resulting patches if there are minor changes like > that and they are reviewed internally. Need to get back to you about > exactly where in the process we failed here. I did ran igt after rebasing with upstream drm-next but only kms tests and didn't saw any problem. We have glretrace running continuously with rebooting VM's every time it runs and it too didn't failed so didn't suspected anything wrong with what I sent. Unfortunately we don't run igt continuously so I guess that is what we should work on. I am yet to investigate why only drm_read failed. Although from stack trace I can see it is failing when set_config is called from vmwgfx fb_dev driver. > > Thanks, > Thomas > > > > > - Some other crack in the matrix? > > > > > Thanks, > > > Thomas > > > > > > > > > On Fri, 2018-12-21 at 11:35 -0800, Deepak Rawat wrote: > > > > Somehow the code to put the damage blob on destroy plane state > > > > and > > > > set > > > > the blob to NULL when duplicate plane state was not merged. May > > > > be > > > > because the files are refactored since the patch was written. > > > > With > > > > this > > > > fix add those. > > > > > > > > Cc: Daniel Vetter > > > > Signed-off-by: Deepak Rawat > > > > Needs a Fixes: tag referencing the broken commit, pls remember to > > add > > these anytime you fix an issue with a commit. I'll add that and > > push > > it to drm-misc-next-fixes. Thanks Daniel for this. Will make sure in future. > > > > Thanks, Daniel > > > > > > --- > > > > drivers/gpu/drm/drm_atomic_state_helper.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c > > > > b/drivers/gpu/drm/drm_atomic_state_helper.c > > > > index 3ba996069d69..709355c6bac6 100644 > > > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > > > @@ -241,6 +241,7 @@ void > > > > __drm_atomic_helper_plane_duplicate_state(struct drm_plane > > > > *plane, > > > > > > > > state->fence = NULL; > > > > state->commit = NULL; > > > > + state->fb_damage_clips = NULL; > > > > } > > > > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); > > > > > > > > @@ -285,6 +286,8 @@ void > > > > __drm_atomic_helper_plane_destroy_state(struct drm_plane_state > > > > *state) > > > > > > > > if (state->commit) > > > > drm_crtc_commit_put(state->commit); > > > > + > > > > + drm_property_blob_put(state->fb_damage_clips); > > > > } > > > > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); > > > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - > > https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch&data=02%7C01%7Cthellstrom%40vmware.com%7Ca62b8426087f410a689b08d6698d9110%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636812454080233380&sdata=6CZm4TU0A7wLpd1sXOUA3URmMQJqndy1WHxhtjnU8P8%3D&re
Re: [PATCH -fixes] drm/vmwgfx: Protect from excessive execbuf kernel memory allocations v2
Reviewed-by: Deepak Rawat
On Wed, 2018-12-12 at 10:38 -0800, Thomas Hellstrom wrote:
> Hi, Deepak,
>
> On Wed, 2018-12-12 at 10:00 -0800, Deepak Singh Rawat wrote:
> > On Wed, 2018-12-12 at 15:35 +0100, Thomas Hellstrom wrote:
> > > With the new validation code, a malicious user-space app could
> > > potentially submit command streams with enough buffer-object and
> > > resource
> > > references in them to have the resulting allocated validion nodes
> > > and
> > > relocations make the kernel run out of GFP_KERNEL memory.
> > >
> > > Protect from this by having the validation code reserve TTM
> > > graphics
> > > memory when allocating.
> > >
> > > Signed-off-by: Thomas Hellstrom
> > > ---
> > > v2: Removed leftover debug printouts
> > > ---
> > > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c| 4 ++-
> > > drivers/gpu/drm/vmwgfx/vmwgfx_drv.h| 5 +++
> > > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c| 2 ++
> > > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 36
> > > +
> > > drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 18 ++-
> > > drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 37
> > > ++
> > > 6 files changed, 100 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > > index 61a84b958d67..d7a2dfb8ee9b 100644
> > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > > @@ -49,6 +49,8 @@
> > >
> > > #define VMWGFX_REPO "In Tree"
> > >
> > > +#define VMWGFX_VALIDATION_MEM_GRAN (16*PAGE_SIZE)
> > > +
> > >
> > > /**
> > > * Fully encoded drm commands. Might move to vmw_drm.h
> > > @@ -918,7 +920,7 @@ static int vmw_driver_load(struct drm_device
> > > *dev, unsigned long chipset)
> > > spin_unlock(&dev_priv->cap_lock);
> > > }
> > >
> > > -
> > > + vmw_validation_mem_init_ttm(dev_priv,
> > > VMWGFX_VALIDATION_MEM_GRAN);
> > > ret = vmw_kms_init(dev_priv);
> > > if (unlikely(ret != 0))
> > > goto out_no_kms;
> > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > > index 59f614225bcd..aca974b14b55 100644
> > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> > > @@ -606,6 +606,9 @@ struct vmw_private {
> > >
> > > struct vmw_cmdbuf_man *cman;
> > > DECLARE_BITMAP(irqthread_pending, VMW_IRQTHREAD_MAX);
> > > +
> > > + /* Validation memory reservation */
> > > + struct vmw_validation_mem vvm;
> > > };
> > >
> > > static inline struct vmw_surface *vmw_res_to_srf(struct
> > > vmw_resource
> > > *res)
> > > @@ -846,6 +849,8 @@ extern int vmw_ttm_global_init(struct
> > > vmw_private
> > > *dev_priv);
> > > extern void vmw_ttm_global_release(struct vmw_private
> > > *dev_priv);
> > > extern int vmw_mmap(struct file *filp, struct vm_area_struct
> > > *vma);
> > >
> > > +extern void vmw_validation_mem_init_ttm(struct vmw_private
> > > *dev_priv,
> > > + size_t gran);
> > > /**
> > > * TTM buffer object driver - vmwgfx_ttm_buffer.c
> > > */
> > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > > b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > > index 260650bb5560..f2d13a72c05d 100644
> > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> > > @@ -3835,6 +3835,8 @@ int vmw_execbuf_process(struct drm_file
> > > *file_priv,
> > > struct sync_file *sync_file = NULL;
> > > DECLARE_VAL_CONTEXT(val_ctx, &sw_context->res_ht, 1);
> > >
> > > + vmw_validation_set_val_mem(&val_ctx, &dev_priv->vvm);
> > > +
> > > if (flags & DRM_VMW_EXECBUF_FLAG_EXPORT_FENCE_FD) {
> > > out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> > > if (out_fence_fd < 0) {
> > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> > > b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> > > index 7b1e5a5cbd2c..f88247046721 100644
> > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_gl
Re: [PATCH -fixes] drm/vmwgfx: Protect from excessive execbuf kernel memory allocations v2
On Wed, 2018-12-12 at 15:35 +0100, Thomas Hellstrom wrote:
> With the new validation code, a malicious user-space app could
> potentially submit command streams with enough buffer-object and
> resource
> references in them to have the resulting allocated validion nodes and
> relocations make the kernel run out of GFP_KERNEL memory.
>
> Protect from this by having the validation code reserve TTM graphics
> memory when allocating.
>
> Signed-off-by: Thomas Hellstrom
> ---
> v2: Removed leftover debug printouts
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c| 4 ++-
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h| 5 +++
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c| 2 ++
> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 36
> +
> drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 18 ++-
> drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 37
> ++
> 6 files changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 61a84b958d67..d7a2dfb8ee9b 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -49,6 +49,8 @@
>
> #define VMWGFX_REPO "In Tree"
>
> +#define VMWGFX_VALIDATION_MEM_GRAN (16*PAGE_SIZE)
> +
>
> /**
> * Fully encoded drm commands. Might move to vmw_drm.h
> @@ -918,7 +920,7 @@ static int vmw_driver_load(struct drm_device
> *dev, unsigned long chipset)
> spin_unlock(&dev_priv->cap_lock);
> }
>
> -
> + vmw_validation_mem_init_ttm(dev_priv,
> VMWGFX_VALIDATION_MEM_GRAN);
> ret = vmw_kms_init(dev_priv);
> if (unlikely(ret != 0))
> goto out_no_kms;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 59f614225bcd..aca974b14b55 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -606,6 +606,9 @@ struct vmw_private {
>
> struct vmw_cmdbuf_man *cman;
> DECLARE_BITMAP(irqthread_pending, VMW_IRQTHREAD_MAX);
> +
> + /* Validation memory reservation */
> + struct vmw_validation_mem vvm;
> };
>
> static inline struct vmw_surface *vmw_res_to_srf(struct vmw_resource
> *res)
> @@ -846,6 +849,8 @@ extern int vmw_ttm_global_init(struct vmw_private
> *dev_priv);
> extern void vmw_ttm_global_release(struct vmw_private *dev_priv);
> extern int vmw_mmap(struct file *filp, struct vm_area_struct *vma);
>
> +extern void vmw_validation_mem_init_ttm(struct vmw_private
> *dev_priv,
> + size_t gran);
> /**
> * TTM buffer object driver - vmwgfx_ttm_buffer.c
> */
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> index 260650bb5560..f2d13a72c05d 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> @@ -3835,6 +3835,8 @@ int vmw_execbuf_process(struct drm_file
> *file_priv,
> struct sync_file *sync_file = NULL;
> DECLARE_VAL_CONTEXT(val_ctx, &sw_context->res_ht, 1);
>
> + vmw_validation_set_val_mem(&val_ctx, &dev_priv->vvm);
> +
> if (flags & DRM_VMW_EXECBUF_FLAG_EXPORT_FENCE_FD) {
> out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> if (out_fence_fd < 0) {
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> index 7b1e5a5cbd2c..f88247046721 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> @@ -96,3 +96,39 @@ void vmw_ttm_global_release(struct vmw_private
> *dev_priv)
> drm_global_item_unref(&dev_priv->bo_global_ref.ref);
> drm_global_item_unref(&dev_priv->mem_global_ref);
> }
> +
> +/* struct vmw_validation_mem callback */
> +static int vmw_vmt_reserve(struct vmw_validation_mem *m, size_t
> size)
> +{
> + static struct ttm_operation_ctx ctx = {.interruptible = false,
> +.no_wait_gpu = false};
> + struct vmw_private *dev_priv = container_of(m, struct
> vmw_private, vvm);
> +
> + return ttm_mem_global_alloc(vmw_mem_glob(dev_priv), size,
> &ctx);
> +}
> +
> +/* struct vmw_validation_mem callback */
> +static void vmw_vmt_unreserve(struct vmw_validation_mem *m, size_t
> size)
> +{
> + struct vmw_private *dev_priv = container_of(m, struct
> vmw_private, vvm);
> +
> + return ttm_mem_global_free(vmw_mem_glob(dev_priv), size);
> +}
> +
> +/**
> + * vmw_validation_mem_init_ttm - Interface the validation memory
> tracker
> + * to ttm.
> + * @dev_priv: Pointer to struct vmw_private. The reason we choose a
> vmw private
> + * rather than a struct vmw_validation_mem is to make sure
> assumption in the
> + * callbacks that struct vmw_private derives from struct
> vmw_validation_mem
> + * holds true.
> + * @gran: The recommended allocation granularity
> + */
> +void vmw_validation_mem_init_ttm(struct vmw_private *dev_priv
RE: [PATCH v2 2/2] drm/selftest: Add drm damage helper selftest
> Hi Deepak, > > Something to consider, with this approach we kind of break the > following behaviour: > "Only tests enabled as module parameters are run, if no test is > explicitly enabled then all of them are run" > > What I think we should do is have just one header where we put the > selftest(check_plane_state, igt_check_plane_statea) > ... > selftest(damage_iter_no_damage, igt_damage_iter_no_damage) > ... > call it drm_modeset_selftests.h > > And separate just the implementations in different source files. > > Then we could call run_selftests just once from test_drm_modeset_init. > > Does this makes sense to you? > Hi Alexandru, Thanks for the review. Yes this does make sense to me. I will change this one to have run_selftests on one single header, single patch 01 is already merged to drm-mics. > > > > Cc: Daniel Vetter > > > > Cc: alexandru-cosmin.gheor...@arm.com > > > > Signed-off-by: Deepak Rawat > > > > Reviewed-by: Daniel Vetter > > > > Reviewed-by: Thomas Hellstrom > > > > > > I guess this needs your entire damage series to make sense, right? > > > > Ah yes sorry. Although this patch is same as what I sent earlier > > except linking with test-drm_modeset.ko > > > > > > > > Another question: Does anyone from vmwgfx want drm-misc commit rights > for > > > pushing stuff like this? > > > > Hi Daniel, Thanks for the suggestion. I am not sure about eligibility > > criteria for > > commit rights but I think I will wait until drm-mics is moved to gitlab and > > I have > > more experience working with open-source. Beside me Thomas works on > > vmwgfx, I am not sure if he already have commit rights. > > > > > -Daniel > > > > --- > > > > drivers/gpu/drm/selftests/Makefile| 3 +- > > > > .../selftests/drm_damage_helper_selftests.h | 22 + > > > > .../drm/selftests/test-drm_damage_helper.c| 828 > ++ > > > > .../drm/selftests/test-drm_modeset_common.c | 10 +- > > > > .../drm/selftests/test-drm_modeset_common.h | 1 + > > > > 5 files changed, 862 insertions(+), 2 deletions(-) > > > > create mode 100644 > > > drivers/gpu/drm/selftests/drm_damage_helper_selftests.h > > > > create mode 100644 drivers/gpu/drm/selftests/test- > drm_damage_helper.c > > > > > > > > diff --git a/drivers/gpu/drm/selftests/Makefile > > > b/drivers/gpu/drm/selftests/Makefile > > > > index 7e6581921da0..c6e63ed12f02 100644 > > > > --- a/drivers/gpu/drm/selftests/Makefile > > > > +++ b/drivers/gpu/drm/selftests/Makefile > > > > @@ -1,3 +1,4 @@ > > > > -test-drm_modeset-y := test-drm_modeset_common.o test- > > > drm_plane_helper.o > > > > +test-drm_modeset-y := test-drm_modeset_common.o test- > > > drm_plane_helper.o \ > > > > + test-drm_damage_helper.o > > > > > > > > obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test- > > > drm_modeset.o > > > > diff --git a/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h > > > b/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h > > > > new file mode 100644 > > > > index ..3a1cbe05bef0 > > > > --- /dev/null > > > > +++ b/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h > > > > @@ -0,0 +1,22 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > +selftest(damage_iter_no_damage, igt_damage_iter_no_damage) > > > > +selftest(damage_iter_no_damage_fractional_src, > > > igt_damage_iter_no_damage_fractional_src) > > > > +selftest(damage_iter_no_damage_src_moved, > > > igt_damage_iter_no_damage_src_moved) > > > > +selftest(damage_iter_no_damage_fractional_src_moved, > > > igt_damage_iter_no_damage_fractional_src_moved) > > > > +selftest(damage_iter_no_damage_not_visible, > > > igt_damage_iter_no_damage_not_visible) > > > > +selftest(damage_iter_no_damage_no_crtc, > > > igt_damage_iter_no_damage_no_crtc) > > > > +selftest(damage_iter_no_damage_no_fb, > > > igt_damage_iter_no_damage_no_fb) > > > > +selftest(damage_iter_simple_damage, igt_damage_iter_simple_damage) > > > > +selftest(damage_iter_single_damage, igt_damage_iter_single_damage) > > > > +selftest(damage_iter_single_damage_intersect_src, > > > igt_damage_iter_single_damage_intersect_src) > > > > +selftest(damage_iter_single_damage_outside_src, > > > igt_damage_iter_single_damage_outside_src) > > > > +selftest(damage_iter_single_damage_fractional_src, > > > igt_damage_iter_single_damage_fractional_src) > > > > +selftest(damage_iter_single_damage_intersect_fractional_src, > > > igt_damage_iter_single_damage_intersect_fractional_src) > > > > +selftest(damage_iter_single_damage_outside_fractional_src, > > > igt_damage_iter_single_damage_outside_fractional_src) > > > > +selftest(damage_iter_single_damage_src_moved, > > > igt_damage_iter_single_damage_src_moved) > > > > +selftest(damage_iter_single_damage_fractional_src_moved, > > > igt_damage_iter_single_damage_fractional_src_moved) > > > > +selftest(damage_iter_damage, igt_damage_iter_damage) > > > > +selftest(damage_iter_damage_one_intersect, > > > igt_damage_iter_damage_one_inters
RE: [PATCH v2 2/2] drm/selftest: Add drm damage helper selftest
> On Tue, Oct 16, 2018 at 01:46:09PM -0700, Deepak Rawat wrote:
> > Selftest for drm damage helper iterator functions.
> >
> > Cc: Daniel Vetter
> > Cc: alexandru-cosmin.gheor...@arm.com
> > Signed-off-by: Deepak Rawat
> > Reviewed-by: Daniel Vetter
> > Reviewed-by: Thomas Hellstrom
>
> I guess this needs your entire damage series to make sense, right?
Ah yes sorry. Although this patch is same as what I sent earlier
except linking with test-drm_modeset.ko
>
> Another question: Does anyone from vmwgfx want drm-misc commit rights for
> pushing stuff like this?
Hi Daniel, Thanks for the suggestion. I am not sure about eligibility criteria
for
commit rights but I think I will wait until drm-mics is moved to gitlab and I
have
more experience working with open-source. Beside me Thomas works on
vmwgfx, I am not sure if he already have commit rights.
> -Daniel
> > ---
> > drivers/gpu/drm/selftests/Makefile| 3 +-
> > .../selftests/drm_damage_helper_selftests.h | 22 +
> > .../drm/selftests/test-drm_damage_helper.c| 828 ++
> > .../drm/selftests/test-drm_modeset_common.c | 10 +-
> > .../drm/selftests/test-drm_modeset_common.h | 1 +
> > 5 files changed, 862 insertions(+), 2 deletions(-)
> > create mode 100644
> drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> > create mode 100644 drivers/gpu/drm/selftests/test-drm_damage_helper.c
> >
> > diff --git a/drivers/gpu/drm/selftests/Makefile
> b/drivers/gpu/drm/selftests/Makefile
> > index 7e6581921da0..c6e63ed12f02 100644
> > --- a/drivers/gpu/drm/selftests/Makefile
> > +++ b/drivers/gpu/drm/selftests/Makefile
> > @@ -1,3 +1,4 @@
> > -test-drm_modeset-y := test-drm_modeset_common.o test-
> drm_plane_helper.o
> > +test-drm_modeset-y := test-drm_modeset_common.o test-
> drm_plane_helper.o \
> > + test-drm_damage_helper.o
> >
> > obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-
> drm_modeset.o
> > diff --git a/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> b/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> > new file mode 100644
> > index ..3a1cbe05bef0
> > --- /dev/null
> > +++ b/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +selftest(damage_iter_no_damage, igt_damage_iter_no_damage)
> > +selftest(damage_iter_no_damage_fractional_src,
> igt_damage_iter_no_damage_fractional_src)
> > +selftest(damage_iter_no_damage_src_moved,
> igt_damage_iter_no_damage_src_moved)
> > +selftest(damage_iter_no_damage_fractional_src_moved,
> igt_damage_iter_no_damage_fractional_src_moved)
> > +selftest(damage_iter_no_damage_not_visible,
> igt_damage_iter_no_damage_not_visible)
> > +selftest(damage_iter_no_damage_no_crtc,
> igt_damage_iter_no_damage_no_crtc)
> > +selftest(damage_iter_no_damage_no_fb,
> igt_damage_iter_no_damage_no_fb)
> > +selftest(damage_iter_simple_damage, igt_damage_iter_simple_damage)
> > +selftest(damage_iter_single_damage, igt_damage_iter_single_damage)
> > +selftest(damage_iter_single_damage_intersect_src,
> igt_damage_iter_single_damage_intersect_src)
> > +selftest(damage_iter_single_damage_outside_src,
> igt_damage_iter_single_damage_outside_src)
> > +selftest(damage_iter_single_damage_fractional_src,
> igt_damage_iter_single_damage_fractional_src)
> > +selftest(damage_iter_single_damage_intersect_fractional_src,
> igt_damage_iter_single_damage_intersect_fractional_src)
> > +selftest(damage_iter_single_damage_outside_fractional_src,
> igt_damage_iter_single_damage_outside_fractional_src)
> > +selftest(damage_iter_single_damage_src_moved,
> igt_damage_iter_single_damage_src_moved)
> > +selftest(damage_iter_single_damage_fractional_src_moved,
> igt_damage_iter_single_damage_fractional_src_moved)
> > +selftest(damage_iter_damage, igt_damage_iter_damage)
> > +selftest(damage_iter_damage_one_intersect,
> igt_damage_iter_damage_one_intersect)
> > +selftest(damage_iter_damage_one_outside,
> igt_damage_iter_damage_one_outside)
> > +selftest(damage_iter_damage_src_moved,
> igt_damage_iter_damage_src_moved)
> > +selftest(damage_iter_damage_not_visible,
> igt_damage_iter_damage_not_visible)
> > diff --git a/drivers/gpu/drm/selftests/test-drm_damage_helper.c
> b/drivers/gpu/drm/selftests/test-drm_damage_helper.c
> > new file mode 100644
> > index ..9cec7778e5b2
> > --- /dev/null
> > +++ b/drivers/gpu/drm/selftests/test-drm_damage_helper.c
> > @@ -0,0 +1,828 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Test case for drm_damage_helper functions
> > + */
> > +
> > +#define pr_fmt(fmt) "drm_damage_helper: " fmt
> > +
> > +#include
> > +
> > +#include "test-drm_modeset_common.h"
> > +
> > +#define TESTS "drm_damage_helper_selftests.h"
> > +#include "drm_selftest.h"
> > +
> > +static void set_plane_src(struct drm_plane_state *state, int x1, int y1,
> > int x2,
> > + int y2)
> > +{
> > + state->src.x1 = x1;
> > + state->src.y1 = y1;
>
RE: [PATCH v3 04/18] drm/selftest: Add drm damage helper selftest
>
> On Tue, Oct 16, 2018 at 02:21:17PM +0200, Daniel Vetter wrote:
> > On Mon, Oct 15, 2018 at 04:11:41PM +0000, Deepak Singh Rawat wrote:
> > > > On Wed, Oct 10, 2018 at 05:16:43PM -0700, Deepak Rawat wrote:
> > > > > Selftest for drm damage helper iterator functions.
> > > > >
> > > > > Cc: ville.syrj...@linux.intel.com
> > > > > Cc: Daniel Vetter
> > > > > Cc: Pekka Paalanen
> > > > > Cc: Daniel Stone
> > > > > Cc: intel-...@lists.freedesktop.org
> > > > > Cc: igt-...@lists.freedesktop.org
> > > > > Cc: petri.latv...@intel.com
> > > > > Cc: ch...@chris-wilson.co.uk
> > > > > Signed-off-by: Deepak Rawat
> > > > > ---
> > > > > drivers/gpu/drm/selftests/Makefile| 3 +-
> > > > > .../selftests/drm_damage_helper_selftests.h | 22 +
> > > > > .../drm/selftests/test-drm_damage_helper.c| 844
> > > > ++
> > > > > 3 files changed, 868 insertions(+), 1 deletion(-)
> > > > > create mode 100644
> > > > drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> > > > > create mode 100644 drivers/gpu/drm/selftests/test-
> > > > drm_damage_helper.c
> > > > >
> > > > > diff --git a/drivers/gpu/drm/selftests/Makefile
> > > > b/drivers/gpu/drm/selftests/Makefile
> > > > > index 9fc349fa18e9..88ac216f5962 100644
> > > > > --- a/drivers/gpu/drm/selftests/Makefile
> > > > > +++ b/drivers/gpu/drm/selftests/Makefile
> > > > > @@ -1 +1,2 @@
> > > > > -obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-
> drm-
> > > > helper.o
> > > > > +obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-
> drm-
> > > > helper.o \
> > > > > + test-drm_damage_helper.o
> > > >
> > > > With the testcase intagrated into the test-drm-helper.ko module, for
> > > > patches 1-4 in this series:
> > > >
> > > > Reviewed-by: Daniel Vetter
> > > >
> > > > Obviously needs some adjusting on the igt side too, since we seem to
> be
> > > > missing the igt scaffolding for tests-drm-helper.ko.
> > > > -Daniel
> > >
> > > Hi Daniel,
> > >
> > > Thanks for the review. I am a little confused here. Should we have single
> > > kernel module for drm plane helper selftest and damage helper selftest?
> > > Also shall I rename the kernel selfttest to kms_*?
> > >
> > > For user-space igt test it should be it makes sense to rename to
> kms_selftets?
> >
> > Since I went back&forth on this way too many times:
> > - igt should be called kms_selftest. Please work together with igt
> > maintainers (Arek and Petri), since we also need to update the CI
> > building infrastructure to make sure it updates the list of subtests
> > implemented by the kernel.
> >
> > - Kernel module I'd call test-drm_modeset.ko. That kernel module can then
> > include the existing test-drm-helper.c (could probably rename to
> > test-drm_plane_helper.c for clarity) and your new damage helper (named
> > test-drm_damage_helper.c for consistency).
> >
> > Does that make sense to everyone?
>
> I was trying to add some selftests, as well here [1], with that in
> mind, I think it makes sense to have just one module, call it
> "test-drm_modeset" or whatever and separate the tests source code base
> on whatever core functionality they are testing.
>
> Besides compiling everything together, probably some stuff will have
> to move out of test-drm-helper.c into some common header. For example
> this "FAIL/FAIL_ON" macros
>
Hi,
Thanks for your input. I have similar change in mind after suggestion from
Daniel. Below is initial draft I did yesterday, will move common code to
a common header.
I hope this aligns with what you are doing.
---
drivers/gpu/drm/selftests/Makefile| 4 ++-
.../gpu/drm/selftests/drm_helper_selftests.c | 27 +++
.../gpu/drm/selftests/drm_helper_selftests.h | 15 +--
...-helper.c => drm_plane_helper_selftests.c} | 16 ---
.../selftests/drm_plane_helper_selftests.h| 9 +++
5 files changed, 51 insertions(+), 20 deletions(-)
create mode 100644 drivers/gpu/drm/selftests/drm_helper_selftests.c
rename drivers/gpu/drm/selftests/{test-drm-helper.c =>
drm_plane_helper_selftests.c} (96%)
create mode 10064
RE: [PATCH v3 04/18] drm/selftest: Add drm damage helper selftest
> > > Obviously needs some adjusting on the igt side too, since we seem to be > > > missing the igt scaffolding for tests-drm-helper.ko. > > > -Daniel > > > > Hi Daniel, > > > > Thanks for the review. I am a little confused here. Should we have single > > kernel module for drm plane helper selftest and damage helper selftest? > > Also shall I rename the kernel selfttest to kms_*? > > > > For user-space igt test it should be it makes sense to rename to > kms_selftets? > > Since I went back&forth on this way too many times: > - igt should be called kms_selftest. Please work together with igt > maintainers (Arek and Petri), since we also need to update the CI > building infrastructure to make sure it updates the list of subtests > implemented by the kernel. > > - Kernel module I'd call test-drm_modeset.ko. That kernel module can then > include the existing test-drm-helper.c (could probably rename to > test-drm_plane_helper.c for clarity) and your new damage helper (named > test-drm_damage_helper.c for consistency). > > Does that make sense to everyone? > Yes it makes sense to me and in fact I had similar changes in mind. And, since existing plane selftest are not invoked from igt it's safe to rename kernel module. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v3 04/18] drm/selftest: Add drm damage helper selftest
> On Wed, Oct 10, 2018 at 05:16:43PM -0700, Deepak Rawat wrote:
> > Selftest for drm damage helper iterator functions.
> >
> > Cc: ville.syrj...@linux.intel.com
> > Cc: Daniel Vetter
> > Cc: Pekka Paalanen
> > Cc: Daniel Stone
> > Cc: intel-...@lists.freedesktop.org
> > Cc: igt-...@lists.freedesktop.org
> > Cc: petri.latv...@intel.com
> > Cc: ch...@chris-wilson.co.uk
> > Signed-off-by: Deepak Rawat
> > ---
> > drivers/gpu/drm/selftests/Makefile| 3 +-
> > .../selftests/drm_damage_helper_selftests.h | 22 +
> > .../drm/selftests/test-drm_damage_helper.c| 844
> ++
> > 3 files changed, 868 insertions(+), 1 deletion(-)
> > create mode 100644
> drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> > create mode 100644 drivers/gpu/drm/selftests/test-
> drm_damage_helper.c
> >
> > diff --git a/drivers/gpu/drm/selftests/Makefile
> b/drivers/gpu/drm/selftests/Makefile
> > index 9fc349fa18e9..88ac216f5962 100644
> > --- a/drivers/gpu/drm/selftests/Makefile
> > +++ b/drivers/gpu/drm/selftests/Makefile
> > @@ -1 +1,2 @@
> > -obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm-
> helper.o
> > +obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm-
> helper.o \
> > + test-drm_damage_helper.o
>
> With the testcase intagrated into the test-drm-helper.ko module, for
> patches 1-4 in this series:
>
> Reviewed-by: Daniel Vetter
>
> Obviously needs some adjusting on the igt side too, since we seem to be
> missing the igt scaffolding for tests-drm-helper.ko.
> -Daniel
Hi Daniel,
Thanks for the review. I am a little confused here. Should we have single
kernel module for drm plane helper selftest and damage helper selftest?
Also shall I rename the kernel selfttest to kms_*?
For user-space igt test it should be it makes sense to rename to kms_selftets?
>
> > diff --git a/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> b/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> > new file mode 100644
> > index ..3a1cbe05bef0
> > --- /dev/null
> > +++ b/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +selftest(damage_iter_no_damage, igt_damage_iter_no_damage)
> > +selftest(damage_iter_no_damage_fractional_src,
> igt_damage_iter_no_damage_fractional_src)
> > +selftest(damage_iter_no_damage_src_moved,
> igt_damage_iter_no_damage_src_moved)
> > +selftest(damage_iter_no_damage_fractional_src_moved,
> igt_damage_iter_no_damage_fractional_src_moved)
> > +selftest(damage_iter_no_damage_not_visible,
> igt_damage_iter_no_damage_not_visible)
> > +selftest(damage_iter_no_damage_no_crtc,
> igt_damage_iter_no_damage_no_crtc)
> > +selftest(damage_iter_no_damage_no_fb,
> igt_damage_iter_no_damage_no_fb)
> > +selftest(damage_iter_simple_damage,
> igt_damage_iter_simple_damage)
> > +selftest(damage_iter_single_damage, igt_damage_iter_single_damage)
> > +selftest(damage_iter_single_damage_intersect_src,
> igt_damage_iter_single_damage_intersect_src)
> > +selftest(damage_iter_single_damage_outside_src,
> igt_damage_iter_single_damage_outside_src)
> > +selftest(damage_iter_single_damage_fractional_src,
> igt_damage_iter_single_damage_fractional_src)
> > +selftest(damage_iter_single_damage_intersect_fractional_src,
> igt_damage_iter_single_damage_intersect_fractional_src)
> > +selftest(damage_iter_single_damage_outside_fractional_src,
> igt_damage_iter_single_damage_outside_fractional_src)
> > +selftest(damage_iter_single_damage_src_moved,
> igt_damage_iter_single_damage_src_moved)
> > +selftest(damage_iter_single_damage_fractional_src_moved,
> igt_damage_iter_single_damage_fractional_src_moved)
> > +selftest(damage_iter_damage, igt_damage_iter_damage)
> > +selftest(damage_iter_damage_one_intersect,
> igt_damage_iter_damage_one_intersect)
> > +selftest(damage_iter_damage_one_outside,
> igt_damage_iter_damage_one_outside)
> > +selftest(damage_iter_damage_src_moved,
> igt_damage_iter_damage_src_moved)
> > +selftest(damage_iter_damage_not_visible,
> igt_damage_iter_damage_not_visible)
> > diff --git a/drivers/gpu/drm/selftests/test-drm_damage_helper.c
> b/drivers/gpu/drm/selftests/test-drm_damage_helper.c
> > new file mode 100644
> > index ..17754734c47a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/selftests/test-drm_damage_helper.c
> > @@ -0,0 +1,844 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Test case for drm_damage_helper functions
> > + */
> > +
> > +#define pr_fmt(fmt) "drm_damage_helper: " fmt
> > +
> > +#include
> > +#include
> > +
> > +#define TESTS "drm_damage_helper_selftests.h"
> > +#include "drm_selftest.h"
> > +
> > +#define FAIL(test, msg, ...) \
> > + do { \
> > + if (test) { \
> > + pr_err("%s/%u: " msg, __FUNCTION__, __LINE__,
> ##__VA_ARGS__); \
> > + return -EINVAL; \
> > + } \
> > + } while (0)
> > +
> > +#define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(
RE: [PATCH 04/18] drm/selftest: Add drm damage helper selftest
> drivers/gpu/drm/selftests/drm_damage_helper_selftests.h > > create mode 100644 drivers/gpu/drm/selftests/test- > drm_damage_helper.c > > > > diff --git a/drivers/gpu/drm/selftests/Makefile > b/drivers/gpu/drm/selftests/Makefile > > index 9fc349fa18e9..88ac216f5962 100644 > > --- a/drivers/gpu/drm/selftests/Makefile > > +++ b/drivers/gpu/drm/selftests/Makefile > > @@ -1 +1,2 @@ > > -obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm- > helper.o > > +obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm- > helper.o \ > > + test-drm_damage_helper.o > > I think it'd be better to add the damage tests to the existing drm-helper > selftest (but as a separate file ofc), using the > > test-drm-helper-$(CONFIG_DRM_DEBUG_SELFTEST) += > > syntax. That avoids having to write yet another igt wrapper test for this. > Also, if you cc intel-...@lists.fd.o with this our CI will run the new > tests for you automatically. > > Cheers, Daniel Hi Daniel, thanks for the review. It looks like adding the damage tests to existing drm-helper-selftest need either renaming the module name (test-drm-helper.ko) or change the filename of test-drm-helper.c. This is because now test-drm-helper.c and test-drm-damage-helper.c have to link together and kernel module is of same name which is causing circular dependency. Both of which looks overkill to me. If it is alright with you can we have separate test module. I already had a patch to integrate the damage test module with igt. Thanks, Deepak > > > > diff --git a/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h > b/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h > > new file mode 100644 > > index ..3a1cbe05bef0 > > --- /dev/null > > +++ b/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h > > @@ -0,0 +1,22 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +selftest(damage_iter_no_damage, igt_damage_iter_no_damage) > > +selftest(damage_iter_no_damage_fractional_src, > igt_damage_iter_no_damage_fractional_src) > > +selftest(damage_iter_no_damage_src_moved, > igt_damage_iter_no_damage_src_moved) > > +selftest(damage_iter_no_damage_fractional_src_moved, > igt_damage_iter_no_damage_fractional_src_moved) > > +selftest(damage_iter_no_damage_not_visible, > igt_damage_iter_no_damage_not_visible) > > +selftest(damage_iter_no_damage_no_crtc, > igt_damage_iter_no_damage_no_crtc) > > +selftest(damage_iter_no_damage_no_fb, > igt_damage_iter_no_damage_no_fb) > > +selftest(damage_iter_simple_damage, > igt_damage_iter_simple_damage) > > +selftest(damage_iter_single_damage, igt_damage_iter_single_damage) > > +selftest(damage_iter_single_damage_intersect_src, > igt_damage_iter_single_damage_intersect_src) > > +selftest(damage_iter_single_damage_outside_src, > igt_damage_iter_single_damage_outside_src) > > +selftest(damage_iter_single_damage_fractional_src, > igt_damage_iter_single_damage_fractional_src) > > +selftest(damage_iter_single_damage_intersect_fractional_src, > igt_damage_iter_single_damage_intersect_fractional_src) > > +selftest(damage_iter_single_damage_outside_fractional_src, > igt_damage_iter_single_damage_outside_fractional_src) > > +selftest(damage_iter_single_damage_src_moved, > igt_damage_iter_single_damage_src_moved) > > +selftest(damage_iter_single_damage_fractional_src_moved, > igt_damage_iter_single_damage_fractional_src_moved) > > +selftest(damage_iter_damage, igt_damage_iter_damage) > > +selftest(damage_iter_damage_one_intersect, > igt_damage_iter_damage_one_intersect) > > +selftest(damage_iter_damage_one_outside, > igt_damage_iter_damage_one_outside) > > +selftest(damage_iter_damage_src_moved, > igt_damage_iter_damage_src_moved) > > +selftest(damage_iter_damage_not_visible, > igt_damage_iter_damage_not_visible) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 05/18] drm: Add helper to implement legacy dirtyfb
> On Thu, Sep 27, 2018 at 05:30:07PM -0700, Deepak Rawat wrote:
> > From: Rob Clark
> >
> > Add an atomic helper to implement dirtyfb support. This is needed to
> > support DSI command-mode panels with x11 userspace (ie. when we can't
> > rely on pageflips to trigger a flush to the panel).
> >
> > v2: Modified the helper to use plane fb_damage_clips property and
> > removed plane_state::dirty flag.
> >
> > v3:
> > - Use uapi drm_mode_rect.
> > - Support annotate flags.
> >
> > Cc: ville.syrj...@linux.intel.com
> > Cc: Daniel Vetter
> > Cc: Pekka Paalanen
> > Signed-off-by: Rob Clark
> > Signed-off-by: Deepak Rawat
> > ---
> > drivers/gpu/drm/drm_damage_helper.c | 121
>
> > include/drm/drm_damage_helper.h | 4 +
> > 2 files changed, 125 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_damage_helper.c
> b/drivers/gpu/drm/drm_damage_helper.c
> > index bc734ddaf180..ecf26275543f 100644
> > --- a/drivers/gpu/drm/drm_damage_helper.c
> > +++ b/drivers/gpu/drm/drm_damage_helper.c
> > @@ -26,6 +26,7 @@
> > *
> > * Authors:
> > * Deepak Rawat
> > + * Rob Clark
> > *
> >
> **
> /
> >
> > @@ -70,6 +71,21 @@
> > * rectangles clipped to &drm_plane_state.src.
> > */
> >
> > +static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
> > + struct drm_mode_rect *dest,
> > + uint32_t num_clips, uint32_t src_inc)
> > +{
> > + while (num_clips > 0) {
> > + dest->x1 = src->x1;
> > + dest->y1 = src->y1;
> > + dest->x2 = src->x2;
> > + dest->y2 = src->y2;
> > + src += src_inc;
> > + dest++;
> > + num_clips--;
> > + }
> > +}
> > +
> > /**
> > * drm_plane_enable_fb_damage_clips - Enables plane fb damage clips
> property.
> > * @plane: Plane on which to enable damage clips property.
> > @@ -123,6 +139,111 @@ void
> drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_check_plane_damage);
> >
> > +/**
> > + * drm_atomic_helper_dirtyfb - Helper for dirtyfb.
> > + * @fb: DRM framebuffer.
> > + * @file_priv: Drm file for the ioctl call.
> > + * @flags: Dirty fb annotate flags.
> > + * @color: Color for annotate fill.
> > + * @clips: Dirty region.
> > + * @num_clips: Count of clip in clips.
> > + *
> > + * A helper to implement &drm_framebuffer_funcs::dirty using damage
> interface
> > + * during plane update. Similar to ioctl, this helper do a full plane
> > update
> > + * when no clips are provided.
>
> The above kerneldoc doesn't make sense to me ... we do have clips provided
> here, it's just a different layout.
Thanks Daniel for the review. I meant if clips are NULL, which is possible.
>
> > + *
> > + * Returns: Zero on success, negative errno on failure.
> > + */
> > +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
> > + struct drm_file *file_priv, unsigned flags,
> > + unsigned color, struct drm_clip_rect *clips,
> > + unsigned num_clips)
> > +{
> > + struct drm_modeset_acquire_ctx ctx;
> > + struct drm_property_blob *damage = NULL;
> > + struct drm_mode_rect *rects = NULL;
> > + struct drm_atomic_state *state;
> > + struct drm_plane *plane;
> > + int ret = 0;
> > +
> > + /*
> > +* When called from ioctl, we are interruptable, but not when called
> > +* internally (ie. defio worker)
> > +*/
> > + drm_modeset_acquire_init(&ctx,
> > + file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
> > +
> > + state = drm_atomic_state_alloc(fb->dev);
> > + if (!state) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > + state->acquire_ctx = &ctx;
> > +
> > + if (clips) {
> > + uint32_t inc = 1;
> > +
> > + if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) {
> > + inc = 2;
> > + num_clips /= 2;
> > + }
> > +
> > + rects = kcalloc(num_clips, sizeof(*rects), GFP_KERNEL);
> > + if (!rects) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + convert_clip_rect_to_rect(clips, rects, num_clips, inc);
> > + damage = drm_property_create_blob(fb->dev,
> > + num_clips * sizeof(*rects),
> > + rects);
> > + if (IS_ERR(damage)) {
> > + ret = PTR_ERR(damage);
> > + damage = NULL;
> > + goto out;
> > + }
> > + }
> > +
> > +retry:
> > + drm_for_each_plane(plane, fb->dev) {
> > + struct drm_plane_state *plane_state;
> > +
> > + if (plane->state->fb != fb)
> > + continue;
> > +
> > + plane_state = drm_atomic
RE: [PATCH 01/14] drm: add new plane property FB_DAMAGE_CLIPS to send damage during plane update
> > > > +#include > > > > +#include > > > > + > > > > +/** > > > > + * DOC: overview > > > > + * > > > > + * FB_DAMAGE_CLIPS is an optional plane property which provides a > > > means to > > > > + * specify a list of damage rectangles on a plane in framebuffer > > > coordinates of > > > > + * the framebuffer attached to the plane. In current context damage is > > > the area > > > > + * of plane framebuffer (excluding the framebuffer area which is > > > > outside > > > of > > > > + * plane src) > > > > > > Not sure why the plane src coordinates need to be mentioned here. The > > > damage is just the part of the fb that has changed. Whether or not it > > > extends past the src coordinates is totally irrelevant as far as I can > > > see. > > > > Thanks Ville for the review, > > > > Well this is plane damage property and only those clips which falls inside > > will be used for plane update, so outside plane src is not required. > > Actually we've never specified that properly. My usual interpretation is > that the plane is allowed to sample past the edge of the src rectangle > (to get nicer looking edges). But not sure whether that would be too > surprising to people though. So we might want follow the GL linear filter > rules by default, and if someone wants better looking edges we could > add a property that allows the filter to reach further past the edge. Ok, it makes sense not to specify plane src part, anyway with current implementation kernel do not error if damage clips are outside plane src. The helper iterator clip to plane_src but depending on the above use case can have other implementations as well. Thanks, Deepak ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 01/14] drm: add new plane property FB_DAMAGE_CLIPS to send damage during plane update
> > On Thu, 6 Sep 2018 21:36:51 +0000 > Deepak Singh Rawat wrote: > > > > > > > - Why no input validation on the damage clips against the framebuffer > > > size? Ime not validating just leads to funny driver bugs down the road, > > > so what's the use-case you have in mind here? > > > > My motivation behind not informing user-space if damage is outside plane > > src, sent during modeset(where it should be provided) or damage is outside > > framebuffer coordinate (simply put damage clips are invalid) is that it's a > > hint. The contract between kernel and user-space is simple, if damage > > clips are valid, kernel might use them (but not always) otherwise they > > will be ignored. Damage can be ignored deep in the stack (like network), > > so all the input validation for nothing. > > > > Also, what all to consider in input validation ? > > * clips are outside plane src > > * damage clips sent during modeset should error ? > > * any other properties. > > > > This will lead to a complex input validation during modeset check like > > if some drivers are OK with damage while scaling whereas others > > cannot support, should this error? > > > > However I am alright doing input validation if this becomes clear what > > kind of validation to actually do. My understanding of drm core is limited. > > Hi, > > I think validating that the area of any clip rectangle does not extend > beyond the framebuffer size would be good. A userspace violating that > is arguably broken, so it would help catch userspace bugs. > > I also would not assume that damage is irrelevant on modeset. Userspace > does not always know if an update will be a modeset or not: drivers vary > there, and userspace that just wants the update through will allow > modesets always while still providing damage. Userspace could also > change video mode timings or pixel format while keeping the resolution > the same, and in that case damage could be meaningful to a driver. > Thanks Pekka, for explanation. I agree clips outside framebuffer should be errored and also it's not a lot for drm core. Also, will look into the cases where drm core should clear the damage clips, like resolution change, etc. May be the better approach would be drm core just provide the helper and driver takes care of driver specific scenarios. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 01/14] drm: add new plane property FB_DAMAGE_CLIPS to send damage during plane update
>
> On Wed, Sep 05, 2018 at 04:38:48PM -0700, Deepak Rawat wrote:
> > From: Lukasz Spintzyk
> >
> > FB_DAMAGE_CLIPS is an 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 "struct 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. As
> > plane src in framebuffer cannot be negative so are damage clips. In
> > damage clip, x1/y1 are inclusive and x2/y2 are exclusive.
> >
> > This patch also exports the kernel internal drm_rect to userspace as
> > drm_mode_rect. This is because "struct drm_clip_rect" is not sufficient
> > to represent damage for current plane size.
> >
> > Upper limit is set on the maximum number of damage clips to avoid
> > overflow by user-space.
> >
> > Driver which are interested in enabling FB_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
> > ---
> > Documentation/gpu/drm-kms.rst | 9 +++
> > drivers/gpu/drm/Makefile| 2 +-
> > drivers/gpu/drm/drm_atomic.c| 13
> > drivers/gpu/drm/drm_atomic_helper.c | 4 ++
> > drivers/gpu/drm/drm_damage_helper.c | 92
> +
> > drivers/gpu/drm/drm_mode_config.c | 6 ++
> > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 1 +
> > include/drm/drm_damage_helper.h | 63
> > include/drm/drm_mode_config.h | 10
> > include/drm/drm_plane.h | 8 +++
> > include/uapi/drm/drm_mode.h | 19 ++
> > 11 files changed, 226 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/gpu/drm/drm_damage_helper.c
> > create mode 100644 include/drm/drm_damage_helper.h
> >
> > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-
> kms.rst
> > index 5dee6b8a4c12..78b66e628857 100644
> > --- a/Documentation/gpu/drm-kms.rst
> > +++ b/Documentation/gpu/drm-kms.rst
> > @@ -542,6 +542,15 @@ Plane Composition Properties
> > .. kernel-doc:: drivers/gpu/drm/drm_blend.c
> > :export:
> >
> > +FB_DAMAGE_CLIPS
> > +~~~
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_damage_helper.c
> > + :doc: overview
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_damage_helper.c
> > + :export:
> > +
> > Color Management Properties
> > ---
> >
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index a6771cef85e2..ca5be0decb3f 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -35,7 +35,7 @@ drm_kms_helper-y := drm_crtc_helper.o
> drm_dp_helper.o drm_probe_helper.o \
> > drm_plane_helper.o drm_dp_mst_topology.o
> drm_atomic_helper.o \
> > drm_kms_helper_common.o drm_dp_dual_mode_helper.o
> \
> > drm_simple_kms_helper.o drm_modeset_helper.o \
> > - drm_scdc_helper.o drm_gem_framebuffer_helper.o
> > + drm_scdc_helper.o drm_gem_framebuffer_helper.o
> drm_damage_helper.o
> >
> > drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
> > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) +=
> drm_fb_helper.o
> > diff --git a/drivers/gpu/drm/drm_atomic.c
> b/drivers/gpu/drm/drm_atomic.c
> > index 3eb061e11e2e..652e78ca1f81 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -857,6 +857,8 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
> > {
> > struct drm_device *dev = plane->dev;
> > struct drm_mode_config *config = &dev->mode_config;
> > + bool replaced = false;
> > + int ret;
> >
> > if (property == config->prop_fb_id) {
> > struct drm_framebuffer *fb =
> drm_framebuffer_lookup(dev, NULL, val);
> > @@ -908,6 +910,14 @@ 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_fb_damage_clips) {
> > + ret = drm_atomic_replace_property_blob_from_id(dev,
> > + &state->fb_damage_clips,
> > + val,
> > + -1,
> > + sizeof(struct drm_rect),
> > + &replaced);
> > + return ret;
> > } else if (plane->funcs->atomic_set_property) {
> > return plane->funcs->atomic_set_property(plane, state,
> > property, val);
> > @@ -976,6 +986,9 @@ drm_atomic_plane_get_property(struct drm_plane
> *plane,
> > *val = state->color_encoding;
> > } else if (property == plane->color_range_property) {
> > *val = state->color_range;
RE: [PATCH 03/14] drm: clear plane damage during full modeset
> > drivers/gpu/drm/drm_atomic_helper.c | 4
> > include/drm/drm_damage_helper.h | 10 ++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> > index be83e2763c18..e06d2d5d582f 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -31,6 +31,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "drm_crtc_helper_internal.h"
> > @@ -88,6 +89,9 @@ drm_atomic_helper_plane_changed(struct
> drm_atomic_state *state,
> > return;
> >
> > crtc_state->planes_changed = true;
> > +
> > + if (drm_atomic_crtc_needs_modeset(crtc_state))
> > + drm_plane_clear_damage(plane_state);
>
> I'm not 100% sure this is the best place to put this. I'm also wondering
> whether we should clear damage when moving planes between crtc on top
> of
> this. But I guess vmwgfx doesn't allow that, we can figure this out when
> the first driver with moveable planes adds damage support.
Yes I agree this is not the best place but it was the one with minimal code
change. IMO should have a separate function for clearing damage.
> > }
> > }
> >
> > diff --git a/include/drm/drm_damage_helper.h
> b/include/drm/drm_damage_helper.h
> > index f1282b459a4f..1f988f7fdd72 100644
> > --- a/include/drm/drm_damage_helper.h
> > +++ b/include/drm/drm_damage_helper.h
> > @@ -71,6 +71,16 @@ drm_plane_get_damage_clips(const struct
> drm_plane_state *state)
> >state->fb_damage_clips->data : NULL);
> > }
> >
> > +/**
> > + * drm_plane_clear_damage - clears damage blob in a plane state
> > + * @state: Plane state
>
> A bit more kerneldoc would be good. Maybe explain how that impacts the
> damage iterator - you get full damage after calling this, which is a bit
> confusing for a function called clear_damage. So definitely worth
> explaining.
>
> With the kerneldoc beefed up:
Agreed.
>
> Reviewed-by: Daniel Vetter
>
> > + */
> > +static inline void drm_plane_clear_damage(struct drm_plane_state
> *state)
> > +{
> > + drm_property_blob_put(state->fb_damage_clips);
> > + state->fb_damage_clips = NULL;
> > +}
> > +
> > void drm_plane_enable_fb_damage_clips(struct drm_plane *plane);
> > int
> > drm_atomic_helper_damage_iter_init(struct
> drm_atomic_helper_damage_iter *iter,
> > --
> > 2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 02/14] drm: add helper iterator functions for plane fb_damage_clips blob
> > #include
> >
> > /**
> > @@ -75,6 +76,11 @@
> > * While kernel does not error for overlapped damage clips, it is
> discouraged.
> > */
> >
> > +static int convert_fixed_to_32(int fixed)
> > +{
> > + return ((fixed >> 15) & 1) + (fixed >> 16);
> > +}
> > +
> > /**
> > * drm_plane_enable_fb_damage_clips - enables plane fb damage clips
> property
> > * @plane: plane on which to enable damage clips property
> > @@ -90,3 +96,90 @@ void drm_plane_enable_fb_damage_clips(struct
> drm_plane *plane)
> >0);
> > }
> > EXPORT_SYMBOL(drm_plane_enable_fb_damage_clips);
> > +
> > +/**
> > + * drm_atomic_helper_damage_iter_init - initialize the damage iterator
> > + * @iter: The iterator to initialize.
> > + * @old_state: old plane state for validation.
> > + * @new_state: plane state from which to iterate the damage clips.
> > + *
> > + * Initialize an iterator that clip framebuffer damage in plane
> fb_damage_clips
> > + * blob to plane src clip. The iterator returns full plane src in case
> > needing
> > + * full update e.g. during full modeset.
> > + *
> > + * With this helper iterator, drivers which enabled fb_damage_clips
> property can
> > + * iterate over the damage clips that falls inside plane src during plane
> > + * update.
> > + *
> > + * Returns: 0 on success and negative error code on error. If an error code
> is
> > + * returned then it means the plane state shouldn't update with attached
> fb.
> > + */
> > +int
> > +drm_atomic_helper_damage_iter_init(struct
> drm_atomic_helper_damage_iter *iter,
> > + const struct drm_plane_state *old_state,
> > + const struct drm_plane_state *new_state)
> > +{
> > + if (!new_state || !new_state->crtc || !new_state->fb)
> > + return -EINVAL;
> > +
> > + if (!new_state->visible)
> > + return -EINVAL;
>
> Can't we handle this by iterating 0 damage rects instead? Would make the
> code a bit cleaner I think.
Agreed.
>
> > +
> > + memset(iter, 0, sizeof(*iter));
> > + iter->clips = drm_plane_get_damage_clips(new_state);
> > + iter->num_clips = drm_plane_get_damage_clips_count(new_state);
> > +
> > + if (!iter->clips)
> > + iter->full_update = true;
> > +
> > + if (!drm_rect_equals(&new_state->src, &old_state->src))
> > + iter->full_update = true;
> > +
> > + iter->plane_src.x1 = convert_fixed_to_32(new_state->src.x1);
> > + iter->plane_src.y1 = convert_fixed_to_32(new_state->src.y1);
> > + iter->plane_src.x2 = convert_fixed_to_32(new_state->src.x2);
> > + iter->plane_src.y2 = convert_fixed_to_32(new_state->src.y2);
>
> I think you want to clip with the clipped rectangles here, not with the
> ones userspace provides.
new_state->src.x1 is the clipped one, clipped in the function
drm_atomic_helper_check_plane_state. Or am I missing something ?
>
> Also I think you're rounding is wrong here - I think you need to round
> down for x/y1, and round up for x/y2 to make sure you catch all the
> pixels?
>
> Unit tests for this, in the form of a drm selftest (like we have for
> drm_mm.c already, but for kms helpers) would be perfect. Much easier to
> review a testcase than do bitmath in my head :-)
Sure I will work on a unit test case for this scenario and work on round up
of plane_src.
>
> > +
> > + if (iter->full_update) {
> > + iter->clips = 0;
> > + iter->curr_clip = 0;
> > + iter->num_clips = 0;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
> > +
> > +/**
> > + * drm_atomic_helper_damage_iter_next - advance the damage iterator
> > + * @iter: The iterator to advance.
> > + * @rect: Return a rectangle in fb coordinate clipped to plane src.
> > + *
> > + * Returns: true if the output is valid, false if we've reached the end of
> the
> > + * rectangle list.
> > + */
> > +bool
> > +drm_atomic_helper_damage_iter_next(struct
> drm_atomic_helper_damage_iter *iter,
> > + struct drm_rect *rect)
> > +{
> > + bool ret = false;
> > +
> > + if (iter->full_update) {
> > + *rect = iter->plane_src;
> > + iter->full_update = false;
> > + return true;
> > + }
> > +
> > + while (iter->curr_clip < iter->num_clips) {
> > + *rect = iter->clips[iter->curr_clip];
> > + iter->curr_clip++;
> > +
> > + if (drm_rect_intersect(rect, &iter->plane_src)) {
> > + ret = true;
> > + break;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
> > diff --git a/include/drm/drm_damage_helper.h
> b/include/drm/drm_damage_helper.h
> > index 217694e9168c..f1282b459a4f 100644
> > --- a/include/drm/drm_damage_helper.h
> > +++ b/include/drm/drm_damage_helper.h
> > @@ -32,6 +32,19 @@
> > #ifndef DRM_DAMAGE_HELPER_H_
> > #define DRM_DAMAGE_HELPER_H_
> >
> > +/**
> > + * s
RE: [PATCH 01/14] drm: add new plane property FB_DAMAGE_CLIPS to send damage during plane update
> >
> > +FB_DAMAGE_CLIPS
> > +~~~
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_damage_helper.c
> > + :doc: overview
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_damage_helper.c
> > + :export:
>
> You forgot to include your nice kerneldoc from the header. Please run
>
> $ make htmldocs
>
> and make sure the generated stuff looks all nice and is complete.
Thanks for the review Daniel, oops I added those docs later in
header and forgot to update here.
>
> > +
> > Color Management Properties
> > ---
> >
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index a6771cef85e2..ca5be0decb3f 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -35,7 +35,7 @@ drm_kms_helper-y := drm_crtc_helper.o
> drm_dp_helper.o drm_probe_helper.o \
> > drm_plane_helper.o drm_dp_mst_topology.o
> drm_atomic_helper.o \
> > drm_kms_helper_common.o drm_dp_dual_mode_helper.o
> \
> > drm_simple_kms_helper.o drm_modeset_helper.o \
> > - drm_scdc_helper.o drm_gem_framebuffer_helper.o
> > + drm_scdc_helper.o drm_gem_framebuffer_helper.o
> drm_damage_helper.o
> >
> > drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
> > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) +=
> drm_fb_helper.o
> > diff --git a/drivers/gpu/drm/drm_atomic.c
> b/drivers/gpu/drm/drm_atomic.c
> > index 3eb061e11e2e..652e78ca1f81 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -857,6 +857,8 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
> > {
> > struct drm_device *dev = plane->dev;
> > struct drm_mode_config *config = &dev->mode_config;
> > + bool replaced = false;
> > + int ret;
> >
> > if (property == config->prop_fb_id) {
> > struct drm_framebuffer *fb =
> drm_framebuffer_lookup(dev, NULL, val);
> > @@ -908,6 +910,14 @@ 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_fb_damage_clips) {
> > + ret = drm_atomic_replace_property_blob_from_id(dev,
> > + &state->fb_damage_clips,
> > + val,
> > + -1,
> > + sizeof(struct drm_rect),
> > + &replaced);
> > + return ret;
> > } else if (plane->funcs->atomic_set_property) {
> > return plane->funcs->atomic_set_property(plane, state,
> > property, val);
> > @@ -976,6 +986,9 @@ drm_atomic_plane_get_property(struct drm_plane
> *plane,
> > *val = state->color_encoding;
> > } else if (property == plane->color_range_property) {
> > *val = state->color_range;
> > + } else if (property == config->prop_fb_damage_clips) {
> > + *val = (state->fb_damage_clips) ?
> > + state->fb_damage_clips->base.id : 0;
> > } else if (plane->funcs->atomic_get_property) {
> > return plane->funcs->atomic_get_property(plane, state,
> property, val);
> > } else {
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> > index 80be74df7ba6..be83e2763c18 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3576,6 +3576,7 @@ void drm_atomic_helper_plane_reset(struct
> drm_plane *plane)
> > /* Reset the alpha value to fully opaque if it matters */
> > if (plane->alpha_property)
> > plane->state->alpha = plane->alpha_property-
> >values[1];
> > + plane->state->fb_damage_clips = NULL;
>
> No need to set to 0, we require kzalloc.
>
> > }
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> > @@ -3598,6 +3599,7 @@ void
> __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
> >
> > state->fence = NULL;
> > state->commit = NULL;
> > + state->fb_damage_clips = NULL;
> > }
> > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
> >
> > @@ -3642,6 +3644,8 @@ void
> __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
> >
> > if (state->commit)
> > drm_crtc_commit_put(state->commit);
> > +
> > + drm_property_blob_put(state->fb_damage_clips);
> > }
> > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
> >
> > diff --git a/drivers/gpu/drm/drm_damage_helper.c
> b/drivers/gpu/drm/drm_damage_helper.c
> > new file mode 100644
> > index ..3e7de5afe7f6
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_damage_helper.c
> > @@ -0,0 +1,92 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> >
> +/*
>
RE: [Linux-graphics-maintainer] [PATCH] drm/vmwgfx: Convert drm_atomic_helper_suspend/resume()
Hi, thanks for the patch. Perhaps can get rid of vmw_kms_resume
and vmw_kms_suspend, otherwise looks good to me.
>
> convert drm_atomic_helper_suspend/resume() to use
> drm_mode_config_helper_suspend/resume().
>
> suspend_state can be removed from vmw_private as
> it will not be used anymore.
>
> Signed-off-by: Ajit Negi
> Signed-off-by: Souptick Joarder
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 6 +++---
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 1 -
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 25 ++---
> 3 files changed, 5 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index bb6dbbe..a317cda 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1521,7 +1521,7 @@ static int vmw_pm_freeze(struct device *kdev)
> WARN_ON(vmw_request_device_late(dev_priv));
> dev_priv->suspend_locked = false;
> ttm_suspend_unlock(&dev_priv->reservation_sem);
> - if (dev_priv->suspend_state)
> + if (dev->mode_config.suspend_state)
> vmw_kms_resume(dev);
> if (dev_priv->enable_fb)
> vmw_fb_on(dev_priv);
> @@ -1558,8 +1558,8 @@ static int vmw_pm_restore(struct device *kdev)
> vmw_fence_fifo_up(dev_priv->fman);
> dev_priv->suspend_locked = false;
> ttm_suspend_unlock(&dev_priv->reservation_sem);
> - if (dev_priv->suspend_state)
> - vmw_kms_resume(dev_priv->dev);
> + if (dev->mode_config.suspend_state)
> + vmw_kms_resume(dev);
>
> if (dev_priv->enable_fb)
> vmw_fb_on(dev_priv);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 1abe217..b8e200b 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -438,7 +438,6 @@ struct vmw_private {
> struct vmw_framebuffer *implicit_fb;
> struct mutex global_kms_state_mutex;
> spinlock_t cursor_lock;
> - struct drm_atomic_state *suspend_state;
>
> /*
>* Context and surface management.
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 23beff5..2d011a1 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -3028,19 +3028,7 @@ int vmw_kms_set_config(struct drm_mode_set
> *set,
> */
> int vmw_kms_suspend(struct drm_device *dev)
> {
> - struct vmw_private *dev_priv = vmw_priv(dev);
> -
> - dev_priv->suspend_state = drm_atomic_helper_suspend(dev);
> - if (IS_ERR(dev_priv->suspend_state)) {
> - int ret = PTR_ERR(dev_priv->suspend_state);
> -
> - DRM_ERROR("Failed kms suspend: %d\n", ret);
> - dev_priv->suspend_state = NULL;
> -
> - return ret;
> - }
> -
> - return 0;
> + return drm_mode_config_helper_suspend(dev);
> }
>
>
> @@ -3055,16 +3043,7 @@ int vmw_kms_suspend(struct drm_device *dev)
> */
> int vmw_kms_resume(struct drm_device *dev)
> {
> - struct vmw_private *dev_priv = vmw_priv(dev);
> - int ret;
> -
> - if (WARN_ON(!dev_priv->suspend_state))
> - return 0;
> -
> - ret = drm_atomic_helper_resume(dev, dev_priv->suspend_state);
> - dev_priv->suspend_state = NULL;
> -
> - return ret;
> + return drm_mode_config_helper_resume(dev);
> }
>
> /**
> --
> 1.9.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [Linux-graphics-maintainer] [PATCH] drm/vmwgfx: Fix potential Spectre v1
Looks good to me based on my limited understanding. Thomas/Sinclair can could you please review and then we can include this in drm-fixes. Thanks, Deepak > > arg.version is indirectly controlled by user-space, hence leading to > a potential exploitation of the Spectre variant 1 vulnerability. > > This issue was detected with the help of Smatch: > > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:4526 vmw_execbuf_ioctl() > warn: > potential spectre issue 'copy_offset' [w] > > Fix this by sanitizing arg.version before using it to index copy_offset > > Notice that given that speculation windows are large, the policy is > to kill the speculation on the first load and not worry if it can be > completed with a dependent load/store [1]. > > [1] > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmarc.i > nfo%2F%3Fl%3Dlinux- > kernel%26m%3D152449131114778%26w%3D2&data=02%7C01%7Clinux- > graphics- > maintainer%40vmware.com%7Cf010b707b8ef4896c1a908d603aebcc6%7Cb39 > 138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636700446365603728& > sdata=0D8lnUScxOmCCWXLHh8Otc3o%2F1yF1SxgGwIklRdMlXY%3D&re > served=0 > > Cc: sta...@vger.kernel.org > Signed-off-by: Gustavo A. R. Silva > --- > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > index 1f13457..ad91c6e 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > @@ -25,6 +25,7 @@ > * > > ** > / > #include > +#include > > #include "vmwgfx_drv.h" > #include "vmwgfx_reg.h" > @@ -4520,8 +4521,10 @@ int vmw_execbuf_ioctl(struct drm_device *dev, > unsigned long data, > return -EINVAL; > } > > - if (arg.version > 1 && > - copy_from_user(&arg.context_handle, > + if (arg.version >= ARRAY_SIZE(copy_offset)) > + return -EFAULT; > + arg.version = array_index_nospec(arg.version, > ARRAY_SIZE(copy_offset)); > + if (copy_from_user(&arg.context_handle, > (void __user *) (data + copy_offset[0]), > copy_offset[arg.version - 1] - > copy_offset[0]) != 0) > -- > 2.7.4 > > ___ > Sent to linux-graphics-maintai...@vmware.com ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 10/10] drm/vmwgfx: Use __drm_atomic_helper_plane_reset instead of copying the logic
Hi Alexandru, Thanks for the patch, for the vmwgfx part: Reviewed-by: Deepak Rawat > > Signed-off-by: Alexandru Gheorghe cosmin.gheor...@arm.com> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > index 466336b34fff..1e0fb3c79b50 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > @@ -720,9 +720,7 @@ void vmw_du_plane_reset(struct drm_plane *plane) > return; > } > > - plane->state = &vps->base; > - plane->state->plane = plane; > - plane->state->rotation = DRM_MODE_ROTATE_0; > + __drm_atomic_helper_plane_reset(plane, &vps->base); > } > > > -- > 2.18.0 > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [Intel-gfx] [PATCH 5/7] drm/vmwgfx: Stop updating plane->fb
>
> On Fri, Apr 06, 2018 at 10:35:00PM +0300, Ville Syrjälä wrote:
> > On Fri, Apr 06, 2018 at 07:14:51PM +0000, Deepak Singh Rawat wrote:
> > > This makes sense once we got rid of plane->fb
> > >
> > > Will this go to drm-next?
> >
> > The plan is to push to drm-misc-next once we get all
> > the ducks in a row.
> >
> > > Could you please CC
> > > me so that I can do some testing myself. Thanks.
> >
> > Here's a branch if you want a head start:
> > git://github.com/vsyrjala/linux.git plane_fb_crtc_nuke_2
> >
> > I'd definitely appreciate some testing of this stuff. Wouldn't
> > want to break you stuff accidentally.
>
> Did we get anywhere with testing this? I'd like to land the remaining
> bits, but I'd feel much safer doing that if it was tested.
Hi Ville,
I did some basic mode-setting testing by taking your patches to
vmwgfx private branch and things seems to work fine.
Thanks,
Deepak
>
> >
> > >
> > > Reviewed-by: Deepak Rawat
> > >
> > >
> > > >
> > > > From: Ville Syrjälä
> > > >
> > > > We want to get rid of plane->fb on atomic drivers. Stop setting it.
> > > >
> > > > Cc: Thomas Hellstrom
> > > > Cc: Sinclair Yeh
> > > > Cc: VMware Graphics
> > > > Cc: Daniel Vetter
> > > > Signed-off-by: Ville Syrjälä
> > > > ---
> > > > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 2 --
> > > > drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 2 --
> > > > 2 files changed, 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> > > > b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> > > > index 648f8127f65a..bbd3f19b1a0b 100644
> > > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> > > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> > > > @@ -525,8 +525,6 @@
> vmw_sou_primary_plane_atomic_update(struct
> > > > drm_plane *plane,
> > > > */
> > > > if (ret != 0)
> > > > DRM_ERROR("Failed to update screen.\n");
> > > > -
> > > > - crtc->primary->fb = plane->state->fb;
> > > > } else {
> > > > /*
> > > > * When disabling a plane, CRTC and FB should always be
> > > > NULL
> > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > > > b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > > > index 67331f01ef32..90445bc590cb 100644
> > > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > > > @@ -1285,8 +1285,6 @@
> vmw_stdu_primary_plane_atomic_update(struct
> > > > drm_plane *plane,
> > > > 1, 1, NULL,
> > > > crtc);
> > > > if (ret)
> > > > DRM_ERROR("Failed to update STDU.\n");
> > > > -
> > > > - crtc->primary->fb = plane->state->fb;
> > > > } else {
> > > > crtc = old_state->crtc;
> > > > stdu = vmw_crtc_to_stdu(crtc);
> > > > --
> > > > 2.16.1
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > ___
> > Intel-gfx mailing list
> > intel-...@lists.freedesktop.org
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lists.freedesktop.org_mailman_listinfo_intel-
> 2Dgfx&d=DwIDAw&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf-
> cyZdStnd2NQpRu98lJP2iYGw&m=3J7W8_yE3JhMDcN3FfZN8bWZON61wueSY
> YfSGxPNHVE&s=TqYFqV1NCzCnakZHMWVyJ9k42n0CUm5Kcl9xW2Cdvz4&e=
>
> --
> Ville Syrjälä
> Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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.
> --
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 5/7] drm/vmwgfx: Stop updating plane->fb
This makes sense once we got rid of plane->fb
Will this go to drm-next? Could you please CC
me so that I can do some testing myself. Thanks.
Reviewed-by: Deepak Rawat
>
> From: Ville Syrjälä
>
> We want to get rid of plane->fb on atomic drivers. Stop setting it.
>
> Cc: Thomas Hellstrom
> Cc: Sinclair Yeh
> Cc: VMware Graphics
> Cc: Daniel Vetter
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 2 --
> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 2 --
> 2 files changed, 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> index 648f8127f65a..bbd3f19b1a0b 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> @@ -525,8 +525,6 @@ vmw_sou_primary_plane_atomic_update(struct
> drm_plane *plane,
>*/
> if (ret != 0)
> DRM_ERROR("Failed to update screen.\n");
> -
> - crtc->primary->fb = plane->state->fb;
> } else {
> /*
>* When disabling a plane, CRTC and FB should always be
> NULL
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index 67331f01ef32..90445bc590cb 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -1285,8 +1285,6 @@ vmw_stdu_primary_plane_atomic_update(struct
> drm_plane *plane,
>1, 1, NULL, crtc);
> if (ret)
> DRM_ERROR("Failed to update STDU.\n");
> -
> - crtc->primary->fb = plane->state->fb;
> } else {
> crtc = old_state->crtc;
> stdu = vmw_crtc_to_stdu(crtc);
> --
> 2.16.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 6/7] drm/vmwgfx: Stop using plane->fb in atomic_enable()
>
> From: Ville Syrjälä
>
> Instead of looking at the (soon to be deprecated) plane->fb we'll
> examing plane->state->fb instead. We can do this because
> vmw_du_crtc_atomic_check() prevents us from enabling a crtc
> without the primary plane also being enabled.
>
> Due to that same reason, I'm actually not sure what the checks here are
> for NULL fb. If we can't enable the crtc without an enabled plane
> we should always have an fb. But I'll leave that for someone else
> to figure out.
Hi Ville,
AFAIK the NULL check is set or clear the implicit framebuffer property
which is specific to vmwgfx and for current hardware version is disabled.
I have this future TODO work item to get rid of implicit placement property
or at least make it read only.
I still don’t have complete understanding of atomic state but this patch looks
good to me.
Reviewed-by: Deepak Rawat
>
> Cc: Thomas Hellstrom
> Cc: Sinclair Yeh
> Cc: VMware Graphics
> Cc: Daniel Vetter
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index 90445bc590cb..152e96cb1c01 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -414,6 +414,7 @@ static void vmw_stdu_crtc_helper_prepare(struct
> drm_crtc *crtc)
> static void vmw_stdu_crtc_atomic_enable(struct drm_crtc *crtc,
> struct drm_crtc_state *old_state)
> {
> + struct drm_plane_state *plane_state = crtc->primary->state;
> struct vmw_private *dev_priv;
> struct vmw_screen_target_display_unit *stdu;
> struct vmw_framebuffer *vfb;
> @@ -422,7 +423,7 @@ static void vmw_stdu_crtc_atomic_enable(struct
> drm_crtc *crtc,
>
> stdu = vmw_crtc_to_stdu(crtc);
> dev_priv = vmw_priv(crtc->dev);
> - fb = crtc->primary->fb;
> + fb = plane_state->fb;
>
> vfb = (fb) ? vmw_framebuffer_to_vfb(fb) : NULL;
>
> --
> 2.16.1
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 4/7] drm/vmwgfx: Stop using plane->fb in vmw_kms_update_implicit_fb()
Reviewed-by: Deepak Rawat
>
> From: Ville Syrjälä
>
> The only caller of vmw_kms_update_implicit_fb() is the page_flip
> hook which itself gets called with the plane mutex already held.
> Hence we can look at plane->state safely. Toss in a lockdep assert
> to make the situation more clear.
>
> Cc: Thomas Hellstrom
> Cc: Sinclair Yeh
> Cc: VMware Graphics
> Cc: Daniel Vetter
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 5a824125c231..a93d290b0f35 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -2811,14 +2811,17 @@ void vmw_kms_update_implicit_fb(struct
> vmw_private *dev_priv,
> struct drm_crtc *crtc)
> {
> struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> + struct drm_plane *plane = crtc->primary;
> struct vmw_framebuffer *vfb;
>
> + lockdep_assert_held(&plane->mutex);
> +
> mutex_lock(&dev_priv->global_kms_state_mutex);
>
> if (!du->is_implicit)
> goto out_unlock;
>
> - vfb = vmw_framebuffer_to_vfb(crtc->primary->fb);
> + vfb = vmw_framebuffer_to_vfb(plane->state->fb);
> WARN_ON_ONCE(dev_priv->num_implicit != 1 &&
>dev_priv->implicit_fb != vfb);
>
> --
> 2.16.1
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
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= ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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: [PATCH 2/7] drm/vmwgfx: Stop using plane->fb in vmw_kms_atomic_check_modeset()
>
> On Thu, Apr 05, 2018 at 08:15:05PM +, Deepak Singh Rawat wrote:
> >
> > >
> > > From: Ville Syrjälä
> > >
> > > Instead of looking at plane->fb let's look at the proper new
> > > plane state.
> > >
> > > Not that the code makes a ton of sense. It's only going through the
> > > crtcs in the atomic state, so assuming not all of them are included
> > > we're not even calculating the total bandwidth here. Also we're
> > > not considering whether each crtc is actually enabled or not.
> > >
> > > Cc: Thomas Hellstrom
> > > Cc: Sinclair Yeh
> > > Cc: VMware Graphics
> > > Cc: Daniel Vetter
> > > Signed-off-by: Ville Syrjälä
> > > ---
> > > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 10 +++---
> > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > index 6728c6247b4b..a2a796b4cc23 100644
> > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > @@ -1536,9 +1536,13 @@ vmw_kms_atomic_check_modeset(struct
> > > drm_device *dev,
> > > unsigned long requested_bb_mem = 0;
> > >
> > > if (dev_priv->active_display_unit ==
> > > vmw_du_screen_target) {
> > > - if (crtc->primary->fb) {
> > > - int cpp = crtc->primary->fb->pitches[0] /
> > > - crtc->primary->fb->width;
> > > + struct drm_plane *plane = crtc->primary;
> > > + struct drm_plane_state *plane_state;
> > > +
> > > + plane_state =
> > > drm_atomic_get_new_plane_state(state, plane);
> > > +
> > > + if (plane_state && plane_state->fb) {
> > > + int cpp = plane_state->fb->format->cpp[0];
> >
> > Hi Ville,
> >
> > Thanks for the patch, recently I have done some refactoring of this code
> area
> > which is no yet sent to dri-devel. But the refactored code eliminated the
> need
> > to look the fb
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__cgit.freedesktop.org_mesa_vmwgfx_commit_-3Fid-
> 3Dc54cdb6549b7d1c04ff61e639fc0e6de0dcc1ed6&d=DwIDAw&c=uilaK90D4T
> OVoH58JNXRgQ&r=zOOG28inJK0762SxAf-
> cyZdStnd2NQpRu98lJP2iYGw&m=lrHQqnjNpBdFNzU0f4b3rLTtaYp0VRzCgZztJ
> ew0kz0&s=mz1Kt2NO_HIpgVtQ9xHvKRQLGXx2HSqY8xt0oiEpGWg&e=
>
> Hmm. What's the timelike for landing that stuff?
I am not sure, I still think there is more work here to clean this up. I guess
for now
we can have your patch to take care of avoiding plane->fb.
>
> A cursory glance tells me we should just change the current code with
> s/cpp/4/ and it should still be fine?
Yes replacing cpp with 4 is fine as that is what virtual hardware always look
for.
>
> BTW the drm_for_each_crtc(crtc, dev) loop in there doesn't look entirely
> kosher. It's potentially going to access crtc->state w/o holding the lock.
Thanks for the suggestion, I will look into this.
>
> >
> > There is still some future work to be done in this area.
> >
> > >
> > > requested_bb_mem += crtc->mode.hdisplay
> > > * cpp *
> > > crtc->mode.vdisplay;
> > > --
> > > 2.16.1
>
> --
> Ville Syrjälä
> Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 2/7] drm/vmwgfx: Stop using plane->fb in vmw_kms_atomic_check_modeset()
>
> From: Ville Syrjälä
>
> Instead of looking at plane->fb let's look at the proper new
> plane state.
>
> Not that the code makes a ton of sense. It's only going through the
> crtcs in the atomic state, so assuming not all of them are included
> we're not even calculating the total bandwidth here. Also we're
> not considering whether each crtc is actually enabled or not.
>
> Cc: Thomas Hellstrom
> Cc: Sinclair Yeh
> Cc: VMware Graphics
> Cc: Daniel Vetter
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 6728c6247b4b..a2a796b4cc23 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -1536,9 +1536,13 @@ vmw_kms_atomic_check_modeset(struct
> drm_device *dev,
> unsigned long requested_bb_mem = 0;
>
> if (dev_priv->active_display_unit ==
> vmw_du_screen_target) {
> - if (crtc->primary->fb) {
> - int cpp = crtc->primary->fb->pitches[0] /
> - crtc->primary->fb->width;
> + struct drm_plane *plane = crtc->primary;
> + struct drm_plane_state *plane_state;
> +
> + plane_state =
> drm_atomic_get_new_plane_state(state, plane);
> +
> + if (plane_state && plane_state->fb) {
> + int cpp = plane_state->fb->format->cpp[0];
Hi Ville,
Thanks for the patch, recently I have done some refactoring of this code area
which is no yet sent to dri-devel. But the refactored code eliminated the need
to look the fb
https://cgit.freedesktop.org/mesa/vmwgfx/commit/?id=c54cdb6549b7d1c04ff61e639fc0e6de0dcc1ed6
There is still some future work to be done in this area.
>
> requested_bb_mem += crtc->mode.hdisplay
> * cpp *
> crtc->mode.vdisplay;
> --
> 2.16.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
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= ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf
> Of Daniel Vetter
> Sent: Thursday, December 21, 2017 5:11 AM
> To: Ville Syrjälä
> Cc: airl...@linux.ie; daniel.vet...@intel.com; dri-
> de...@lists.freedesktop.org; Lukasz Spintzyk
>
> Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for
> drm_plane
>
> On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote:
> > On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
> > > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
> > > Signed-off-by: Lukasz Spintzyk
> > > ---
> > > drivers/gpu/drm/drm_atomic.c | 10 ++
> > > drivers/gpu/drm/drm_mode_config.c | 6 ++
> > > drivers/gpu/drm/drm_plane.c | 1 +
> > > include/drm/drm_mode_config.h | 5 +
> > > include/drm/drm_plane.h | 3 +++
> > > 5 files changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c
> b/drivers/gpu/drm/drm_atomic.c
> > > index b76d49218cf1..cd3b4ed7b04c 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
> > > state->rotation = val;
> > > } else if (property == plane->zpos_property) {
> > > state->zpos = val;
> > > + } else if (property == config->dirty_rects_property) {
> > > + bool replaced = false;
> > > + int ret = drm_atomic_replace_property_blob_from_id(dev,
> > > + &state->dirty_blob,
> > > + val,
> > > + -1,
> > > + &replaced);
> > > + return ret;
> > > } else if (plane->funcs->atomic_set_property) {
> > > return plane->funcs->atomic_set_property(plane, state,
> > > property, val);
> > > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct
> drm_plane *plane,
> > > *val = state->rotation;
> > > } else if (property == plane->zpos_property) {
> > > *val = state->zpos;
> > > + } else if (property == config->dirty_rects_property) {
> > > + *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0;
> > > } else if (plane->funcs->atomic_get_property) {
> > > return plane->funcs->atomic_get_property(plane, state,
> property, val);
> > > } else {
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c
> b/drivers/gpu/drm/drm_mode_config.c
> > > index bc5c46306b3d..d5f1021c6ece 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -293,6 +293,12 @@ static int
> drm_mode_create_standard_properties(struct drm_device *dev)
> > > return -ENOMEM;
> > > dev->mode_config.prop_crtc_id = prop;
> > >
> > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > > + "DIRTY_RECTS", 0);
> > > + if (!prop)
> > > + return -ENOMEM;
> > > + dev->mode_config.dirty_rects_property = prop;
> > > +
> > > prop = drm_property_create_bool(dev,
> DRM_MODE_PROP_ATOMIC,
> > > "ACTIVE");
> > > if (!prop)
> > > diff --git a/drivers/gpu/drm/drm_plane.c
> b/drivers/gpu/drm/drm_plane.c
> > > index 37a93cdffb4a..add110f025e5 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device
> *dev, struct drm_plane *plane,
> > > drm_object_attach_property(&plane->base, config-
> >prop_src_y, 0);
> > > drm_object_attach_property(&plane->base, config-
> >prop_src_w, 0);
> > > drm_object_attach_property(&plane->base, config-
> >prop_src_h, 0);
> > > + drm_object_attach_property(&plane->base, config-
> >dirty_rects_property, 0);
> > > }
> > >
> > > if (config->allow_fb_modifiers)
> > > diff --git a/include/drm/drm_mode_config.h
> b/include/drm/drm_mode_config.h
> > > index e5f3b43014e1..65f64eb04c0c 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -599,6 +599,11 @@ struct drm_mode_config {
> > >* &drm_crtc.
> > >*/
> > > struct drm_property *prop_crtc_id;
> > > + /**
> > > + * @dirty_rects_property: Optional plane property to mark damaged
> > > + * regions on the plane framebuffer.
> >
> > What exactly would the blob contain?
> >
> > The comment seems to be implying these are in fb coordiantes as
> > opposed to plane crtc coordinates? Not sure which would be more
> > convenient. At least if they're fb coordinates then you probably
> > want some helpers to translate/rotate/scale those rects to the
> > crtc coordinates. Actual use depends on the driver/hw I suppose.
>
> Yeah I think we also should add a decoded state to the drm_plane_state,
> which has the full structure and all the details.
Hi Daniel,
I am working on this functionality to implement the helper functions t
RE: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
Hi Lukasz,
I hope you are doing great. I was busy with some other stuff but now will be
working on page-flip damage. At this point I have high level understanding of
what changes to make and before I start just wanted to confirm from you,
whether you have made any progress to avoid duplicate work.
Thanks,
Deepak
From: Lukasz Spintzyk [mailto:lukasz.spint...@displaylink.com]
Sent: Thursday, January 4, 2018 5:53 AM
To: Thomas Hellstrom ; dri-devel@lists.freedesktop.org;
daniel.vet...@intel.com; gust...@padovan.org; seanp...@chromium.org;
airl...@linux.ie; Deepak Singh Rawat
Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
On 28/12/2017 10:03, Thomas Hellstrom wrote:
Hi, Lukasz!
(Sorry for top-posting).
We have Deepak from our team working on the same subject. I think he's in over
the holidays so I'll add him to the CC list.
Great!
Adding damage to the plane state is, IMO the correct way to do it. However,
from your proposal it's not clear whether damage is given in the plane-, crtc-
or framebuffer coordinates. The last conclusion from our email thread
discussion was that they should be given in framebuffer coordinates with
helpers to compute plane coordinates or crtc coordinates. The reason being that
it's easier for user-space apps to send damage that way, and again, we have the
full information that we can clip and scale if necessary. Most drivers probably
want the damage in clipped plane- or crtc coordinates. Helpers could for
example be in the form of region iterators.
Personally i don't know the difference between plane rects and framebuffer
rects. I don't know what would be better. I was thinking about coordinates of
framebuffer that is attached to drm_plane_state.
Full (multi-rect) damage regions are OK with us, although we should keep in
mind that we won't be able to compute region unions in the kernel (yet?).
Implying: Should we forbid overlapping rects at the interface level or should
we just recommend rects not to be overlapping? The former would be pretty hard
to enforce efficiently.
I would be for recommendation. We can add some helper functions to combine
rects and set some limits on number of rects to prevent abuse of that interface.
Another thing we should think about is how to use this interface for the legacy
"dirtyfb" call. Probably we need to clear the damage property on each
state-update, or set a flag that "this is a dirtyfb state update".
IMO we should also have as an end goal of this work to have gnome-shell on drm
sending damage regions on page-flip, which means either porting gnome-shell to
atomic or set up a new legacy page-flip-with-atomic ioctl.
Can't we reuse dirtyfb ioctl for this purpose? It would be called before
page_flip ioctl?
/Thomas
On 12/21/2017 12:10 PM, Lukasz Spintzyk wrote:
Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
Signed-off-by: Lukasz Spintzyk mailto:lukasz.spint...@displaylink.com
---
drivers/gpu/drm/drm_atomic.c | 10 ++
drivers/gpu/drm/drm_mode_config.c | 6 ++
drivers/gpu/drm/drm_plane.c | 1 +
include/drm/drm_mode_config.h | 5 +
include/drm/drm_plane.h | 3 +++
5 files changed, 25 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b76d49218cf1..cd3b4ed7b04c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct drm_plane
*plane,
state->rotation = val;
} else if (property == plane->zpos_property) {
state->zpos = val;
+ } else if (property == config->dirty_rects_property) {
+ bool replaced = false;
+ int ret = drm_atomic_replace_property_blob_from_id(dev,
+ &state->dirty_blob,
+ val,
+ -1,
+ &replaced);
+ return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->rotation;
} else if (property == plane->zpos_property) {
*val = state->zpos;
+ } else if (property == config->dirty_rects_property) {
+ *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0;
} else if (plane->funcs->atomic_get_property) {
return plane->funcs->atomic_get_property(plane, state, property,
val);
} else {
diff --git a/drivers/gpu/drm/drm_mode_config.c
b/drivers/gpu/drm/drm_mode_config.c
index bc5c46306b3d..d5f1021c6ece 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -293,6 +293,12 @@ static int drm_mode_create_s
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-devel@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-devel@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=
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: page-flip with damage?
Hi Michal, Are you currently working on this feature. I started to look into page flip with damage for VMware drivers. Thanks, Deepak > -Original Message- > From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf > Of Michal Lukaszek > Sent: Tuesday, September 26, 2017 12:02 AM > To: dri-devel@lists.freedesktop.org > Subject: RE: page-flip with damage? > > Thomas Hellstrom wrote: > > > > > Page flips, while efficient on real hardware, aren't that efficient in > > > other situations, like for virtual devices with local, or even worse, > > > So I'd like to start looking at page-flips with damage > > > > This is funny, it's exactly what we pointed out during the lighting talk at > XDC2017 > > https://urldefense.proofpoint.com/v2/url?u=https- > 3A__youtu.be_R2XHZd4uXRI-3Ft- > 3D6h2m50s&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762S > xAf-cyZdStnd2NQpRu98lJP2iYGw&m=MeqFMzaER- > gbFwJbxAovSVKR5y4mpUOeQkVPBC4Ixrs&s=kq0q9TAXS1cc- > 8aAgqYcfeYyh6oLZOLs4NL9-ixdPzI&e= > > > > And VMware gets a mention, too. > > > > :) > > > > Join forces? > > > > Thanks, > > Michal > > ___ > dri-devel mailing list > dri-devel@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=MeqFMzaER- > gbFwJbxAovSVKR5y4mpUOeQkVPBC4Ixrs&s=ppNKRV_ldifYVpCbA-r- > 6e1vc7_6Wu_KboAacOSdM30&e= ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Behavior of drmModeSetCrtc wrt to framebuffer
Hi All, I need your help in determining the behavior of drmModeSetCrtc when it is called with a FB and mode. Should it display the content of FB to physical screen or not? I ran the program https://github.com/dvdhrm/docs/blob/master/drm-howto/modeset.c with Vmware's vmwgfx drm driver and it will not display the FB, but calling drmModeDirtyFB after the FB has modified, the program works as expected (to display the fb content to screen). Also the same program works as expected with intel drm driver. Please let me know if you need any other information. Thanks in advance, Deepak ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
