Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 18 Jul 2019 11:23:50 -0400
From:   Sean Paul <sean@poorly.run>
To:     Liviu Dudau <Liviu.Dudau@arm.com>
Cc:     "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>,
        "james qian wang (Arm Technology China)" <james.qian.wang@arm.com>,
        "maarten.lankhorst@linux.intel.com" 
        <maarten.lankhorst@linux.intel.com>,
        "seanpaul@chromium.org" <seanpaul@chromium.org>,
        "airlied@linux.ie" <airlied@linux.ie>,
        Brian Starkey <Brian.Starkey@arm.com>,
        "Julien Yin (Arm Technology China)" <Julien.Yin@arm.com>,
        "Jonathan Chai (Arm Technology China)" <Jonathan.Chai@arm.com>,
        Ayan Halder <Ayan.Halder@arm.com>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        nd <nd@arm.com>
Subject: Re: [PATCH] drm/komeda: Adds error event print functionality
Message-ID: <20190718152350.GC31819@art_vandelay>
References: <1561604994-26925-1-git-send-email-lowry.li@arm.com>
 <20190718131737.GD5942@e110455-lin.cambridge.arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190718131737.GD5942@e110455-lin.cambridge.arm.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 18, 2019 at 02:17:37PM +0100, Liviu Dudau wrote:
> On Thu, Jun 27, 2019 at 04:10:36AM +0100, Lowry Li (Arm Technology China) wrote:

/snip

> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > index 647bce5..1462bac 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > @@ -47,6 +47,8 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
> >  	memset(&evts, 0, sizeof(evts));
> >  	status = mdev->funcs->irq_handler(mdev, &evts);
> >  
> > +	komeda_print_events(&evts);
> 
> Calling this function from the IRQ handler is a bad idea. We should use debugfs
> if you really want to have a trace of the events, but I personally don't see
> value in having this functionality in the kernel at all. You can expose the
> value of the evts->global and evts->pipes[] as integers and decode that in
> userspace or as a debugfs entry.

Alternatively, consider using kernel trace events. They allow you to selectively
turn on/off certain events and also allow you to customize which data is
recorded and how it's formatted. Seems like a good fit from the quick scan I've
done.

Sean

> 
> Best regards,
> Liviu
> 
> > +
> >  	/* Notify the crtc to handle the events */
> >  	for (i = 0; i < kms->n_crtcs; i++)
> >  		komeda_crtc_handle_event(&kms->crtcs[i], &evts);
> > -- 
> > 1.9.1
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

-- 
Sean Paul, Software Engineer, Google / Chromium OS