Re: [PATCH V3 0/3] Expanding the basic vkms features

[Haneen Mohammed <hamohammed.sa@gmail.com>]

On Wed, May 23, 2018 at 11:15:18AM +0200, Daniel Vetter wrote:
> On Mon, May 21, 2018 at 10:04:23PM -0300, Rodrigo Siqueira wrote:
> > This series of patches add a centralized initialization mechanism, a
> > single CRTC with a plane, an encoder, and extra module information. 
> > 
> > Changes in v2:
> >  - Remove unused definitions
> >  - Improve file names
> >  - Improve code separation
> >  - Remove unnecessary formats
> 
> Oops, I merged v2 already. I think we need follow-up patches.
>  
> > Changes in v3:
> >  - Adds drm_crtc_helper_funcs with a simple atomic_check
> 
> Sorry for the late comment on v2, but I think we need to figure out why
> this goes boom first. It imo shouldn't.
> 
> >  - Adds extra hooks for drm_connector_funcs hooks (reset,
> >    atomic_duplicate_state, atomic_destroy_state)
> 
> Hm, reset shouldn't be required. Why do you need it?
> 

I've checked the code, and I think the memory for [plane/connector/crtc]
states that are used later in duplicate/destroy are allocated in drm_atomic_helper_*_reset. 
Otherwise checking /sys/kernel/debug/dri/0/state or running the igt
tests causes null pointer dereference error.

> Wrt duplicate/destroy state, those are mandatory. I think it'd be good to
> have checks for those in the drm_*_init functions, but only for atomic
> drivers. You can use drm_drv_uses_atomic_modeset() and WARN_ON(). There's
> a bunch of examples already for checking for this stuff, see e.g.
> dma_fence_init().
>
> >  - Adds drm_connector_helper_funcs
> >  - Adds drm_plane_helper_funcs
> 
> Same here, would be good to add WARN_ON to the relevant _init() functions
> to make sure all the mandatory stuff is there to begin with.
> 
> Since Rodrigo has typed the fixes to vkms already, could you Haneen look
> into adding these checks to the core drm core?
>

Sure, I'll work on that.

> Thanks, Daniel
> 
> >  - Changes in the commit messages
> > 
> > Rodrigo Siqueira (3):
> >   drm/vkms: Add mode_config initialization
> >   drm/vkms: Add basic CRTC initialization
> >   drm/vkms: Add extra information about vkms
> > 
> >  drivers/gpu/drm/Kconfig            |   8 +-
> >  drivers/gpu/drm/vkms/Makefile      |   2 +-
> >  drivers/gpu/drm/vkms/vkms_crtc.c   |  47 ++++++++++++
> >  drivers/gpu/drm/vkms/vkms_drv.c    |  55 +++++---------
> >  drivers/gpu/drm/vkms/vkms_drv.h    |  33 +++++++-
> >  drivers/gpu/drm/vkms/vkms_output.c | 118 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/vkms/vkms_plane.c  |  62 +++++++++++++++
> >  7 files changed, 285 insertions(+), 40 deletions(-)
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_crtc.c
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_output.c
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_plane.c
> > 
> > -- 
> > 2.17.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel