Re: [PATCH v5 05/11] drm/fb-helper: Remove drm_fb_helper_crtc

[Sam Ravnborg <sam@ravnborg.org>]

Hi Noralf.

On Wed, May 15, 2019 at 04:51:04PM +0200, Noralf Trønnes wrote:
> Hi Sam,
> 
> [looks like Thundebird decided to throw away my reply, so I'll try again]
I guess it awaits moderator approval as the from address was different
from what you otherwise used - I got both mails.

> 
> Den 15.05.2019 11.04, skrev Sam Ravnborg:
> > Hi Noralf.
> > 
> > I have read through the cahnes a copuple of times not and feel confident
> > to add my r-b if the comments are considered.
> > 
> > On Mon, May 06, 2019 at 08:01:33PM +0200, Noralf Trønnes wrote:
> >> It now only contains the modeset so use that directly instead and attach
> >> a modeset array to drm_client_dev. drm_fb_helper will use this array.
> >> Code will later be moved to drm_client, so add code there in a new file
> >> drm_client_modeset.c with MIT license to match drm_fb_helper.c.
> > 
> > The first part of this commit log could use some re-pharsing.
> > What is "It" etc.
> > 
> 
> I could do this:
> 
> struct drm_fb_helper_crtc is now just a wrapper around drm_mode_set so
> use that directly instead and attach it as a modeset array onto
> drm_client_dev. drm_fb_helper will use this array to store its modesets
> which means it will always initialize a drm_client, but it will not
> register the client (callbacks) unless it's the generic fbdev emulation.
This is much better - thanks.

> 
> >> @@ -532,8 +535,7 @@ static int restore_fbdev_mode_legacy(struct drm_fb_helper *fb_helper)
> >>  						    DRM_MODE_ROTATE_0);
> >>  	}
> >>  
> >> -	for (i = 0; i < fb_helper->crtc_count; i++) {
> >> -		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
> >> +	drm_client_for_each_modeset(mode_set, client) {
> >>  		struct drm_crtc *crtc = mode_set->crtc;
> >>  
> >>  		if (crtc->funcs->cursor_set2) {
> > This function requires modeset_mutex to be held. Maybe add comment?
> > 
> 
> drm_client_for_each_modeset() has a lockdep warn (courtesy of Daniel
> Vetter):
> 
> #define drm_client_for_each_modeset(modeset, client) \
> 	for (({ lockdep_assert_held(&(client)->modeset_mutex); }), \
> 	     modeset = (client)->modesets; modeset->crtc; modeset++)
> 
> >> @@ -1842,7 +1805,7 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
> >>  
> >>  	pan_set(fb_helper, var->xoffset, var->yoffset);
> >>  
> >> -	ret = restore_fbdev_mode_atomic(fb_helper, true);
> >> +	ret = restore_fbdev_mode_force(fb_helper);
> > This change looks alien compared to other changes.
> > Does it belong to this patchset?
> > 
> 
> It's mentioned in the commit message:
> 
> In pan_display_atomic() restore_fbdev_mode_force() is used instead of
> restore_fbdev_mode_atomic() because that one will later become internal
> to drm_client_modeset.
Right, I forgot about this detail from the commit log.

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel