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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
