diff mbox series

[v3] drm/vkms: Add overlay plane support

Message ID 20190309153920.GA3646@armorer (mailing list archive)
State New, archived
Headers show
Series [v3] drm/vkms: Add overlay plane support | expand

Commit Message

Mamta Shukla March 9, 2019, 3:39 p.m. UTC
Add overlay plane support in vkms aligned with cursor and primary
plane with module option 'enable_overlay' to enable/disable overlay
plane while testing.

This currently passes plane-position-covered-pipe-A-plane subtest
from IGT kms_plane test.

Signed-off-by: Mamta Shukla <mamtashukla555@gmail.com>
---
changes in v3:
-Fix spelling mistake 'palne'->'plane'
-Use switch case
-Use logical operator '||' in place of '|'

change in v2:
-Fix warning: return makes pointer from integer without a cast using
 ERR_PTR

 drivers/gpu/drm/vkms/vkms_crc.c   | 42 +++++++++++++++++++++++++++----
 drivers/gpu/drm/vkms/vkms_drv.c   |  4 +++
 drivers/gpu/drm/vkms/vkms_drv.h   |  8 ++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 36 +++++++++++++++++++++++++-
 4 files changed, 84 insertions(+), 6 deletions(-)

Comments

Mamta Shukla March 12, 2019, 7:06 p.m. UTC | #1
On Sun, Mar 10, 2019 at 9:00 PM Rodrigo Siqueira
<rodrigosiqueiramelo@gmail.com> wrote:
>
> Hi,
>
> Thanks for your patch.
>
> You can see my comments inline.
>
> On 03/09, Mamta Shukla wrote:
> > Add overlay plane support in vkms aligned with cursor and primary
> > plane with module option 'enable_overlay' to enable/disable overlay
> > plane while testing.
> >
> > This currently passes plane-position-covered-pipe-A-plane subtest
> > from IGT kms_plane test.
>
> What the difference of running this test with and without your patch?
> This test pass without your patch as well. Additionally, I briefly
> looked at the test, and l could not get confident that
> "plane-position-covered-pipe-A-plane" validate overlays. Did I miss
> something?
>
> > Signed-off-by: Mamta Shukla <mamtashukla555@gmail.com>
> > ---
> > changes in v3:
> > -Fix spelling mistake 'palne'->'plane'
> > -Use switch case
> > -Use logical operator '||' in place of '|'
> >
> > change in v2:
> > -Fix warning: return makes pointer from integer without a cast using
> >  ERR_PTR
> >
> >  drivers/gpu/drm/vkms/vkms_crc.c   | 42 +++++++++++++++++++++++++++----
> >  drivers/gpu/drm/vkms/vkms_drv.c   |  4 +++
> >  drivers/gpu/drm/vkms/vkms_drv.h   |  8 ++++++
> >  drivers/gpu/drm/vkms/vkms_plane.c | 36 +++++++++++++++++++++++++-
> >  4 files changed, 84 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > index 4dd6c155363d..82bfee7c79b5 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > @@ -109,6 +109,26 @@ static void blend(void *vaddr_dst, void *vaddr_src,
> >       }
> >  }
> >
> > +static void compose_overlay(struct vkms_crc_data *overlay_crc,
> > +                         struct vkms_crc_data *primary_crc, void *vaddr_out)
> > +{
> > +     struct drm_gem_object *overlay_obj;
> > +     struct vkms_gem_object  *overlay_vkms_obj;
> > +
> > +     overlay_obj = drm_gem_fb_get_obj(&overlay_crc->fb, 0);
> > +     overlay_vkms_obj = drm_gem_to_vkms_gem(overlay_obj);
> > +     mutex_lock(&overlay_vkms_obj->pages_lock);
> > +     if (!overlay_vkms_obj->vaddr) {
> > +             DRM_WARN("overlay plane vaddr is NULL");
> > +             goto out;
> > +     }
> > +
> > +     blend(vaddr_out, overlay_vkms_obj->vaddr, primary_crc, overlay_crc);
> > +
> > +out:
> > +     mutex_unlock(&overlay_vkms_obj->pages_lock);
> > +}
> > +
> >  static void compose_cursor(struct vkms_crc_data *cursor_crc,
> >                          struct vkms_crc_data *primary_crc, void *vaddr_out)
> >  {
> > @@ -131,7 +151,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc,
> >  }
> >
> >  static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> > -                           struct vkms_crc_data *cursor_crc)
> > +                           struct vkms_crc_data *cursor_crc,
> > +                           struct vkms_crc_data *overlay_crc)
> >  {
> >       struct drm_framebuffer *fb = &primary_crc->fb;
> >       struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> > @@ -154,6 +175,8 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> >       memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> >       mutex_unlock(&vkms_obj->pages_lock);
> >
> > +     if (overlay_crc)
> > +             compose_overlay(overlay_crc, primary_crc, vaddr_out);
> >       if (cursor_crc)
> >               compose_cursor(cursor_crc, primary_crc, vaddr_out);
> >
> > @@ -184,6 +207,7 @@ void vkms_crc_work_handle(struct work_struct *work)
> >                                               output);
> >       struct vkms_crc_data *primary_crc = NULL;
> >       struct vkms_crc_data *cursor_crc = NULL;
> > +     struct vkms_crc_data *overlay_crc = NULL;
> >       struct drm_plane *plane;
> >       u32 crc32 = 0;
> >       u64 frame_start, frame_end;
> > @@ -208,14 +232,22 @@ void vkms_crc_work_handle(struct work_struct *work)
> >               if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> >                       continue;
> >
> > -             if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > +             switch (plane->type) {
> > +             case DRM_PLANE_TYPE_PRIMARY:
> >                       primary_crc = crc_data;
> > -             else
> > +                     break;
> > +             case DRM_PLANE_TYPE_CURSOR:
> >                       cursor_crc = crc_data;
> > +                     break;
> > +             case DRM_PLANE_TYPE_OVERLAY:
> > +                     overlay_crc = crc_data;
> > +                     break;
> > +             }
> >       }
> >
> > -     if (primary_crc)
> > -             crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> > +     if (primary_crc)  {
> > +             crc32 = _vkms_get_crc(primary_crc, cursor_crc, overlay_crc);
> > +     }
>
> Please, remember to use checkpatch in your patches before sending them.
>
> >       frame_end = drm_crtc_accurate_vblank_count(crtc);
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 738dd6206d85..b08ad6f95675 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -29,6 +29,10 @@ bool enable_cursor;
> >  module_param_named(enable_cursor, enable_cursor, bool, 0444);
> >  MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
> >
> > +bool enable_overlay;
> > +module_param_named(enable_overlay, enable_overlay, bool, 0444);
> > +MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
> > +
> >  static const struct file_operations vkms_driver_fops = {
> >       .owner          = THIS_MODULE,
> >       .open           = drm_open,
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 81f1cfbeb936..6861dbee01fd 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -20,6 +20,8 @@
> >
> >  extern bool enable_cursor;
> >
> > +extern bool enable_overlay;
> > +
> >  static const u32 vkms_formats[] = {
> >       DRM_FORMAT_XRGB8888,
> >  };
> > @@ -28,6 +30,10 @@ static const u32 vkms_cursor_formats[] = {
> >       DRM_FORMAT_ARGB8888,
> >  };
> >
> > +static const u32 vkms_overlay_formats[] = {
> > +     DRM_FORMAT_ARGB8888,
> > +};
> > +
> >  struct vkms_crc_data {
> >       struct drm_framebuffer fb;
> >       struct drm_rect src, dst;
> > @@ -118,6 +124,8 @@ int vkms_output_init(struct vkms_device *vkmsdev);
> >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> >                                 enum drm_plane_type type);
> >
> > +struct drm_plane *vkms_overlay_init(struct vkms_device *vkmsdev);
> > +
> >  /* Gem stuff */
> >  struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> >                                      struct drm_file *file,
> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > index 0e67d2d42f0c..090b21a2afab 100644
> > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > @@ -114,7 +114,7 @@ static int vkms_plane_atomic_check(struct drm_plane *plane,
> >       if (IS_ERR(crtc_state))
> >               return PTR_ERR(crtc_state);
> >
> > -     if (plane->type == DRM_PLANE_TYPE_CURSOR)
> > +     if ((plane->type == DRM_PLANE_TYPE_CURSOR) || (plane->type == DRM_PLANE_TYPE_OVERLAY))
> >               can_position = true;
> >
> >       ret = drm_atomic_helper_check_plane_state(state, crtc_state,
> > @@ -167,6 +167,40 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> >       .cleanup_fb             = vkms_cleanup_fb,
> >  };
> >
> > +struct drm_plane *vkms_overlay_init(struct vkms_device *vkmsdev)
> > +{
> > +     struct drm_device *dev = &vkmsdev->drm;
> > +     const struct drm_plane_helper_funcs *funcs;
> > +     struct drm_plane *overlay;
> > +     const u32 *formats;
> > +     int ret, nformats;
> > +     unsigned int blend_caps  = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> > +                             BIT(DRM_MODE_BLEND_PREMULTI);
> > +
> > +     overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
> > +     if (!overlay)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     formats = vkms_overlay_formats;
> > +     nformats = ARRAY_SIZE(vkms_overlay_formats);
> > +     funcs = &vkms_primary_helper_funcs;
> > +     drm_plane_helper_add(overlay, funcs);
> > +     drm_plane_create_alpha_property(overlay);
> > +     drm_plane_create_blend_mode_property(overlay, blend_caps);
> > +
> > +     ret = drm_universal_plane_init(dev, overlay, 0,
> > +                                     &vkms_plane_funcs,
> > +                                     formats, nformats,
> > +                                     NULL,
> > +                                     DRM_PLANE_TYPE_OVERLAY, NULL);
> > +     if (ret) {
> > +             kfree(overlay);
> > +             return ERR_PTR(ret);
> > +     }
> > +
>
> IMHO you don't need to create a new function just for overlays
> initialization, because we already have a function named
> vkms_plane_init() that centralizes the responsibility of initializing
> planes. It could be better if you extend vkms_plane_init() to make it
> handle overlays.
>
Used vkms_overlay_init() as a separate function as added few more
properties for overlay as alpha channel and alpha blending where as
these properties are not added for primary and cursor using
vkms_plane_init() . So to avoid breaking other parts of code, added it
separately.

> Additionally, I think you never initialized overlays in your patch. You
> have the initialization function, but no ones invoke it. Am I right or I
> missed something?
Thank you for pointing this out. I corrected this by invoking
vkms_overlay_init() with enable_overlay option in vkms_output_init()
as -
if (enable_overlay) {
        overlay = vkms_overlay_init(vkmsdev);
 }
And it works fine while loading driver using sudo modprobe vkms
enable_cursor=1  enable_overlay =1

> Best Regards
>
> > +     return overlay;
> > +}
> > +
> >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> >                                 enum drm_plane_type type)
> >  {
> > --
> > 2.17.1
> >
But the IGT test kms_plane.c fails with error :
[  504.028013] [drm:drm_crtc_add_crc_entry [drm]] *ERROR* Overflow of
CRC buffer, userspace reads too slow.
[  504.040961] [drm] failed to queue vkms_crc_work_handle

>
> --
> Rodrigo Siqueira
> https://siqueira.tech
> Graduate Student
> Department of Computer Science
> University of São Paulo

---
Mamta
Rodrigo Siqueira March 14, 2019, 1:57 p.m. UTC | #2
On 03/13, Mamta Shukla wrote:
> On Sun, Mar 10, 2019 at 9:00 PM Rodrigo Siqueira
> <rodrigosiqueiramelo@gmail.com> wrote:
> >
> > Hi,
> >
> > Thanks for your patch.
> >
> > You can see my comments inline.
> >
> > On 03/09, Mamta Shukla wrote:
> > > Add overlay plane support in vkms aligned with cursor and primary
> > > plane with module option 'enable_overlay' to enable/disable overlay
> > > plane while testing.
> > >
> > > This currently passes plane-position-covered-pipe-A-plane subtest
> > > from IGT kms_plane test.
> >
> > What the difference of running this test with and without your patch?
> > This test pass without your patch as well. Additionally, I briefly
> > looked at the test, and l could not get confident that
> > "plane-position-covered-pipe-A-plane" validate overlays. Did I miss
> > something?
> >
> > > Signed-off-by: Mamta Shukla <mamtashukla555@gmail.com>
> > > ---
> > > changes in v3:
> > > -Fix spelling mistake 'palne'->'plane'
> > > -Use switch case
> > > -Use logical operator '||' in place of '|'
> > >
> > > change in v2:
> > > -Fix warning: return makes pointer from integer without a cast using
> > >  ERR_PTR
> > >
> > >  drivers/gpu/drm/vkms/vkms_crc.c   | 42 +++++++++++++++++++++++++++----
> > >  drivers/gpu/drm/vkms/vkms_drv.c   |  4 +++
> > >  drivers/gpu/drm/vkms/vkms_drv.h   |  8 ++++++
> > >  drivers/gpu/drm/vkms/vkms_plane.c | 36 +++++++++++++++++++++++++-
> > >  4 files changed, 84 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > > index 4dd6c155363d..82bfee7c79b5 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > > @@ -109,6 +109,26 @@ static void blend(void *vaddr_dst, void *vaddr_src,
> > >       }
> > >  }
> > >
> > > +static void compose_overlay(struct vkms_crc_data *overlay_crc,
> > > +                         struct vkms_crc_data *primary_crc, void *vaddr_out)
> > > +{
> > > +     struct drm_gem_object *overlay_obj;
> > > +     struct vkms_gem_object  *overlay_vkms_obj;
> > > +
> > > +     overlay_obj = drm_gem_fb_get_obj(&overlay_crc->fb, 0);
> > > +     overlay_vkms_obj = drm_gem_to_vkms_gem(overlay_obj);
> > > +     mutex_lock(&overlay_vkms_obj->pages_lock);
> > > +     if (!overlay_vkms_obj->vaddr) {
> > > +             DRM_WARN("overlay plane vaddr is NULL");
> > > +             goto out;
> > > +     }
> > > +
> > > +     blend(vaddr_out, overlay_vkms_obj->vaddr, primary_crc, overlay_crc);
> > > +
> > > +out:
> > > +     mutex_unlock(&overlay_vkms_obj->pages_lock);
> > > +}
> > > +
> > >  static void compose_cursor(struct vkms_crc_data *cursor_crc,
> > >                          struct vkms_crc_data *primary_crc, void *vaddr_out)
> > >  {
> > > @@ -131,7 +151,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc,
> > >  }
> > >
> > >  static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> > > -                           struct vkms_crc_data *cursor_crc)
> > > +                           struct vkms_crc_data *cursor_crc,
> > > +                           struct vkms_crc_data *overlay_crc)
> > >  {
> > >       struct drm_framebuffer *fb = &primary_crc->fb;
> > >       struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> > > @@ -154,6 +175,8 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> > >       memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> > >       mutex_unlock(&vkms_obj->pages_lock);
> > >
> > > +     if (overlay_crc)
> > > +             compose_overlay(overlay_crc, primary_crc, vaddr_out);
> > >       if (cursor_crc)
> > >               compose_cursor(cursor_crc, primary_crc, vaddr_out);
> > >
> > > @@ -184,6 +207,7 @@ void vkms_crc_work_handle(struct work_struct *work)
> > >                                               output);
> > >       struct vkms_crc_data *primary_crc = NULL;
> > >       struct vkms_crc_data *cursor_crc = NULL;
> > > +     struct vkms_crc_data *overlay_crc = NULL;
> > >       struct drm_plane *plane;
> > >       u32 crc32 = 0;
> > >       u64 frame_start, frame_end;
> > > @@ -208,14 +232,22 @@ void vkms_crc_work_handle(struct work_struct *work)
> > >               if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> > >                       continue;
> > >
> > > -             if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > +             switch (plane->type) {
> > > +             case DRM_PLANE_TYPE_PRIMARY:
> > >                       primary_crc = crc_data;
> > > -             else
> > > +                     break;
> > > +             case DRM_PLANE_TYPE_CURSOR:
> > >                       cursor_crc = crc_data;
> > > +                     break;
> > > +             case DRM_PLANE_TYPE_OVERLAY:
> > > +                     overlay_crc = crc_data;
> > > +                     break;
> > > +             }
> > >       }
> > >
> > > -     if (primary_crc)
> > > -             crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> > > +     if (primary_crc)  {
> > > +             crc32 = _vkms_get_crc(primary_crc, cursor_crc, overlay_crc);
> > > +     }
> >
> > Please, remember to use checkpatch in your patches before sending them.
> >
> > >       frame_end = drm_crtc_accurate_vblank_count(crtc);
> > >
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > index 738dd6206d85..b08ad6f95675 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > @@ -29,6 +29,10 @@ bool enable_cursor;
> > >  module_param_named(enable_cursor, enable_cursor, bool, 0444);
> > >  MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
> > >
> > > +bool enable_overlay;
> > > +module_param_named(enable_overlay, enable_overlay, bool, 0444);
> > > +MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
> > > +
> > >  static const struct file_operations vkms_driver_fops = {
> > >       .owner          = THIS_MODULE,
> > >       .open           = drm_open,
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index 81f1cfbeb936..6861dbee01fd 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -20,6 +20,8 @@
> > >
> > >  extern bool enable_cursor;
> > >
> > > +extern bool enable_overlay;
> > > +
> > >  static const u32 vkms_formats[] = {
> > >       DRM_FORMAT_XRGB8888,
> > >  };
> > > @@ -28,6 +30,10 @@ static const u32 vkms_cursor_formats[] = {
> > >       DRM_FORMAT_ARGB8888,
> > >  };
> > >
> > > +static const u32 vkms_overlay_formats[] = {
> > > +     DRM_FORMAT_ARGB8888,
> > > +};
> > > +
> > >  struct vkms_crc_data {
> > >       struct drm_framebuffer fb;
> > >       struct drm_rect src, dst;
> > > @@ -118,6 +124,8 @@ int vkms_output_init(struct vkms_device *vkmsdev);
> > >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> > >                                 enum drm_plane_type type);
> > >
> > > +struct drm_plane *vkms_overlay_init(struct vkms_device *vkmsdev);
> > > +
> > >  /* Gem stuff */
> > >  struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> > >                                      struct drm_file *file,
> > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > index 0e67d2d42f0c..090b21a2afab 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > @@ -114,7 +114,7 @@ static int vkms_plane_atomic_check(struct drm_plane *plane,
> > >       if (IS_ERR(crtc_state))
> > >               return PTR_ERR(crtc_state);
> > >
> > > -     if (plane->type == DRM_PLANE_TYPE_CURSOR)
> > > +     if ((plane->type == DRM_PLANE_TYPE_CURSOR) || (plane->type == DRM_PLANE_TYPE_OVERLAY))
> > >               can_position = true;
> > >
> > >       ret = drm_atomic_helper_check_plane_state(state, crtc_state,
> > > @@ -167,6 +167,40 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> > >       .cleanup_fb             = vkms_cleanup_fb,
> > >  };
> > >
> > > +struct drm_plane *vkms_overlay_init(struct vkms_device *vkmsdev)
> > > +{
> > > +     struct drm_device *dev = &vkmsdev->drm;
> > > +     const struct drm_plane_helper_funcs *funcs;
> > > +     struct drm_plane *overlay;
> > > +     const u32 *formats;
> > > +     int ret, nformats;
> > > +     unsigned int blend_caps  = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> > > +                             BIT(DRM_MODE_BLEND_PREMULTI);
> > > +
> > > +     overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
> > > +     if (!overlay)
> > > +             return ERR_PTR(-ENOMEM);
> > > +
> > > +     formats = vkms_overlay_formats;
> > > +     nformats = ARRAY_SIZE(vkms_overlay_formats);
> > > +     funcs = &vkms_primary_helper_funcs;
> > > +     drm_plane_helper_add(overlay, funcs);
> > > +     drm_plane_create_alpha_property(overlay);
> > > +     drm_plane_create_blend_mode_property(overlay, blend_caps);
> > > +
> > > +     ret = drm_universal_plane_init(dev, overlay, 0,
> > > +                                     &vkms_plane_funcs,
> > > +                                     formats, nformats,
> > > +                                     NULL,
> > > +                                     DRM_PLANE_TYPE_OVERLAY, NULL);
> > > +     if (ret) {
> > > +             kfree(overlay);
> > > +             return ERR_PTR(ret);
> > > +     }
> > > +
> >
> > IMHO you don't need to create a new function just for overlays
> > initialization, because we already have a function named
> > vkms_plane_init() that centralizes the responsibility of initializing
> > planes. It could be better if you extend vkms_plane_init() to make it
> > handle overlays.
> >
> Used vkms_overlay_init() as a separate function as added few more
> properties for overlay as alpha channel and alpha blending where as
> these properties are not added for primary and cursor using
> vkms_plane_init() . So to avoid breaking other parts of code, added it
> separately.

So, here is the function that told you in the previous email: 

struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
				  enum drm_plane_type type)
{
	struct drm_device *dev = &vkmsdev->drm;
	const struct drm_plane_helper_funcs *funcs;
	struct drm_plane *plane;
	const u32 *formats;
	int ret, nformats;

	plane = kzalloc(sizeof(*plane), GFP_KERNEL);
	if (!plane)
		return ERR_PTR(-ENOMEM);

	if (type == DRM_PLANE_TYPE_CURSOR) {
		formats = vkms_cursor_formats;
		nformats = ARRAY_SIZE(vkms_cursor_formats);
		funcs = &vkms_primary_helper_funcs;
	} else {
		formats = vkms_formats;
		nformats = ARRAY_SIZE(vkms_formats);
		funcs = &vkms_primary_helper_funcs;
	}

	ret = drm_universal_plane_init(dev, plane, 0,
				       &vkms_plane_funcs,
				       formats, nformats,
				       NULL, type, NULL);
	if (ret) {
		kfree(plane);
		return ERR_PTR(ret);
	}

	drm_plane_helper_add(plane, funcs);

	return plane;
}

Why can you not change the ‘ifs’ condition by a switch/case and add the
overlay there? I did not get why you cannot expand this function. Could
you shed some light on this?

> > Additionally, I think you never initialized overlays in your patch. You
> > have the initialization function, but no ones invoke it. Am I right or I
> > missed something?
> Thank you for pointing this out. I corrected this by invoking
> vkms_overlay_init() with enable_overlay option in vkms_output_init()
> as -
> if (enable_overlay) {
>         overlay = vkms_overlay_init(vkmsdev);
>  }
> And it works fine while loading driver using sudo modprobe vkms
> enable_cursor=1  enable_overlay =1
> 
> > Best Regards
> >
> > > +     return overlay;
> > > +}
> > > +
> > >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> > >                                 enum drm_plane_type type)
> > >  {
> > > --
> > > 2.17.1
> > >
> But the IGT test kms_plane.c fails with error :
> [  504.028013] [drm:drm_crtc_add_crc_entry [drm]] *ERROR* Overflow of
> CRC buffer, userspace reads too slow.
> [  504.040961] [drm] failed to queue vkms_crc_work_handle

Try to apply the following patch, and test it again:

https://patchwork.freedesktop.org/series/57193/

Best Regards

> >
> > --
> > Rodrigo Siqueira
> > https://siqueira.tech
> > Graduate Student
> > Department of Computer Science
> > University of São Paulo
> 
> ---
> Mamta
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
index 4dd6c155363d..82bfee7c79b5 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_crc.c
@@ -109,6 +109,26 @@  static void blend(void *vaddr_dst, void *vaddr_src,
 	}
 }
 
