From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH v11 3/4] drm/i915: Use new CRC debugfs API Date: Wed, 16 Nov 2016 16:08:30 +0200 Message-ID: <87eg2b1pr5.fsf@intel.com> References: <1475767268-14379-1-git-send-email-tomeu.vizoso@collabora.com> <1475767268-14379-4-git-send-email-tomeu.vizoso@collabora.com> <87inrq2vee.fsf@intel.com> <20161115071609.GI8202@suiko.acc.umu.se> <87a8d12lmy.fsf@intel.com> <87h9771t0k.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Tomeu Vizoso Cc: David Airlie , Intel Graphics Development , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , Thierry Reding , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org T24gV2VkLCAxNiBOb3YgMjAxNiwgVG9tZXUgVml6b3NvIDx0b21ldS52aXpvc29AY29sbGFib3Jh LmNvbT4gd3JvdGU6Cj4gT24gMTYgTm92ZW1iZXIgMjAxNiBhdCAxMzo1OCwgSmFuaSBOaWt1bGEg PGphbmkubmlrdWxhQGxpbnV4LmludGVsLmNvbT4gd3JvdGU6Cj4+IE9uIFdlZCwgMTYgTm92IDIw MTYsIFRvbWV1IFZpem9zbyA8dG9tZXUudml6b3NvQGNvbGxhYm9yYS5jb20+IHdyb3RlOgo+Pj4g T24gMTUgTm92ZW1iZXIgMjAxNiBhdCAwOToyNywgSmFuaSBOaWt1bGEgPGphbmkubmlrdWxhQGxp bnV4LmludGVsLmNvbT4gd3JvdGU6Cj4+Pj4gT24gVHVlLCAxNSBOb3YgMjAxNiwgRGF2aWQgV2Vp bmVoYWxsIDx0YW9Aa2VybmVsLm9yZz4gd3JvdGU6Cj4+Pj4+IE9uIE1vbiwgTm92IDE0LCAyMDE2 IGF0IDEyOjQ0OjI1UE0gKzAyMDAsIEphbmkgTmlrdWxhIHdyb3RlOgo+Pj4+Pj4gT24gVGh1LCAw NiBPY3QgMjAxNiwgVG9tZXUgVml6b3NvIDx0b21ldS52aXpvc29AY29sbGFib3JhLmNvbT4gd3Jv dGU6Cj4+Pj4+PiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kaXNw bGF5LmMgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kaXNwbGF5LmMKPj4+Pj4+ID4gaW5k ZXggMjNhNmM3MjEzZWNhLi43NDEyYTA1ZmE1ZDkgMTAwNjQ0Cj4+Pj4+PiA+IC0tLSBhL2RyaXZl cnMvZ3B1L2RybS9pOTE1L2ludGVsX2Rpc3BsYXkuYwo+Pj4+Pj4gPiArKysgYi9kcml2ZXJzL2dw dS9kcm0vaTkxNS9pbnRlbF9kaXNwbGF5LmMKPj4+Pj4+ID4gQEAgLTE0NjM2LDYgKzE0NjM2LDcg QEAgc3RhdGljIGNvbnN0IHN0cnVjdCBkcm1fY3J0Y19mdW5jcyBpbnRlbF9jcnRjX2Z1bmNzID0g ewo+Pj4+Pj4gPiAgICAucGFnZV9mbGlwID0gaW50ZWxfY3J0Y19wYWdlX2ZsaXAsCj4+Pj4+PiA+ ICAgIC5hdG9taWNfZHVwbGljYXRlX3N0YXRlID0gaW50ZWxfY3J0Y19kdXBsaWNhdGVfc3RhdGUs Cj4+Pj4+PiA+ICAgIC5hdG9taWNfZGVzdHJveV9zdGF0ZSA9IGludGVsX2NydGNfZGVzdHJveV9z dGF0ZSwKPj4+Pj4+ID4gKyAgLnNldF9jcmNfc291cmNlID0gaW50ZWxfY3J0Y19zZXRfY3JjX3Nv dXJjZSwKPj4+Pj4+ID4gIH07Cj4+Pj4+PiA+Cj4+Pj4+PiA+ICAvKioKPj4+Pj4+ID4gZGlmZiAt LWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2Rydi5oIGIvZHJpdmVycy9ncHUvZHJt L2k5MTUvaW50ZWxfZHJ2LmgKPj4+Pj4+ID4gaW5kZXggNzM3MjYxYjA5MTEwLi4zMTg5NGI3YzY1 MTcgMTAwNjQ0Cj4+Pj4+PiA+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2Rydi5o Cj4+Pj4+PiA+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2Rydi5oCj4+Pj4+PiA+ IEBAIC0xODQ0LDYgKzE4NDQsMTQgQEAgdm9pZCBpbnRlbF9jb2xvcl9sb2FkX2x1dHMoc3RydWN0 IGRybV9jcnRjX3N0YXRlICpjcnRjX3N0YXRlKTsKPj4+Pj4+ID4gIC8qIGludGVsX3BpcGVfY3Jj LmMgKi8KPj4+Pj4+ID4gIGludCBpbnRlbF9waXBlX2NyY19jcmVhdGUoc3RydWN0IGRybV9taW5v ciAqbWlub3IpOwo+Pj4+Pj4gPiAgdm9pZCBpbnRlbF9waXBlX2NyY19jbGVhbnVwKHN0cnVjdCBk cm1fbWlub3IgKm1pbm9yKTsKPj4+Pj4+ID4gKyNpZmRlZiBDT05GSUdfREVCVUdfRlMKPj4+Pj4+ ID4gK2ludCBpbnRlbF9jcnRjX3NldF9jcmNfc291cmNlKHN0cnVjdCBkcm1fY3J0YyAqY3J0Yywg Y29uc3QgY2hhciAqc291cmNlX25hbWUsCj4+Pj4+PiA+ICsgICAgICAgICAgICAgICAgICAgICAg ICBzaXplX3QgKnZhbHVlc19jbnQpOwo+Pj4+Pj4gPiArI2Vsc2UKPj4+Pj4+ID4gK3N0YXRpYyBp bmxpbmUgaW50IGludGVsX2NydGNfc2V0X2NyY19zb3VyY2Uoc3RydWN0IGRybV9jcnRjICpjcnRj LAo+Pj4+Pj4gPiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBjb25zdCBj aGFyICpzb3VyY2VfbmFtZSwKPj4+Pj4+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgc2l6ZV90ICp2YWx1ZXNfY250KSB7IHJldHVybiAwOyB9Cj4+Pj4+PiA+ICsjZW5k aWYKPj4+Pj4+Cj4+Pj4+PiAiaW5saW5lIiBoZXJlIGRvZXNuJ3Qgd29yayBiZWNhdXNlIGl0J3Mg dXNlZCBhcyBhIGZ1bmN0aW9uIHBvaW50ZXIuCj4+Pj4+Pgo+Pj4+Pj4gSXMgaXQgYmV0dGVyIHRv IGhhdmUgYSBmdW5jdGlvbiB0aGF0IHJldHVybnMgMCBmb3IgLnNldF9jcmNfc291cmNlLCBvcgo+ Pj4+Pj4gdG8gc2V0IC5zZXRfY3JjX3NvdXJjZSB0byBOVUxMIHdoZW4gQ09ORklHX0RFQlVHX0ZT PW4/Cj4+Pj4+Cj4+Pj4+IEknZCBzYXkgdGhhdCB3aGVuZXZlciB3ZSBoYXZlIGEgZnVuY3Rpb24g cG9pbnRlciB3ZSBzaG91bGQgaGF2ZSBhIGR1bW15Cj4+Pj4+IGZ1bmN0aW9uIHdpdGhvdXQgc2lk ZS1lZmZlY3RzIGZvciB0aGlzIGtpbmQgb2YgdGhpbmdzLgo+Pj4+Cj4+Pj4gV2hvZXZlciBjYWxs cyAuc2V0X2NyY19zb3VyY2UgY291bGQgZG8gc21hcnRlciB0aGluZ3MgZGVwZW5kaW5nIG9uIHRo ZQo+Pj4+IGhvb2sgbm90IGJlaW5nIHRoZXJlIHZzLiBqdXN0IHNpbGVudGx5IHBsdW5naW5nIG9u Lgo+Pj4KPj4+IEluIHRoaXMgc3BlY2lmaWMgY2FzZSwgd2hlbiBDT05GSUdfREVCVUdfRlM9biBp dCBkb2Vzbid0IG1ha2UgYW55Cj4+PiBzZW5zZSB0byBjYWxsIHRoYXQgY2FsbGJhY2ssIHNvIEkg dGhpbmsgd2Ugc2hvdWxkIGhhdmUgYSBkdW1teQo+Pj4gaW1wbGVtZW50YXRpb24gdG8gYXZvaWQg YWRkaW5nIGFuIGlmZGVmIHRvIHRoZSAuYy4KPj4KPj4gV2UgZG9uJ3Qgd2FudCB0aGUgaWZkZWYg dG8gdGhlIC5jIGZpbGUsIGJ1dCB3ZSBjb3VsZCBkbwo+Pgo+PiAjaWZkZWYgQ09ORklHX0RFQlVH X0ZTCj4+IGludCBpbnRlbF9jcnRjX3NldF9jcmNfc291cmNlKHN0cnVjdCBkcm1fY3J0YyAqY3J0 YywgY29uc3QgY2hhciAqc291cmNlX25hbWUsCj4+ICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgc2l6ZV90ICp2YWx1ZXNfY250KTsKPj4gI2Vsc2UKPj4gI2RlZmluZSBpbnRlbF9jcnRj X3NldF9jcmNfc291cmNlIE5VTEwKPj4gI2VuZGlmCj4KPiBTb3VuZHMgZ29vZCB0byBtZSwgYW5k IHRob3VnaCBJIGRvbid0IGhhdmUgYW55IG9iamVjdGlvbnMsIHdvbmRlciB3aHkKPiB0aGlzIGlz bid0IGEgY29tbW9uIGlkaW9tIGluIHRoZSBEUk0gc3Vic3lzdGVtLiBJIHdhcyBhYmxlIHRvIGZp bmQKPiBvbmx5IG9uZSBpbnN0YW5jZTogZHJtX2NvbXBhdF9pb2N0bC4KCkhlaCwgYW5kIGl0IHdh cyBJIHdobyBzdWdnZXN0ZWQgdGhhdCB0b28uIE1heWJlIGdldCBhIHNlY29uZCBvcGluaW9uLiA7 KQoKQlIsCkphbmkuCgo+Cj4gUmVnYXJkcywKPgo+IFRvbWV1CgotLSAKSmFuaSBOaWt1bGEsIElu dGVsIE9wZW4gU291cmNlIFRlY2hub2xvZ3kgQ2VudGVyCl9fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fCkludGVsLWdmeCBtYWlsaW5nIGxpc3QKSW50ZWwtZ2Z4 QGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWls bWFuL2xpc3RpbmZvL2ludGVsLWdmeAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753340AbcKPOIe (ORCPT ); Wed, 16 Nov 2016 09:08:34 -0500 Received: from mga14.intel.com ([192.55.52.115]:59776 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751589AbcKPOId (ORCPT ); Wed, 16 Nov 2016 09:08:33 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,500,1473145200"; d="scan'208";a="787150571" From: Jani Nikula To: Tomeu Vizoso Cc: David Airlie , Intel Graphics Development , "linux-kernel\@vger.kernel.org" , "dri-devel\@lists.freedesktop.org" , Thierry Reding , Daniel Vetter Subject: Re: [Intel-gfx] [PATCH v11 3/4] drm/i915: Use new CRC debugfs API In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <1475767268-14379-1-git-send-email-tomeu.vizoso@collabora.com> <1475767268-14379-4-git-send-email-tomeu.vizoso@collabora.com> <87inrq2vee.fsf@intel.com> <20161115071609.GI8202@suiko.acc.umu.se> <87a8d12lmy.fsf@intel.com> <87h9771t0k.fsf@intel.com> Date: Wed, 16 Nov 2016 16:08:30 +0200 Message-ID: <87eg2b1pr5.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 16 Nov 2016, Tomeu Vizoso wrote: > On 16 November 2016 at 13:58, Jani Nikula wrote: >> On Wed, 16 Nov 2016, Tomeu Vizoso wrote: >>> On 15 November 2016 at 09:27, Jani Nikula wrote: >>>> On Tue, 15 Nov 2016, David Weinehall wrote: >>>>> On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote: >>>>>> On Thu, 06 Oct 2016, Tomeu Vizoso wrote: >>>>>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>>>> > index 23a6c7213eca..7412a05fa5d9 100644 >>>>>> > --- a/drivers/gpu/drm/i915/intel_display.c >>>>>> > +++ b/drivers/gpu/drm/i915/intel_display.c >>>>>> > @@ -14636,6 +14636,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = { >>>>>> > .page_flip = intel_crtc_page_flip, >>>>>> > .atomic_duplicate_state = intel_crtc_duplicate_state, >>>>>> > .atomic_destroy_state = intel_crtc_destroy_state, >>>>>> > + .set_crc_source = intel_crtc_set_crc_source, >>>>>> > }; >>>>>> > >>>>>> > /** >>>>>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>>>>> > index 737261b09110..31894b7c6517 100644 >>>>>> > --- a/drivers/gpu/drm/i915/intel_drv.h >>>>>> > +++ b/drivers/gpu/drm/i915/intel_drv.h >>>>>> > @@ -1844,6 +1844,14 @@ void intel_color_load_luts(struct drm_crtc_state *crtc_state); >>>>>> > /* intel_pipe_crc.c */ >>>>>> > int intel_pipe_crc_create(struct drm_minor *minor); >>>>>> > void intel_pipe_crc_cleanup(struct drm_minor *minor); >>>>>> > +#ifdef CONFIG_DEBUG_FS >>>>>> > +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name, >>>>>> > + size_t *values_cnt); >>>>>> > +#else >>>>>> > +static inline int intel_crtc_set_crc_source(struct drm_crtc *crtc, >>>>>> > + const char *source_name, >>>>>> > + size_t *values_cnt) { return 0; } >>>>>> > +#endif >>>>>> >>>>>> "inline" here doesn't work because it's used as a function pointer. >>>>>> >>>>>> Is it better to have a function that returns 0 for .set_crc_source, or >>>>>> to set .set_crc_source to NULL when CONFIG_DEBUG_FS=n? >>>>> >>>>> I'd say that whenever we have a function pointer we should have a dummy >>>>> function without side-effects for this kind of things. >>>> >>>> Whoever calls .set_crc_source could do smarter things depending on the >>>> hook not being there vs. just silently plunging on. >>> >>> In this specific case, when CONFIG_DEBUG_FS=n it doesn't make any >>> sense to call that callback, so I think we should have a dummy >>> implementation to avoid adding an ifdef to the .c. >> >> We don't want the ifdef to the .c file, but we could do >> >> #ifdef CONFIG_DEBUG_FS >> int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name, >> size_t *values_cnt); >> #else >> #define intel_crtc_set_crc_source NULL >> #endif > > Sounds good to me, and though I don't have any objections, wonder why > this isn't a common idiom in the DRM subsystem. I was able to find > only one instance: drm_compat_ioctl. Heh, and it was I who suggested that too. Maybe get a second opinion. ;) BR, Jani. > > Regards, > > Tomeu -- Jani Nikula, Intel Open Source Technology Center