From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey.Brodkin@synopsys.com (Alexey Brodkin) Date: Fri, 3 Mar 2017 19:24:06 +0000 Subject: [PATCH] drm/arcpgu: use .mode_fixup instead of .atomic_check In-Reply-To: <873786fc-406e-850d-f9fb-a6c9ba8cf95c@synopsys.com> References: <1488475674-6694-1-git-send-email-abrodkin@synopsys.com> <20170302195437.mlhzia2q2oav27mr@phenom.ffwll.local> <1488547649.2940.5.camel@synopsys.com> <873786fc-406e-850d-f9fb-a6c9ba8cf95c@synopsys.com> List-ID: Message-ID: <1488569045.2973.14.camel@synopsys.com> To: linux-snps-arc@lists.infradead.org Hi Jose, On Fri, 2017-03-03@18:05 +0000, Jose Abreu wrote: > Hi Alexey, > > > On 03-03-2017 13:27, Alexey Brodkin wrote: > > > > > > So if I understood you correct here what I really need is just to get rid of existing check, > > right? I.e. the following is to be in v2 respin: > > ------------------------------->8------------------------------- > > diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c > > index ad9a95916f1f..86f1555914e8 100644 > > --- a/drivers/gpu/drm/arc/arcpgu_crtc.c > > +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c > > @@ -129,20 +129,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc) > > ??????????????????????????????~ARCPGU_CTRL_ENABLE_MASK); > > ?} > > ? > > -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc, > > -????????????????????????????????????struct drm_crtc_state *state) > > -{ > > -???????struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > > -???????struct drm_display_mode *mode = &state->adjusted_mode; > > -???????long rate, clk_rate = mode->clock * 1000; > > - > > -???????rate = clk_round_rate(arcpgu->clk, clk_rate); > > -???????if (rate != clk_rate) > > -???????????????return -EINVAL; > > - > > -???????return 0; > > -} > > - > > ?static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, > > ??????????????????????????????????????struct drm_crtc_state *state) > > ?{ > > @@ -165,7 +151,6 @@ static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = { > > ????????.disable????????= arc_pgu_crtc_disable, > > ????????.prepare????????= arc_pgu_crtc_disable, > > ????????.commit?????????= arc_pgu_crtc_enable, > > -???????.atomic_check???= arc_pgu_crtc_atomic_check, > > ????????.atomic_begin???= arc_pgu_crtc_atomic_begin, > > ?}; > > ------------------------------->8------------------------------- > > I don't think you can remove the check entirely as this will make > any mode be accepted, right? Correct. Otherwise we'll get some modes and devices that don't work. Remember our saga with 74.25 vs 74.40 MHz? With our PLLs on AXS and HSDK boards we may generate 74.25 MHz clock which satisfy some monitors especially those who pass correct EDID to the host. But what if EDID is either corrupted or doesn't exist (that's my case with some industrial monitor as well as with old DVI monitor)? In that case Linux kernel attempts to calculate all the values including pixel clock but then instead of 74.25 we'll get 74.40 and equipment that used to work is no longer useful. So strictly speaking existing check makes perfect sense. But it reduces compatibility with not very good monitors. Probably better solution to the problem is just to throw away [my] faulty HW and buy equipment that conforms to standards (not really sure if EDID is a hard requirement for DVI/HDMI displays or this is just an option). BTW I'm wondering if there're any guidelines on what could be pixel clock deviation from the requested one? -Alexey From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Brodkin Subject: Re: [PATCH] drm/arcpgu: use .mode_fixup instead of .atomic_check Date: Fri, 3 Mar 2017 19:24:06 +0000 Message-ID: <1488569045.2973.14.camel@synopsys.com> References: <1488475674-6694-1-git-send-email-abrodkin@synopsys.com> <20170302195437.mlhzia2q2oav27mr@phenom.ffwll.local> <1488547649.2940.5.camel@synopsys.com> <873786fc-406e-850d-f9fb-a6c9ba8cf95c@synopsys.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <873786fc-406e-850d-f9fb-a6c9ba8cf95c@synopsys.com> Content-Language: en-US Content-ID: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-snps-arc" Errors-To: linux-snps-arc-bounces+gla-linux-snps-arc=m.gmane.org@lists.infradead.org To: Jose Abreu Cc: "airlied@linux.ie" , "daniel.vetter@ffwll.ch" , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "daniel@ffwll.ch" , "linux-snps-arc@lists.infradead.org" List-Id: dri-devel@lists.freedesktop.org SGkgSm9zZSwNCg0KT24gRnJpLCAyMDE3LTAzLTAzIGF0IDE4OjA1ICswMDAwLCBKb3NlIEFicmV1 IHdyb3RlOg0KPiBIaSBBbGV4ZXksDQo+IA0KPiANCj4gT24gMDMtMDMtMjAxNyAxMzoyNywgQWxl eGV5IEJyb2RraW4gd3JvdGU6DQo+ID4gDQo+ID4gDQo+ID4gU28gaWYgSSB1bmRlcnN0b29kIHlv dSBjb3JyZWN0IGhlcmUgd2hhdCBJIHJlYWxseSBuZWVkIGlzIGp1c3QgdG8gZ2V0IHJpZCBvZiBl eGlzdGluZyBjaGVjaywNCj4gPiByaWdodD8gSS5lLiB0aGUgZm9sbG93aW5nIGlzIHRvIGJlIGlu IHYyIHJlc3BpbjoNCj4gPiAtLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tPjgtLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2Ry bS9hcmMvYXJjcGd1X2NydGMuYyBiL2RyaXZlcnMvZ3B1L2RybS9hcmMvYXJjcGd1X2NydGMuYw0K PiA+IGluZGV4IGFkOWE5NTkxNmYxZi4uODZmMTU1NTkxNGU4IDEwMDY0NA0KPiA+IC0tLSBhL2Ry aXZlcnMvZ3B1L2RybS9hcmMvYXJjcGd1X2NydGMuYw0KPiA+ICsrKyBiL2RyaXZlcnMvZ3B1L2Ry bS9hcmMvYXJjcGd1X2NydGMuYw0KPiA+IEBAIC0xMjksMjAgKzEyOSw2IEBAIHN0YXRpYyB2b2lk IGFyY19wZ3VfY3J0Y19kaXNhYmxlKHN0cnVjdCBkcm1fY3J0YyAqY3J0YykNCj4gPiDCoMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqB+QVJD UEdVX0NUUkxfRU5BQkxFX01BU0spOw0KPiA+IMKgfQ0KPiA+IMKgDQo+ID4gLXN0YXRpYyBpbnQg YXJjX3BndV9jcnRjX2F0b21pY19jaGVjayhzdHJ1Y3QgZHJtX2NydGMgKmNydGMsDQo+ID4gLcKg wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC oMKgwqDCoMKgwqDCoHN0cnVjdCBkcm1fY3J0Y19zdGF0ZSAqc3RhdGUpDQo+ID4gLXsNCj4gPiAt wqDCoMKgwqDCoMKgwqBzdHJ1Y3QgYXJjcGd1X2RybV9wcml2YXRlICphcmNwZ3UgPSBjcnRjX3Rv X2FyY3BndV9wcml2KGNydGMpOw0KPiA+IC3CoMKgwqDCoMKgwqDCoHN0cnVjdCBkcm1fZGlzcGxh eV9tb2RlICptb2RlID0gJnN0YXRlLT5hZGp1c3RlZF9tb2RlOw0KPiA+IC3CoMKgwqDCoMKgwqDC oGxvbmcgcmF0ZSwgY2xrX3JhdGUgPSBtb2RlLT5jbG9jayAqIDEwMDA7DQo+ID4gLQ0KPiA+IC3C oMKgwqDCoMKgwqDCoHJhdGUgPSBjbGtfcm91bmRfcmF0ZShhcmNwZ3UtPmNsaywgY2xrX3JhdGUp Ow0KPiA+IC3CoMKgwqDCoMKgwqDCoGlmIChyYXRlICE9IGNsa19yYXRlKQ0KPiA+IC3CoMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqByZXR1cm4gLUVJTlZBTDsNCj4gPiAtDQo+ID4gLcKgwqDC oMKgwqDCoMKgcmV0dXJuIDA7DQo+ID4gLX0NCj4gPiAtDQo+ID4gwqBzdGF0aWMgdm9pZCBhcmNf cGd1X2NydGNfYXRvbWljX2JlZ2luKHN0cnVjdCBkcm1fY3J0YyAqY3J0YywNCj4gPiDCoMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg wqDCoMKgwqDCoMKgc3RydWN0IGRybV9jcnRjX3N0YXRlICpzdGF0ZSkNCj4gPiDCoHsNCj4gPiBA QCAtMTY1LDcgKzE1MSw2IEBAIHN0YXRpYyBjb25zdCBzdHJ1Y3QgZHJtX2NydGNfaGVscGVyX2Z1 bmNzIGFyY19wZ3VfY3J0Y19oZWxwZXJfZnVuY3MgPSB7DQo+ID4gwqDCoMKgwqDCoMKgwqDCoC5k aXNhYmxlwqDCoMKgwqDCoMKgwqDCoD0gYXJjX3BndV9jcnRjX2Rpc2FibGUsDQo+ID4gwqDCoMKg wqDCoMKgwqDCoC5wcmVwYXJlwqDCoMKgwqDCoMKgwqDCoD0gYXJjX3BndV9jcnRjX2Rpc2FibGUs DQo+ID4gwqDCoMKgwqDCoMKgwqDCoC5jb21taXTCoMKgwqDCoMKgwqDCoMKgwqA9IGFyY19wZ3Vf Y3J0Y19lbmFibGUsDQo+ID4gLcKgwqDCoMKgwqDCoMKgLmF0b21pY19jaGVja8KgwqDCoD0gYXJj X3BndV9jcnRjX2F0b21pY19jaGVjaywNCj4gPiDCoMKgwqDCoMKgwqDCoMKgLmF0b21pY19iZWdp bsKgwqDCoD0gYXJjX3BndV9jcnRjX2F0b21pY19iZWdpbiwNCj4gPiDCoH07DQo+ID4gLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLT44LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LQ0KPiANCj4gSSBkb24ndCB0aGluayB5b3UgY2FuIHJlbW92ZSB0aGUgY2hlY2sgZW50aXJlbHkg YXMgdGhpcyB3aWxsIG1ha2UNCj4gYW55IG1vZGUgYmUgYWNjZXB0ZWQsIHJpZ2h0Pw0KDQpDb3Jy ZWN0LiBPdGhlcndpc2Ugd2UnbGwgZ2V0IHNvbWUgbW9kZXMgYW5kIGRldmljZXMgdGhhdA0KZG9u J3Qgd29yay4NCg0KUmVtZW1iZXIgb3VyIHNhZ2Egd2l0aCA3NC4yNSB2cyA3NC40MCBNSHo/DQoN CldpdGggb3VyIFBMTHMgb24gQVhTIGFuZCBIU0RLIGJvYXJkcyB3ZSBtYXkgZ2VuZXJhdGUgNzQu MjUgTUh6IGNsb2NrDQp3aGljaCBzYXRpc2Z5IHNvbWUgbW9uaXRvcnMgZXNwZWNpYWxseSB0aG9z ZSB3aG8gcGFzcyBjb3JyZWN0IEVESUQgdG8gdGhlIGhvc3QuDQpCdXQgd2hhdCBpZiBFRElEIGlz IGVpdGhlciBjb3JydXB0ZWQgb3IgZG9lc24ndCBleGlzdCAodGhhdCdzIG15IGNhc2Ugd2l0aA0K c29tZSBpbmR1c3RyaWFsIG1vbml0b3IgYXMgd2VsbCBhcyB3aXRoIG9sZCBEVkkgbW9uaXRvcik/ DQoNCkluIHRoYXQgY2FzZSBMaW51eCBrZXJuZWwgYXR0ZW1wdHMgdG8gY2FsY3VsYXRlIGFsbCB0 aGUgdmFsdWVzIGluY2x1ZGluZyBwaXhlbCBjbG9jaw0KYnV0IHRoZW4gaW5zdGVhZCBvZiA3NC4y NSB3ZSdsbCBnZXQgNzQuNDAgYW5kIGVxdWlwbWVudCB0aGF0IHVzZWQgdG8gd29yayBpcyBubyBs b25nZXIgdXNlZnVsLg0KDQpTbyBzdHJpY3RseSBzcGVha2luZyBleGlzdGluZyBjaGVjayBtYWtl cyBwZXJmZWN0IHNlbnNlLiBCdXQgaXQgcmVkdWNlcw0KY29tcGF0aWJpbGl0eSB3aXRoIG5vdCB2 ZXJ5IGdvb2QgbW9uaXRvcnMuDQoNClByb2JhYmx5IGJldHRlciBzb2x1dGlvbiB0byB0aGUgcHJv YmxlbSBpcyBqdXN0IHRvIHRocm93IGF3YXkgW215XSBmYXVsdHkgSFcgYW5kDQpidXkgZXF1aXBt ZW50IHRoYXQgY29uZm9ybXMgdG8gc3RhbmRhcmRzIChub3QgcmVhbGx5IHN1cmUgaWYgRURJRCBp cyBhIGhhcmQNCnJlcXVpcmVtZW50IGZvciBEVkkvSERNSSBkaXNwbGF5cyBvciB0aGlzIGlzIGp1 c3QgYW4gb3B0aW9uKS4NCg0KQlRXIEknbSB3b25kZXJpbmcgaWYgdGhlcmUncmUgYW55IGd1aWRl bGluZXMgb24gd2hhdCBjb3VsZCBiZSBwaXhlbCBjbG9jaw0KZGV2aWF0aW9uIGZyb20gdGhlIHJl cXVlc3RlZCBvbmU/DQoNCi1BbGV4ZXkKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX18KbGludXgtc25wcy1hcmMgbWFpbGluZyBsaXN0CmxpbnV4LXNucHMtYXJj QGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9s aXN0aW5mby9saW51eC1zbnBzLWFyYw== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752139AbdCCTcC (ORCPT ); Fri, 3 Mar 2017 14:32:02 -0500 Received: from smtprelay.synopsys.com ([198.182.47.9]:46222 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751722AbdCCTcA (ORCPT ); Fri, 3 Mar 2017 14:32:00 -0500 From: Alexey Brodkin To: Jose Abreu CC: "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "daniel@ffwll.ch" , "daniel.vetter@ffwll.ch" , "linux-snps-arc@lists.infradead.org" , "airlied@linux.ie" Subject: Re: [PATCH] drm/arcpgu: use .mode_fixup instead of .atomic_check Thread-Topic: [PATCH] drm/arcpgu: use .mode_fixup instead of .atomic_check Thread-Index: AQHSk3pbdSSv6ibt+0alj80EoRWJA6GB5e0AgAEmKoCAAE2bgIAAFgeA Date: Fri, 3 Mar 2017 19:24:06 +0000 Message-ID: <1488569045.2973.14.camel@synopsys.com> References: <1488475674-6694-1-git-send-email-abrodkin@synopsys.com> <20170302195437.mlhzia2q2oav27mr@phenom.ffwll.local> <1488547649.2940.5.camel@synopsys.com> <873786fc-406e-850d-f9fb-a6c9ba8cf95c@synopsys.com> In-Reply-To: <873786fc-406e-850d-f9fb-a6c9ba8cf95c@synopsys.com> Accept-Language: en-US, ru-RU Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.225.15.124] Content-Type: text/plain; charset="utf-8" Content-ID: MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v23JW5dK009953 Hi Jose, On Fri, 2017-03-03 at 18:05 +0000, Jose Abreu wrote: > Hi Alexey, > > > On 03-03-2017 13:27, Alexey Brodkin wrote: > > > > > > So if I understood you correct here what I really need is just to get rid of existing check, > > right? I.e. the following is to be in v2 respin: > > ------------------------------->8------------------------------- > > diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c > > index ad9a95916f1f..86f1555914e8 100644 > > --- a/drivers/gpu/drm/arc/arcpgu_crtc.c > > +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c > > @@ -129,20 +129,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc) > >                               ~ARCPGU_CTRL_ENABLE_MASK); > >  } > >   > > -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc, > > -                                    struct drm_crtc_state *state) > > -{ > > -       struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > > -       struct drm_display_mode *mode = &state->adjusted_mode; > > -       long rate, clk_rate = mode->clock * 1000; > > - > > -       rate = clk_round_rate(arcpgu->clk, clk_rate); > > -       if (rate != clk_rate) > > -               return -EINVAL; > > - > > -       return 0; > > -} > > - > >  static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, > >                                       struct drm_crtc_state *state) > >  { > > @@ -165,7 +151,6 @@ static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = { > >         .disable        = arc_pgu_crtc_disable, > >         .prepare        = arc_pgu_crtc_disable, > >         .commit         = arc_pgu_crtc_enable, > > -       .atomic_check   = arc_pgu_crtc_atomic_check, > >         .atomic_begin   = arc_pgu_crtc_atomic_begin, > >  }; > > ------------------------------->8------------------------------- > > I don't think you can remove the check entirely as this will make > any mode be accepted, right? Correct. Otherwise we'll get some modes and devices that don't work. Remember our saga with 74.25 vs 74.40 MHz? With our PLLs on AXS and HSDK boards we may generate 74.25 MHz clock which satisfy some monitors especially those who pass correct EDID to the host. But what if EDID is either corrupted or doesn't exist (that's my case with some industrial monitor as well as with old DVI monitor)? In that case Linux kernel attempts to calculate all the values including pixel clock but then instead of 74.25 we'll get 74.40 and equipment that used to work is no longer useful. So strictly speaking existing check makes perfect sense. But it reduces compatibility with not very good monitors. Probably better solution to the problem is just to throw away [my] faulty HW and buy equipment that conforms to standards (not really sure if EDID is a hard requirement for DVI/HDMI displays or this is just an option). BTW I'm wondering if there're any guidelines on what could be pixel clock deviation from the requested one? -Alexey