+static void compose_overlay(struct vkms_crc_data *overlay_crc,
+			    struct vkms_crc_data *primary_crc, void *vaddr_out)
+{
+	struct drm_gem_object *overlay_obj;
+	struct vkms_gem_object  *overlay_vkms_obj;
+
+	overlay_obj = drm_gem_fb_get_obj(&overlay_crc->fb, 0);
+	overlay_vkms_obj = drm_gem_to_vkms_gem(overlay_obj);
+	mutex_lock(&overlay_vkms_obj->pages_lock);
+	if (!overlay_vkms_obj->vaddr) {
+		DRM_WARN("overlay plane vaddr is NULL");
+		goto out;
+	}
+
+	blend(vaddr_out, overlay_vkms_obj->vaddr, primary_crc, overlay_crc);
+
+out:
+	mutex_unlock(&overlay_vkms_obj->pages_lock);
+}
+
 static void compose_cursor(struct vkms_crc_data *cursor_crc,
 			   struct vkms_crc_data *primary_crc, void *vaddr_out)
 {
@@ -131,7 +151,8 @@  static void compose_cursor(struct vkms_crc_data *cursor_crc,
 }
 
 static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
-			      struct vkms_crc_data *cursor_crc)
+			      struct vkms_crc_data *cursor_crc,
+			      struct vkms_crc_data *overlay_crc)
 {
 	struct drm_framebuffer *fb = &primary_crc->fb;
 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
@@ -154,6 +175,8 @@  static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
 	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
 	mutex_unlock(&vkms_obj->pages_lock);
 
+	if (overlay_crc)
+		compose_overlay(overlay_crc, primary_crc, vaddr_out);
 	if (cursor_crc)
 		compose_cursor(cursor_crc, primary_crc, vaddr_out);
 
@@ -184,6 +207,7 @@  void vkms_crc_work_handle(struct work_struct *work)
 						output);
 	struct vkms_crc_data *primary_crc = NULL;
 	struct vkms_crc_data *cursor_crc = NULL;
