Re: [PATCH v2 08/12] drm/fb-helper: Move out commit code

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 17, 2019 at 07:56:20PM +0200, Noralf Trønnes wrote:
> 
> 
> Den 16.04.2019 10.38, skrev Daniel Vetter:
> > On Sun, Apr 07, 2019 at 06:52:39PM +0200, Noralf Trønnes wrote:
> >> Move the modeset commit code to drm_client_modeset.
> >> No changes except exporting API.
> >>
> >> v2: Move to drm_client_modeset.c instead of drm_client.c
> >>
> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >> ---
> >>  drivers/gpu/drm/drm_client_modeset.c | 287 +++++++++++++++++++++++++++
> 
> <snip>
> 
> >> -/**
> >> - * drm_client_panel_rotation() - Check panel orientation
> >> - * @modeset: DRM modeset
> >> - * @rotation: Returned rotation value
> >> - *
> >> - * This function checks if the primary plane in @modeset can hw rotate to match
> >> - * the panel orientation on its connector.
> >> - *
> >> - * Note: Currently only 0 and 180 degrees are supported.
> >> - *
> >> - * Return:
> >> - * True if the plane can do the rotation, false otherwise.
> >> - */
> >> -bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
> > 
> > Why not static? Doesn't seem to be used by anything outside of
> > drm_client_modeset.c.
> > 
> 
> It is used in drm_fb_helper.c:drm_setup_crtcs_fb() to set up any
> rotation and do fbcon sw rotation if necessary. Clients that support
> rotation need to call it.
> 
> >> -{
> >> -	struct drm_connector *connector = modeset->connectors[0];
> >> -	struct drm_plane *plane = modeset->crtc->primary;
> >> -	u64 valid_mask = 0;
> >> -	unsigned int i;
> >> -
> >> -	if (!modeset->num_connectors)
> >> -		return false;
> >> -
> >> -	switch (connector->display_info.panel_orientation) {
> >> -	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
> >> -		*rotation = DRM_MODE_ROTATE_180;
> >> -		break;
> >> -	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
> >> -		*rotation = DRM_MODE_ROTATE_90;
> >> -		break;
> >> -	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
> >> -		*rotation = DRM_MODE_ROTATE_270;
> >> -		break;
> >> -	default:
> >> -		*rotation = DRM_MODE_ROTATE_0;
> >> -	}
> >> -
> >> -	/*
> >> -	 * TODO: support 90 / 270 degree hardware rotation,
> >> -	 * depending on the hardware this may require the framebuffer
> >> -	 * to be in a specific tiling format.
> >> -	 */
> >> -	if (*rotation != DRM_MODE_ROTATE_180 || !plane->rotation_property)
> >> -		return false;
> >> -
> >> -	for (i = 0; i < plane->rotation_property->num_values; i++)
> >> -		valid_mask |= (1ULL << plane->rotation_property->values[i]);
> >> -
> >> -	if (!(*rotation & valid_mask))
> >> -		return false;
> >> -
> >> -	return true;
> >> -}
> >> -
> 
> <snip>
> 
> >> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> >> index 858c8be70870..64b725b318f2 100644
> >> --- a/include/drm/drm_client.h
> >> +++ b/include/drm/drm_client.h
> >> @@ -154,6 +154,10 @@ int drm_client_modeset_create(struct drm_client_dev *client);
> >>  void drm_client_modeset_free(struct drm_client_dev *client);
> >>  void drm_client_modeset_release(struct drm_client_dev *client);
> >>  struct drm_mode_set *drm_client_find_modeset(struct drm_client_dev *client, struct drm_crtc *crtc);
> >> +bool drm_client_panel_rotation(struct drm_mode_set *modeset, unsigned int *rotation);
> >> +int drm_client_modeset_commit_force(struct drm_client_dev *client);
> > 
> > I think latest here the _force postfix stopped making sense. It's just a
> > commit. Also I'm wondering whether we shouldn't pull the
> > master_acquire_internal into these helpers here, there's not really a
> > use-case I can think of where we should not check for other masters.
> > 
> 
> drm_master_internal_acquire() is used in various places in drm_fb_helper
> for functions that doesn't make sense to move to drm_client, like:
> - drm_fb_helper_setcmap
> - drm_fb_helper_ioctl
> - drm_fb_helper_pan_display

See discussion on the earlier patches, I completely backtracked on this
after better understanding why we need _force.

And exporting/using master_acquire_internal by drm_clients makes total
sense to me.
-Daniel

> 
> Noralf.
> 
> > Only two kernel modeset requests want to ignore master status:
> > - debug enter/leave, which is utterly broken by design (and outright
> >   disable for any atomic driver)
> > - panic handling, for which we now have a really nice plan, plus first
> >   sketches of an implementation.
> > 
> > Cheers, Daniel
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx