Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
On 7/28/21 7:00 AM, Sam Ravnborg wrote:
> [You don't often get email from s...@ravnborg.org. Learn why this is
> important at http://aka.ms/LearnAboutSenderIdentification.]
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Hi Dan,
>
> I hope you can fine to test this patch from Thomas.
> If this works then we can forget about my attempt to do the same.
I'll test this as soon as I can and let you know.
Thanks,
Dan
>
> Hi Thomas,
>
> IRQ_NOTCONNECTED check seems redundant, as mentioned in another patch
> already.
>
> With that considered:
> Reviewed-by: Sam Ravnborg
>
> We shall wait for testing from Dan before you apply it as I had made a
> similar patch that failed to work. I assume my patch was buggy but I
> prefer to be sure.
>
> Sam
>
> On Tue, Jul 27, 2021 at 08:27:10PM +0200, Thomas Zimmermann wrote:
>> Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's
>> IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers
>> don't benefit from using it. DRM IRQ callbacks are now being called
>> directly or inlined.
>>
>> Signed-off-by: Thomas Zimmermann
>> ---
>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 85
>> 1 file changed, 52 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> index f09b6dd8754c..cfa8c2c9c8aa 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> @@ -22,7 +22,6 @@
>> #include
>> #include
>> #include
>> -#include
>> #include
>> #include
>>
>> @@ -557,6 +556,56 @@ static irqreturn_t atmel_hlcdc_dc_irq_handler(int irq,
>> void *data)
>>return IRQ_HANDLED;
>> }
>>
>> +static void atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev)
>> +{
>> + struct atmel_hlcdc_dc *dc = dev->dev_private;
>> + unsigned int cfg = 0;
>> + int i;
>> +
>> + /* Enable interrupts on activated layers */
>> + for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) {
>> + if (dc->layers[i])
>> + cfg |= ATMEL_HLCDC_LAYER_STATUS(i);
>> + }
>> +
>> + regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, cfg);
>> +}
>> +
>> +static void atmel_hlcdc_dc_irq_disable(struct drm_device *dev)
>> +{
>> + struct atmel_hlcdc_dc *dc = dev->dev_private;
>> + unsigned int isr;
>> +
>> + regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IDR, 0x);
>> + regmap_read(dc->hlcdc->regmap, ATMEL_HLCDC_ISR, &isr);
>> +}
>> +
>> +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, unsigned int
>> irq)
>> +{
>> + int ret;
>> +
>> + if (irq == IRQ_NOTCONNECTED)
>> + return -ENOTCONN;
>> +
>> + atmel_hlcdc_dc_irq_disable(dev);
>> +
>> + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0,
>> dev->driver->name, dev);
>> + if (ret)
>> + return ret;
>> +
>> + atmel_hlcdc_dc_irq_postinstall(dev);
>> +
>> + return 0;
>> +}
>> +
>> +static void atmel_hlcdc_dc_irq_uninstall(struct drm_device *dev)
>> +{
>> + struct atmel_hlcdc_dc *dc = dev->dev_private;
>> +
>> + atmel_hlcdc_dc_irq_disable(dev);
>> + free_irq(dc->hlcdc->irq, dev);
>> +}
>> +
>> static const struct drm_mode_config_funcs mode_config_funcs = {
>>.fb_create = drm_gem_fb_create,
>>.atomic_check = drm_atomic_helper_check,
>> @@ -647,7 +696,7 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
>>drm_mode_config_reset(dev);
>>
>>pm_runtime_get_sync(dev->dev);
>> - ret = drm_irq_install(dev, dc->hlcdc->irq);
>> + ret = atmel_hlcdc_dc_irq_install(dev, dc->hlcdc->irq);
>>pm_runtime_put_sync(dev->dev);
>>if (ret < 0) {
>>dev_err(dev->dev, "failed to install IRQ handler\n");
>> @@ -676,7 +725,7 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev)
>>drm_mode_config_cleanup(dev);
>>
>>pm_runtime_get_sync(dev->dev);
>> - drm_irq_uninstall(dev);
>> + atmel_hlcdc_dc_irq_uninstall(dev);
>>pm_runtime_put_sync(dev->dev);
>>
>>dev->dev_private = NULL;
>> @@ -685,40 +734,10 @@ static void atmel_hlcdc_dc_unload(struct drm_device
>> *dev)
>>clk_disable_unprepare(dc->hlcdc->periph_clk);
>> }
>>
>> -static int atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev)
>> -{
>> - struct atmel_hlcdc_dc *dc = dev->dev_private;
>> - unsigned int cfg = 0;
>> - int i;
>> -
>> - /* Enable interrupts on activated layers */
>> - for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) {
>> - if (dc->layers[i])
>> - cfg |= ATMEL_HLCDC_LAYER_STATUS(i);
>> - }
>> -
>> - regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, cfg);
>> -
>> - return 0;
>> -}
>> -
>> -static void atmel_hlcdc_dc_irq_uninstall(struct drm_device *dev)
>> -{
>> - struct atmel_hlcdc_dc *dc = dev->dev_private;
>> - unsigne
Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
On 7/28/21 8:44 AM, Sam Ravnborg wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Dan, > > On Wed, Jul 28, 2021 at 03:11:08PM +, dan.sned...@microchip.com wrote: >> On 7/28/21 7:00 AM, Sam Ravnborg wrote: >>> [You don't often get email from s...@ravnborg.org. Learn why this is >>> important at http://aka.ms/LearnAboutSenderIdentification.] >>> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >>> content is safe >>> >>> Hi Dan, >>> >>> I hope you can fine to test this patch from Thomas. >>> If this works then we can forget about my attempt to do the same. >> >> I'll test this as soon as I can and let you know. > > Thanks, crossing my fingers... (which explains the funny spelling from > time to time) > > Sam > So I ran the test on an A5D27 XULT board with a PDA5 display. Our graphics demos that come with our linux4sam releases seem to run just fine. modetest -v seems to run just fine. However, vbltest returns "drmWaitVBlank (relative) failed ret: -1". I don't understand why modetest -v is working and vbltest isn't, but that's what I'm seeing. Thanks, Dan
Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
On 7/28/21 11:11 AM, Sam Ravnborg wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Dan, > > thanks for the quick feedback! > > On Wed, Jul 28, 2021 at 05:50:34PM +, dan.sned...@microchip.com wrote: >> On 7/28/21 8:44 AM, Sam Ravnborg wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >>> content is safe >>> >>> Hi Dan, >>> >>> On Wed, Jul 28, 2021 at 03:11:08PM +, dan.sned...@microchip.com wrote: On 7/28/21 7:00 AM, Sam Ravnborg wrote: > [You don't often get email from s...@ravnborg.org. Learn why this is > important at http://aka.ms/LearnAboutSenderIdentification.] > > EXTERNAL EMAIL: Do not click links or open attachments unless you know > the content is safe > > Hi Dan, > > I hope you can fine to test this patch from Thomas. > If this works then we can forget about my attempt to do the same. I'll test this as soon as I can and let you know. >>> >>> Thanks, crossing my fingers... (which explains the funny spelling from >>> time to time) >>> >>> Sam >>> So I ran the test on an A5D27 XULT board with a PDA5 display. Our >> graphics demos that come with our linux4sam releases seem to run just >> fine. modetest -v seems to run just fine. However, vbltest returns >> "drmWaitVBlank (relative) failed ret: -1". I don't understand why >> modetest -v is working and vbltest isn't, but that's what I'm seeing. > > Strange indeed. > > > Just to be sure... > Can you confirm that vbltest is working OK *before* this patch? > > Sam > I'm afraid vbltest works without the patch, but not with it. Dan
Re: Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
Hi Thomas, On 7/28/21 11:17 AM, Thomas Zimmermann wrote: > Hi > > Am 28.07.21 um 20:11 schrieb Sam Ravnborg: >> Hi Dan, >> >> thanks for the quick feedback! >> >> On Wed, Jul 28, 2021 at 05:50:34PM +, dan.sned...@microchip.com >> wrote: >>> On 7/28/21 8:44 AM, Sam Ravnborg wrote: EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe Hi Dan, On Wed, Jul 28, 2021 at 03:11:08PM +, dan.sned...@microchip.com wrote: > On 7/28/21 7:00 AM, Sam Ravnborg wrote: >> [You don't often get email from s...@ravnborg.org. Learn why this >> is important at http://aka.ms/LearnAboutSenderIdentification.] >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you >> know the content is safe >> >> Hi Dan, >> >> I hope you can fine to test this patch from Thomas. >> If this works then we can forget about my attempt to do the same. > > I'll test this as soon as I can and let you know. Thanks, crossing my fingers... (which explains the funny spelling from time to time) Sam So I ran the test on an A5D27 XULT board with a PDA5 display. Our >>> graphics demos that come with our linux4sam releases seem to run just >>> fine. modetest -v seems to run just fine. However, vbltest returns >>> "drmWaitVBlank (relative) failed ret: -1". I don't understand why >>> modetest -v is working and vbltest isn't, but that's what I'm seeing. > > Thanks for testing. > >> >> Strange indeed. >> >> >> Just to be sure... >> Can you confirm that vbltest is working OK *before* this patch? > > Yes, can you please verify that it regressed. If so, this would mean > that the driver misses vblank interrupts with the patch applied. Yes, unfortunately the vbltest works before this patch, but fails after this patch is applied. Best Regards, Dan > > Best regards > Thomas > >> >> Sam >> >
Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
Hi Sam,
On 7/28/21 12:08 PM, Sam Ravnborg wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Hi Dan,
>
Just to be sure...
Can you confirm that vbltest is working OK *before* this patch?
>>>
>>> Yes, can you please verify that it regressed. If so, this would mean
>>> that the driver misses vblank interrupts with the patch applied.
>>
>> Yes, unfortunately the vbltest works before this patch, but fails after
>> this patch is applied.
>
> I think I got it - we need to set irq_enabled to true.
> The documentation says so:
> "
> * @irq_enabled:
> *
> * Indicates that interrupt handling is enabled, specifically vblank
> * handling. Drivers which don't use drm_irq_install() need to set
> this
> * to true manually.
> "
>
> Can you try to add the following line:
>
>
> +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, unsigned int
> irq)
> +{
> + int ret;
> +
> + if (irq == IRQ_NOTCONNECTED)
> + return -ENOTCONN;
> +
>
> dev->irq_enabled = true;<= THIS LINE
>
>
> + atmel_hlcdc_dc_irq_disable(dev);
> +
> + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0,
> dev->driver->name, dev);
> + if (ret)
> + return ret;
>
> I hope this fixes it.
It does! With the irq_enabled line added everything is looking good.
Best Regards,
Dan
>
> Sam
>
Re: Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
Hi Thomas,
On 7/29/21 12:18 PM, Thomas Zimmermann wrote:
> Hi
>
> Am 28.07.21 um 22:11 schrieb Sam Ravnborg:
>> Hi Dan,
>>
I think I got it - we need to set irq_enabled to true.
The documentation says so:
"
* @irq_enabled:
*
* Indicates that interrupt handling is enabled,
specifically vblank
* handling. Drivers which don't use drm_irq_install()
need to set this
* to true manually.
"
Can you try to add the following line:
+static int atmel_hlcdc_dc_irq_install(struct drm_device *dev,
unsigned int irq)
+{
+ int ret;
+
+ if (irq == IRQ_NOTCONNECTED)
+ return -ENOTCONN;
+
dev->irq_enabled = true; <= THIS LINE
+ atmel_hlcdc_dc_irq_disable(dev);
+
+ ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0,
dev->driver->name, dev);
+ if (ret)
+ return ret;
I hope this fixes it.
>>>
>>> It does! With the irq_enabled line added everything is looking good.
>
> Are you sure, you're testing with the latest drm-misc-next or drm-tip?
> Because using irq_enabled is deprecated and the flag was recently
> replaced by commit 1e4cd78ed493 ("drm: Don't test for IRQ support in
> VBLANK ioctls").
>
> Best regards
> Thomas
>
I was testing with 5.14-rc3. I can test with drm-tip or drm-misc-next.
There a preferred branch to test from?
Thanks and regards,
Dan
>>
>> Great, thanks for testing.
>>
>> Thomas - I assume you will do a re-spin and there is likely some fixes
>> for the applied IRQ conversions too.
>>
>> Note - irq_enabled must be cleared if request_irq fails. I did not
>> include this in the testing here.
>>
>> Sam
>>
>
Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
Hi Thomas and Sam,
On 7/29/21 12:48 PM, Sam Ravnborg wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Hi Thomas,
>
>>
>> Are you sure, you're testing with the latest drm-misc-next or drm-tip?
>> Because using irq_enabled is deprecated and the flag was recently replaced
>> by commit 1e4cd78ed493 ("drm: Don't test for IRQ support in VBLANK ioctls").
Ok, My fault for testing on the wrong branch. When I test this patch on
drm-misc-next it works great. Sorry for the confusion!
>
> I was looking at drm-misc-fixes which did not have this commit :-(
> Just my silly excuse why I was convinced this was the issue.
>
> Sam
>
Best regards,
Dan
Re: Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
On 7/30/21 1:31 AM, Thomas Zimmermann wrote:
> Hi Dan and Sam
>
> Am 29.07.21 um 21:55 schrieb dan.sned...@microchip.com:
>> Hi Thomas and Sam,
>> On 7/29/21 12:48 PM, Sam Ravnborg wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>> know the content is safe
>>>
>>> Hi Thomas,
>>>
Are you sure, you're testing with the latest drm-misc-next or drm-tip?
Because using irq_enabled is deprecated and the flag was recently
replaced
by commit 1e4cd78ed493 ("drm: Don't test for IRQ support in VBLANK
ioctls").
>>
>> Ok, My fault for testing on the wrong branch. When I test this patch on
>> drm-misc-next it works great. Sorry for the confusion!
>>
>>>
>>> I was looking at drm-misc-fixes which did not have this commit :-(
>>> Just my silly excuse why I was convinced this was the issue.
>
> Don't worry.
>
> I'll add Sam's R-b and a Tested-by from Dan to the patch. Is that ok?
The tested-by works for me! Thanks!
>
> Best regards
> Thomas
>
>
>>>
>>> Sam
>>>
>>
>> Best regards,
>> Dan
>>
>
Re: [PATCH v1 0/2] drm/atmel-hlcdc: drop use of drm_irq mid-layer
I tested this patch set on the 9x60ek board and unfortunately it causes errors. The vbltest returns -1 and I also see the display get out of sync while testing with a gui application. Regards, Dan