+	struct vkms_crc_data *overlay_crc = NULL;
 	struct drm_plane *plane;
 	u32 crc32 = 0;
 	u64 frame_start, frame_end;
@@ -208,14 +232,22 @@  void vkms_crc_work_handle(struct work_struct *work)
 		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
 			continue;
 
-		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+		switch (plane->type) {
+		case DRM_PLANE_TYPE_PRIMARY:
 			primary_crc = crc_data;
-		else
+			break;
+		case DRM_PLANE_TYPE_CURSOR:
 			cursor_crc = crc_data;
+			break;
+		case DRM_PLANE_TYPE_OVERLAY:
+			overlay_crc = crc_data;
+			break;
+		}
 	}
 
-	if (primary_crc)
-		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
+	if (primary_crc)  {
+		crc32 = _vkms_get_crc(primary_crc, cursor_crc, overlay_crc);
+	}
 
 	frame_end = drm_crtc_accurate_vblank_count(crtc);
 
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 738dd6206d85..b08ad6f95675 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -29,6 +29,10 @@  bool enable_cursor;
 module_param_named(enable_cursor, enable_cursor, bool, 0444);
 MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
 
+bool enable_overlay;
+module_param_named(enable_overlay, enable_overlay, bool, 0444);
+MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
+
 static const struct file_operations vkms_driver_fops = {
 	.owner		= THIS_MODULE,
 	.open		= drm_open,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 81f1cfbeb936..6861dbee01fd 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -20,6 +20,8 @@ 
 
 extern bool enable_cursor;
 
+extern bool enable_overlay;
+
 static const u32 vkms_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
@@ -28,6 +30,10 @@  static const u32 vkms_cursor_formats[] = {
 	DRM_FORMAT_ARGB8888,
 };
 
+static const u32 vkms_overlay_formats[] = {
+	DRM_FORMAT_ARGB8888,
+};
+
 struct vkms_crc_data {
 	struct drm_framebuffer fb;
 	struct drm_rect src, dst;
@@ -118,6 +124,8 @@  int vkms_output_init(struct vkms_device *vkmsdev);
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 				  enum drm_plane_type type);
 
+struct drm_plane *vkms_overlay_init(struct vkms_device *vkmsdev);
+
 /* Gem stuff */
 struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
 				       struct drm_file *file,
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 0e67d2d42f0c..090b21a2afab 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -114,7 +114,7 @@  static int vkms_plane_atomic_check(struct drm_plane *plane,
 	if (IS_ERR(crtc_state))
 		return PTR_ERR(crtc_state);
 
-	if (plane->type == DRM_PLANE_TYPE_CURSOR)
+	if ((plane->type == DRM_PLANE_TYPE_CURSOR) || (plane->type == DRM_PLANE_TYPE_OVERLAY))
 		can_position = true;
 
 	ret = drm_atomic_helper_check_plane_state(state, crtc_state,
@@ -167,6 +167,40 @@  static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
 	.cleanup_fb		= vkms_cleanup_fb,
 };
 
+struct drm_plane *vkms_overlay_init(struct vkms_device *vkmsdev)
+{
+	struct drm_device *dev = &vkmsdev->drm;
+	const struct drm_plane_helper_funcs *funcs;
+	struct drm_plane *overlay;
+	const u32 *formats;
+	int ret, nformats;
+	unsigned int blend_caps  = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
+				BIT(DRM_MODE_BLEND_PREMULTI);
+
+	overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
+	if (!overlay)
+		return ERR_PTR(-ENOMEM);
+
+	formats = vkms_overlay_formats;
+	nformats = ARRAY_SIZE(vkms_overlay_formats);
+	funcs = &vkms_primary_helper_funcs;
+	drm_plane_helper_add(overlay, funcs);
+	drm_plane_create_alpha_property(overlay);
+	drm_plane_create_blend_mode_property(overlay, blend_caps);
+
+	ret = drm_universal_plane_init(dev, overlay, 0,
+					&vkms_plane_funcs,
+					formats, nformats,
+					NULL,
+					DRM_PLANE_TYPE_OVERLAY, NULL);
+	if (ret) {
+		kfree(overlay);
+		return ERR_PTR(ret);
+	}
+
+	return overlay;
+}
+
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 				  enum drm_plane_type type)
 {