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

["Noralf Trønnes" <noralf@tronnes.org>]

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