From: Tomi Valkeinen <tomi.valkeinen@ti.com> To: Hans Verkuil <hverkuil@xs4all.nl>, <linux-media@vger.kernel.org> Cc: <dri-devel@lists.freedesktop.org>, Hans Verkuil <hans.verkuil@cisco.com> Subject: Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done Date: Mon, 8 Oct 2018 15:45:24 +0300 [thread overview] Message-ID: <70112c2d-8551-4d78-0a4e-de55d71b98dc@ti.com> (raw) In-Reply-To: <20181004090900.32915-5-hverkuil@xs4all.nl> Hi Hans, On 04/10/18 12:08, Hans Verkuil wrote: > From: Hans Verkuil <hans.verkuil@cisco.com> > > The TX FIFO has to be cleared if the transmit failed due to e.g. > a NACK condition, otherwise the hardware will keep trying to > transmit the message. > > An attempt was made to do this, but it was done after the call to > cec_transmit_done, which can cause a race condition since the call > to cec_transmit_done can cause a new transmit to be issued, and > then attempting to clear the TX FIFO will actually clear the new > transmit instead of the old transmit and the new transmit simply > never happens. > > By clearing the FIFO before transmit_done is called this race > is fixed. > > Note that there is no reason to clear the FIFO if the transmit > was successful, so the attempt to clear the FIFO in that case > was dropped. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 35 ++++++++++++------------- > 1 file changed, 17 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c > index 340383150fb9..dee66a5101b5 100644 > --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c > +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c > @@ -106,6 +106,22 @@ static void hdmi_cec_received_msg(struct hdmi_core_data *core) > } > } > > +static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap) > +{ > + struct hdmi_core_data *core = cec_get_drvdata(adap); > + int retry = HDMI_CORE_CEC_RETRY; > + int temp; > + > + REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7); > + while (retry) { > + temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3); > + if (FLD_GET(temp, 7, 7) == 0) > + break; This is fine, but as you're using the helper macros already, there's REG_GET: REG_GET(core->base, HDMI_CEC_DBG_3, 7, 7) which removes the need for temp. Are you sure this works reliably? Usually when polling a register bit, I like to measure real-world-time in some way to ensure I actually poll for a certain amount of time. And just a matter of opinion, but I would've written: while (retry) { if (!REG_GET(..)) return true; retry--; } return false; > + retry--; > + } > + return retry != 0; > +} > + > void hdmi4_cec_irq(struct hdmi_core_data *core) > { > u32 stat0 = hdmi_read_reg(core->base, HDMI_CEC_INT_STATUS_0); > @@ -117,36 +133,19 @@ void hdmi4_cec_irq(struct hdmi_core_data *core) > if (stat0 & 0x20) { > cec_transmit_done(core->adap, CEC_TX_STATUS_OK, > 0, 0, 0, 0); > - REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7); > } else if (stat1 & 0x02) { > u32 dbg3 = hdmi_read_reg(core->base, HDMI_CEC_DBG_3); > > + hdmi_cec_clear_tx_fifo(core->adap); Would a dev_err be ok here? Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
WARNING: multiple messages have this Message-ID (diff)
From: Tomi Valkeinen <tomi.valkeinen@ti.com> To: Hans Verkuil <hverkuil@xs4all.nl>, linux-media@vger.kernel.org Cc: Hans Verkuil <hans.verkuil@cisco.com>, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done Date: Mon, 8 Oct 2018 15:45:24 +0300 [thread overview] Message-ID: <70112c2d-8551-4d78-0a4e-de55d71b98dc@ti.com> (raw) In-Reply-To: <20181004090900.32915-5-hverkuil@xs4all.nl> Hi Hans, On 04/10/18 12:08, Hans Verkuil wrote: > From: Hans Verkuil <hans.verkuil@cisco.com> > > The TX FIFO has to be cleared if the transmit failed due to e.g. > a NACK condition, otherwise the hardware will keep trying to > transmit the message. > > An attempt was made to do this, but it was done after the call to > cec_transmit_done, which can cause a race condition since the call > to cec_transmit_done can cause a new transmit to be issued, and > then attempting to clear the TX FIFO will actually clear the new > transmit instead of the old transmit and the new transmit simply > never happens. > > By clearing the FIFO before transmit_done is called this race > is fixed. > > Note that there is no reason to clear the FIFO if the transmit > was successful, so the attempt to clear the FIFO in that case > was dropped. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 35 ++++++++++++------------- > 1 file changed, 17 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c > index 340383150fb9..dee66a5101b5 100644 > --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c > +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c > @@ -106,6 +106,22 @@ static void hdmi_cec_received_msg(struct hdmi_core_data *core) > } > } > > +static bool hdmi_cec_clear_tx_fifo(struct cec_adapter *adap) > +{ > + struct hdmi_core_data *core = cec_get_drvdata(adap); > + int retry = HDMI_CORE_CEC_RETRY; > + int temp; > + > + REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7); > + while (retry) { > + temp = hdmi_read_reg(core->base, HDMI_CEC_DBG_3); > + if (FLD_GET(temp, 7, 7) == 0) > + break; This is fine, but as you're using the helper macros already, there's REG_GET: REG_GET(core->base, HDMI_CEC_DBG_3, 7, 7) which removes the need for temp. Are you sure this works reliably? Usually when polling a register bit, I like to measure real-world-time in some way to ensure I actually poll for a certain amount of time. And just a matter of opinion, but I would've written: while (retry) { if (!REG_GET(..)) return true; retry--; } return false; > + retry--; > + } > + return retry != 0; > +} > + > void hdmi4_cec_irq(struct hdmi_core_data *core) > { > u32 stat0 = hdmi_read_reg(core->base, HDMI_CEC_INT_STATUS_0); > @@ -117,36 +133,19 @@ void hdmi4_cec_irq(struct hdmi_core_data *core) > if (stat0 & 0x20) { > cec_transmit_done(core->adap, CEC_TX_STATUS_OK, > 0, 0, 0, 0); > - REG_FLD_MOD(core->base, HDMI_CEC_DBG_3, 0x1, 7, 7); > } else if (stat1 & 0x02) { > u32 dbg3 = hdmi_read_reg(core->base, HDMI_CEC_DBG_3); > > + hdmi_cec_clear_tx_fifo(core->adap); Would a dev_err be ok here? Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2018-10-08 19:57 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-10-04 9:08 [PATCH 0/5] cec/adv/omap: fixes and new status flags Hans Verkuil 2018-10-04 9:08 ` Hans Verkuil 2018-10-04 9:08 ` [PATCH 1/5] cec: add new tx/rx status bits to detect aborts/timeouts Hans Verkuil 2018-10-04 9:08 ` Hans Verkuil 2018-10-04 9:08 ` [PATCH 2/5] adv7604: when the EDID is cleared, unconfigure CEC as well Hans Verkuil 2018-10-04 9:08 ` Hans Verkuil 2018-10-04 9:08 ` [PATCH 3/5] adv7842: " Hans Verkuil 2018-10-04 9:08 ` Hans Verkuil 2018-10-04 9:08 ` [PATCH 4/5] omapdrm/dss/hdmi4_cec.c: clear TX FIFO before transmit_done Hans Verkuil 2018-10-04 9:08 ` Hans Verkuil 2018-10-05 14:13 ` Hans Verkuil 2018-10-05 14:13 ` Hans Verkuil 2018-10-08 12:52 ` Tomi Valkeinen 2018-10-08 12:52 ` Tomi Valkeinen 2018-10-08 12:58 ` Hans Verkuil 2018-10-08 12:58 ` Hans Verkuil 2018-10-08 12:45 ` Tomi Valkeinen [this message] 2018-10-08 12:45 ` Tomi Valkeinen 2018-10-08 12:55 ` Hans Verkuil 2018-10-08 12:55 ` Hans Verkuil 2018-10-08 13:03 ` Tomi Valkeinen 2018-10-08 13:03 ` Tomi Valkeinen 2018-10-04 9:09 ` [PATCH 5/5] omapdrm/dss/hdmi4_cec.c: don't set the retransmit count Hans Verkuil 2018-10-04 9:09 ` Hans Verkuil 2018-10-08 12:47 ` Tomi Valkeinen 2018-10-08 12:47 ` Tomi Valkeinen 2018-10-08 13:01 ` Hans Verkuil 2018-10-08 13:01 ` Hans Verkuil
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=70112c2d-8551-4d78-0a4e-de55d71b98dc@ti.com \ --to=tomi.valkeinen@ti.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=hans.verkuil@cisco.com \ --cc=hverkuil@xs4all.nl \ --cc=linux-media@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.