Hi Sam,
[looks like Thundebird decided to throw away my reply, so I'll try again]
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.
>> @@ -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.
Thanks for looking at this, I'll spin a new version.
Noralf.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
