[v3,0/7] drm/ast: Fix modesetting's framebuffer usage

Message

Thomas Zimmermann Dec. 2, 2019, 11:15 a.m. UTC

(was: drm/ast: Fix potential NULL-pointer read during atomic mode setting)

The CRTC's modesetting code in ast requires the format of the primary
plane's framebuffer. There's currently a segmentation fault if the
framebuffer is set to NULL. This happens when disabling the display
during suspend.

Patch 1 moves the modesetting code behind a test for the framebuffer
in atomic_flush(). It returns if the framebuffer is NULL. This fixes
the segmentation fault in a simple and back-portable way.

The rest of the patchset addresses problems in the code where access
between plane and CRTC state is convoluted and hard to follow. With
the changes applied, the atomic_check() functions of primary plane and
CRTC set the necessary state that is later used by atomic_flush().

Configurations without framebuffer cannot be rejected because this
breaks drm_framebuffer_remove(). Instead accept them and switch off
the screen to avoid garbage output. Patch 2 implements this logic for
the primary plane.

Patches 3 to 5 implement atomic_check() for planes and introduce
struct ast_crtc_state. In patches 6 and 7, the atomic_check() functions
of CRTC and primary plane set display mode and framebuffer format in
the AST CRTC state; and atomic_flush() picks up these settings for
configuring the mode.

The patchset has been tested by running the fbdev console, Gnome,
and Weston on an AST1100 chipset. Gnome's display has been suspended
multiple times to verify the bugfix.

v3:
	* start anew with a patch series
	* put bugfix into atomic_flush()
	* cleanup mode and framebuffer references in modesetting
v2:
	* do an atomic check for plane
	* reject invisible primary planes

Thomas Zimmermann (7):
  drm/ast: Move modesetting code to CRTC's atomic_flush()
  drm/ast: Enable and disable screen in primary-plane functions
  drm/ast: Clean up arguments of register functions
  drm/ast: Add plane atomic_check() functions
  drm/ast: Introduce struct ast_crtc_state
  drm/ast: Store VBIOS mode info in struct ast_crtc_state
  drm/ast: Store primary-plane format in struct ast_crtc_state

 drivers/gpu/drm/ast/ast_drv.h  |  11 ++
 drivers/gpu/drm/ast/ast_mode.c | 276 ++++++++++++++++++++-------------
 2 files changed, 176 insertions(+), 111 deletions(-)

--
2.23.0

Comments

Gerd Hoffmann Dec. 5, 2019, 8:10 a.m. UTC | #1

On Mon, Dec 02, 2019 at 12:15:50PM +0100, Thomas Zimmermann wrote:
> (was: drm/ast: Fix potential NULL-pointer read during atomic mode setting)
> 
> The CRTC's modesetting code in ast requires the format of the primary
> plane's framebuffer. There's currently a segmentation fault if the
> framebuffer is set to NULL. This happens when disabling the display
> during suspend.
> 
> Patch 1 moves the modesetting code behind a test for the framebuffer
> in atomic_flush(). It returns if the framebuffer is NULL. This fixes
> the segmentation fault in a simple and back-portable way.
> 
> The rest of the patchset addresses problems in the code where access
> between plane and CRTC state is convoluted and hard to follow. With
> the changes applied, the atomic_check() functions of primary plane and
> CRTC set the necessary state that is later used by atomic_flush().
> 
> Configurations without framebuffer cannot be rejected because this
> breaks drm_framebuffer_remove(). Instead accept them and switch off
> the screen to avoid garbage output. Patch 2 implements this logic for
> the primary plane.
> 
> Patches 3 to 5 implement atomic_check() for planes and introduce
> struct ast_crtc_state. In patches 6 and 7, the atomic_check() functions
> of CRTC and primary plane set display mode and framebuffer format in
> the AST CRTC state; and atomic_flush() picks up these settings for
> configuring the mode.
> 
> The patchset has been tested by running the fbdev console, Gnome,
> and Weston on an AST1100 chipset. Gnome's display has been suspended
> multiple times to verify the bugfix.

Disclaimer: Don't know the hardware.
Can't spot any issues in the series, so
Acked-by: Gerd Hoffmann <[e-mail]>

cheers,
  Gerd