* Re: [PATCH] drm/bochs: Fix NULL dereference on atomic_disable helper [not found] <20190310212150.xhhb4abzm7j3ain2@smtp.gmail.com> @ 2019-03-11 6:22 ` Gerd Hoffmann [not found] ` <20190311062200.4mtel3j5fjxtxrcf@sirius.home.kraxel.org> 1 sibling, 0 replies; 5+ messages in thread From: Gerd Hoffmann @ 2019-03-11 6:22 UTC (permalink / raw) To: Rodrigo Siqueira Cc: David Airlie, linux-kernel, dri-devel, Daniel Vetter, virtualization On Sun, Mar 10, 2019 at 06:21:50PM -0300, Rodrigo Siqueira wrote: > When the subtest basic-plain-flip from IGT project was executed on Bochs > driver, the following bug appears: > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000000 > ? drm_atomic_helper_commit_modeset_disables+0x396/0x3e0 [drm_kms_helper] > +static void bochs_crtc_atomic_disable(struct drm_crtc *crtc, > + struct drm_crtc_state *old_state) > +{ > +} > + > static void bochs_crtc_atomic_flush(struct drm_crtc *crtc, > struct drm_crtc_state *old_crtc_state) > { > @@ -66,6 +71,7 @@ static const struct drm_crtc_funcs bochs_crtc_funcs = { > static const struct drm_crtc_helper_funcs bochs_helper_funcs = { > .mode_set_nofb = bochs_crtc_mode_set_nofb, > .atomic_enable = bochs_crtc_atomic_enable, > + .atomic_disable = bochs_crtc_atomic_disable, Shouldn't we make the callback optional instead of adding empty dummy functions to drivers? cheers, Gerd ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20190311062200.4mtel3j5fjxtxrcf@sirius.home.kraxel.org>]
[parent not found: <20190311101159.t6mbzm5sdwuh5muc@smtp.gmail.com>]
* Re: [PATCH] drm/bochs: Fix NULL dereference on atomic_disable helper [not found] ` <20190311101159.t6mbzm5sdwuh5muc@smtp.gmail.com> @ 2019-03-11 13:07 ` Gerd Hoffmann [not found] ` <20190311130716.6hjqci43clic5lxx@sirius.home.kraxel.org> 1 sibling, 0 replies; 5+ messages in thread From: Gerd Hoffmann @ 2019-03-11 13:07 UTC (permalink / raw) To: Rodrigo Siqueira Cc: David Airlie, linux-kernel, dri-devel, Daniel Vetter, virtualization Hi, > > > static void bochs_crtc_atomic_flush(struct drm_crtc *crtc, > > > struct drm_crtc_state *old_crtc_state) > > > { > > > @@ -66,6 +71,7 @@ static const struct drm_crtc_funcs bochs_crtc_funcs = { > > > static const struct drm_crtc_helper_funcs bochs_helper_funcs = { > > > .mode_set_nofb = bochs_crtc_mode_set_nofb, > > > .atomic_enable = bochs_crtc_atomic_enable, > > > + .atomic_disable = bochs_crtc_atomic_disable, > > > > Shouldn't we make the callback optional instead of adding empty dummy > > functions to drivers? > > Hi Gerd, > > I agree, and I can work in this issue. > Just one question, should we make atomic_enable optional as well? IIRC the drm code checks for the atomic_enable callback presence to figure whenever it should take the atomic or legacy code paths. So, I think that will not work. cheers, Gerd ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20190311130716.6hjqci43clic5lxx@sirius.home.kraxel.org>]
* Re: [PATCH] drm/bochs: Fix NULL dereference on atomic_disable helper [not found] ` <20190311130716.6hjqci43clic5lxx@sirius.home.kraxel.org> @ 2019-03-11 13:23 ` Daniel Vetter 2019-03-11 13:39 ` Gerd Hoffmann [not found] ` <20190311133919.azyhtioxf23duf3g@sirius.home.kraxel.org> 0 siblings, 2 replies; 5+ messages in thread From: Daniel Vetter @ 2019-03-11 13:23 UTC (permalink / raw) To: Gerd Hoffmann Cc: Rodrigo Siqueira, David Airlie, linux-kernel, dri-devel, virtualization, Daniel Vetter On Mon, Mar 11, 2019 at 02:07:16PM +0100, Gerd Hoffmann wrote: > Hi, > > > > > static void bochs_crtc_atomic_flush(struct drm_crtc *crtc, > > > > struct drm_crtc_state *old_crtc_state) > > > > { > > > > @@ -66,6 +71,7 @@ static const struct drm_crtc_funcs bochs_crtc_funcs = { > > > > static const struct drm_crtc_helper_funcs bochs_helper_funcs = { > > > > .mode_set_nofb = bochs_crtc_mode_set_nofb, > > > > .atomic_enable = bochs_crtc_atomic_enable, > > > > + .atomic_disable = bochs_crtc_atomic_disable, > > > > > > Shouldn't we make the callback optional instead of adding empty dummy > > > functions to drivers? > > > > Hi Gerd, > > > > I agree, and I can work in this issue. > > Just one question, should we make atomic_enable optional as well? > > IIRC the drm code checks for the atomic_enable callback presence to > figure whenever it should take the atomic or legacy code paths. It should check for drm_driver->mode_config.funcs.atomic_commit for that, see drm_drv_uses_atomic_modeset(). Anything else should be a bug. Or do you mean the fallback to the old crtc helper prepare/commit callbacks? We'd need to make all of them optional ofc, with atomic_ variants being preferred ofc. -Daniel > > So, I think that will not work. > > cheers, > Gerd > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/bochs: Fix NULL dereference on atomic_disable helper 2019-03-11 13:23 ` Daniel Vetter @ 2019-03-11 13:39 ` Gerd Hoffmann [not found] ` <20190311133919.azyhtioxf23duf3g@sirius.home.kraxel.org> 1 sibling, 0 replies; 5+ messages in thread From: Gerd Hoffmann @ 2019-03-11 13:39 UTC (permalink / raw) To: Rodrigo Siqueira, David Airlie, virtualization, dri-devel, linux-kernel Hi, > > IIRC the drm code checks for the atomic_enable callback presence to > > figure whenever it should take the atomic or legacy code paths. > > It should check for drm_driver->mode_config.funcs.atomic_commit for that, > see drm_drv_uses_atomic_modeset(). Anything else should be a bug. > > Or do you mean the fallback to the old crtc helper prepare/commit > callbacks? Probably the later. There was some reason why I've left in the empty bochs_crtc_atomic_enable() callback ... cheers, Gerd ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20190311133919.azyhtioxf23duf3g@sirius.home.kraxel.org>]
[parent not found: <20190311174958.xdw4lrvf6ydeplvc@smtp.gmail.com>]
* Re: [PATCH] drm/bochs: Fix NULL dereference on atomic_disable helper [not found] ` <20190311174958.xdw4lrvf6ydeplvc@smtp.gmail.com> @ 2019-03-11 18:48 ` Daniel Vetter 0 siblings, 0 replies; 5+ messages in thread From: Daniel Vetter @ 2019-03-11 18:48 UTC (permalink / raw) To: Rodrigo Siqueira; +Cc: David Airlie, virtualization, dri-devel, linux-kernel On Mon, Mar 11, 2019 at 02:49:58PM -0300, Rodrigo Siqueira wrote: > On 03/11, Gerd Hoffmann wrote: > > Hi, > > > > > > IIRC the drm code checks for the atomic_enable callback presence to > > > > figure whenever it should take the atomic or legacy code paths. > > > > > > It should check for drm_driver->mode_config.funcs.atomic_commit for that, > > > see drm_drv_uses_atomic_modeset(). Anything else should be a bug. > > > > > > Or do you mean the fallback to the old crtc helper prepare/commit > > > callbacks? > > > > Probably the later. There was some reason why I've left in the empty > > bochs_crtc_atomic_enable() callback ... > > Just for checking before I start to work in this patch: > The correct solution should be made atomic_enable and atomic_disable > optional, right? I should do it, and check if Bochs driver really needs > bochs_crtc_atomic_enable after my change, right? Yup. I just tried to remember why we haven't done this yet, but I think that was a patch to make crtc->helper_funcs optional. And that doesn't make sense imo, since if your crtc doesn't do anything then you don't really have an atomic driver :-) And if there's ever a legit use case for this, then that drive probably shouldn't use the atomic helpers ... But making crtc_helper_funcs->atomic_enable/disable optional sounds like a good idea. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-11 18:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20190310212150.xhhb4abzm7j3ain2@smtp.gmail.com>
2019-03-11 6:22 ` [PATCH] drm/bochs: Fix NULL dereference on atomic_disable helper Gerd Hoffmann
[not found] ` <20190311062200.4mtel3j5fjxtxrcf@sirius.home.kraxel.org>
[not found] ` <20190311101159.t6mbzm5sdwuh5muc@smtp.gmail.com>
2019-03-11 13:07 ` Gerd Hoffmann
[not found] ` <20190311130716.6hjqci43clic5lxx@sirius.home.kraxel.org>
2019-03-11 13:23 ` Daniel Vetter
2019-03-11 13:39 ` Gerd Hoffmann
[not found] ` <20190311133919.azyhtioxf23duf3g@sirius.home.kraxel.org>
[not found] ` <20190311174958.xdw4lrvf6ydeplvc@smtp.gmail.com>
2019-03-11 18:48 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox