Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752068AbdGFIeb (ORCPT ); Thu, 6 Jul 2017 04:34:31 -0400 Received: from mail-lf0-f66.google.com ([209.85.215.66]:33463 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750862AbdGFIe2 (ORCPT ); Thu, 6 Jul 2017 04:34:28 -0400 Date: Thu, 6 Jul 2017 10:34:13 +0200 From: Daniel Vetter To: Peter Rosin Cc: linux-kernel@vger.kernel.org, Boris Brezillon , dri-devel@lists.freedesktop.org, Daniel Vetter Subject: Re: [PATCH v3 05/16] drm/fb-helper: do a generic fb_setcmap helper in terms of crtc .gamma_set Message-ID: <20170706083413.ihujro5p7qlazg3a@phenom.ffwll.local> Mail-Followup-To: Peter Rosin , linux-kernel@vger.kernel.org, Boris Brezillon , dri-devel@lists.freedesktop.org, Daniel Vetter References: <1499164632-5582-1-git-send-email-peda@axentia.se> <1499164632-5582-6-git-send-email-peda@axentia.se> <20170705062144.bdyngsas4uc5dj2d@phenom.ffwll.local> <3cbc02dc-e535-7d66-923b-c9a4d0f14f13@axentia.se> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3cbc02dc-e535-7d66-923b-c9a4d0f14f13@axentia.se> X-Operating-System: Linux phenom 4.11.0-1-amd64 User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1319 Lines: 33 > >> @@ -1369,27 +1362,57 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > >> memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g)); > >> memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b)); > >> > >> - for (j = 0; j < cmap->len; j++) { > >> - u16 hred, hgreen, hblue, htransp = 0xffff; > >> + ret = crtc->funcs->gamma_set(crtc, r, g, b, > >> + crtc->gamma_size, crtc_state); > > > > I guess my description of what I have in mind wasn't really clear. I think > > a proper atomic commit should never reuse one of the old hooks > > (->gamma_set) here, that's just confusing. Instead what I had in mind is > > to do the proper adjusting that gamma_set does here in this function, i.e. > > > > - create the new blob, fill it with the cmap data > > > > - assign that blob to the crtc state: > > > > drm_atomic_replace_property_blob(&crtc_state->gamma_lut, > > new_table, &temp); > > That function is static, and... Missed these comments here. I think we should just make them unstatic, that might also explain why gamma_set is going this indirect path. Means we need some kerneldoc, but that's not much work. Ping the original author if you have questions. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch