Re: [PATCH (repost) 1/5] drm_dp_cec: check that aux has a transfer function
On Mon, 2018-08-20 at 22:47 +0200, Hans Verkuil wrote:
> On 08/20/2018 08:51 PM, Lyude Paul wrote:
> > On Fri, 2018-08-17 at 16:11 +0200, Hans Verkuil wrote:
> > > From: Hans Verkuil
> > >
> > > If aux->transfer == NULL, then just return without doing
> > > anything. In that case the function is likely called for
> > > a non-(e)DP connector.
> > >
> > > This never happened for the i915 driver, but the nouveau and amdgpu
> > > drivers need this check.
> >
> > Could you give a backtrace from where you're hitting this issue with nouveau
> > and
> > amdgpu? It doesn't make a whole ton of sense to have connectors registering
> > DP
> > aux busses if they aren't actually DP, that should probably just be fixed...
>
Agh, nouveau and amdgpu making questionable decisions :s... but I suppose that
makes sense.
Reviewed-by: Lyude Paul
> The difference between the i915 driver and the nouveau (and amdgpu) driver is
> that in the i915 driver the drm_dp_cec_set_edid/unset_edid/irq functions are
> called from code that is exclusively for DisplayPort connectors.
>
> For nouveau/amdgpu they are called from code that is shared between
> DisplayPort
> and HDMI, so aux->transfer may be NULL.
>
> Rather than either testing for the connector type or for a non-NULL aux-
> >transfer
> every time I call a drm_dp_cec_* function, it is better to just test for
> aux->transfer in the drm_dp_cec_* functions themselves. It's more robust.
>
> So there isn't a bug or anything like that, it's just so that these drm_dp_cec
> functions can handle a slightly different driver design safely.
>
> The registration and unregistration of the cec devices is always DP specific,
> and an attempt to register a cec device for a non-DP connector will now fail
> with a WARN_ON.
>
> Regards,
>
> Hans
>
> >
> > >
> > > The alternative would be to add this check in those drivers before
> > > every drm_dp_cec call, but it makes sense to check it in the
> > > drm_dp_cec functions to prevent a kernel oops.
> > >
> > > Signed-off-by: Hans Verkuil
> > > ---
> > > drivers/gpu/drm/drm_dp_cec.c | 14 ++
> > > 1 file changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> > > index 988513346e9c..1407b13a8d5d 100644
> > > --- a/drivers/gpu/drm/drm_dp_cec.c
> > > +++ b/drivers/gpu/drm/drm_dp_cec.c
> > > @@ -238,6 +238,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
> > > u8 cec_irq;
> > > int ret;
> > >
> > > + /* No transfer function was set, so not a DP connector */
> > > + if (!aux->transfer)
> > > + return;
> > > +
> > > mutex_lock(>cec.lock);
> > > if (!aux->cec.adap)
> > > goto unlock;
> > > @@ -293,6 +297,10 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux,
> > > const
> > > struct edid *edid)
> > > unsigned int num_las = 1;
> > > u8 cap;
> > >
> > > + /* No transfer function was set, so not a DP connector */
> > > + if (!aux->transfer)
> > > + return;
> > > +
> > > #ifndef CONFIG_MEDIA_CEC_RC
> > > /*
> > >* CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
> > > @@ -361,6 +369,10 @@ EXPORT_SYMBOL(drm_dp_cec_set_edid);
> > > */
> > > void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
> > > {
> > > + /* No transfer function was set, so not a DP connector */
> > > + if (!aux->transfer)
> > > + return;
> > > +
> > > cancel_delayed_work_sync(>cec.unregister_work);
> > >
> > > mutex_lock(>cec.lock);
> > > @@ -404,6 +416,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux
> > > *aux,
> > > const char *name,
> > > struct device *parent)
> > > {
> > > WARN_ON(aux->cec.adap);
> > > + if (WARN_ON(!aux->transfer))
> > > + return;
> > > aux->cec.name = name;
> > > aux->cec.parent = parent;
> > > INIT_DELAYED_WORK(>cec.unregister_work,
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH (repost) 1/5] drm_dp_cec: check that aux has a transfer function
On 08/20/2018 08:51 PM, Lyude Paul wrote:
> On Fri, 2018-08-17 at 16:11 +0200, Hans Verkuil wrote:
>> From: Hans Verkuil
>>
>> If aux->transfer == NULL, then just return without doing
>> anything. In that case the function is likely called for
>> a non-(e)DP connector.
>>
>> This never happened for the i915 driver, but the nouveau and amdgpu
>> drivers need this check.
> Could you give a backtrace from where you're hitting this issue with nouveau
> and
> amdgpu? It doesn't make a whole ton of sense to have connectors registering DP
> aux busses if they aren't actually DP, that should probably just be fixed...
The difference between the i915 driver and the nouveau (and amdgpu) driver is
that in the i915 driver the drm_dp_cec_set_edid/unset_edid/irq functions are
called from code that is exclusively for DisplayPort connectors.
For nouveau/amdgpu they are called from code that is shared between DisplayPort
and HDMI, so aux->transfer may be NULL.
Rather than either testing for the connector type or for a non-NULL
aux->transfer
every time I call a drm_dp_cec_* function, it is better to just test for
aux->transfer in the drm_dp_cec_* functions themselves. It's more robust.
So there isn't a bug or anything like that, it's just so that these drm_dp_cec
functions can handle a slightly different driver design safely.
The registration and unregistration of the cec devices is always DP specific,
and an attempt to register a cec device for a non-DP connector will now fail
with a WARN_ON.
Regards,
Hans
>
>>
>> The alternative would be to add this check in those drivers before
>> every drm_dp_cec call, but it makes sense to check it in the
>> drm_dp_cec functions to prevent a kernel oops.
>>
>> Signed-off-by: Hans Verkuil
>> ---
>> drivers/gpu/drm/drm_dp_cec.c | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
>> index 988513346e9c..1407b13a8d5d 100644
>> --- a/drivers/gpu/drm/drm_dp_cec.c
>> +++ b/drivers/gpu/drm/drm_dp_cec.c
>> @@ -238,6 +238,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
>> u8 cec_irq;
>> int ret;
>>
>> +/* No transfer function was set, so not a DP connector */
>> +if (!aux->transfer)
>> +return;
>> +
>> mutex_lock(>cec.lock);
>> if (!aux->cec.adap)
>> goto unlock;
>> @@ -293,6 +297,10 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
>> struct edid *edid)
>> unsigned int num_las = 1;
>> u8 cap;
>>
>> +/* No transfer function was set, so not a DP connector */
>> +if (!aux->transfer)
>> +return;
>> +
>> #ifndef CONFIG_MEDIA_CEC_RC
>> /*
>> * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
>> @@ -361,6 +369,10 @@ EXPORT_SYMBOL(drm_dp_cec_set_edid);
>> */
>> void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
>> {
>> +/* No transfer function was set, so not a DP connector */
>> +if (!aux->transfer)
>> +return;
>> +
>> cancel_delayed_work_sync(>cec.unregister_work);
>>
>> mutex_lock(>cec.lock);
>> @@ -404,6 +416,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux
>> *aux,
>> const char *name,
>> struct device *parent)
>> {
>> WARN_ON(aux->cec.adap);
>> +if (WARN_ON(!aux->transfer))
>> +return;
>> aux->cec.name = name;
>> aux->cec.parent = parent;
>> INIT_DELAYED_WORK(>cec.unregister_work,
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH (repost) 1/5] drm_dp_cec: check that aux has a transfer function
On Fri, 2018-08-17 at 16:11 +0200, Hans Verkuil wrote:
> From: Hans Verkuil
>
> If aux->transfer == NULL, then just return without doing
> anything. In that case the function is likely called for
> a non-(e)DP connector.
>
> This never happened for the i915 driver, but the nouveau and amdgpu
> drivers need this check.
Could you give a backtrace from where you're hitting this issue with nouveau and
amdgpu? It doesn't make a whole ton of sense to have connectors registering DP
aux busses if they aren't actually DP, that should probably just be fixed...
>
> The alternative would be to add this check in those drivers before
> every drm_dp_cec call, but it makes sense to check it in the
> drm_dp_cec functions to prevent a kernel oops.
>
> Signed-off-by: Hans Verkuil
> ---
> drivers/gpu/drm/drm_dp_cec.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> index 988513346e9c..1407b13a8d5d 100644
> --- a/drivers/gpu/drm/drm_dp_cec.c
> +++ b/drivers/gpu/drm/drm_dp_cec.c
> @@ -238,6 +238,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
> u8 cec_irq;
> int ret;
>
> + /* No transfer function was set, so not a DP connector */
> + if (!aux->transfer)
> + return;
> +
> mutex_lock(>cec.lock);
> if (!aux->cec.adap)
> goto unlock;
> @@ -293,6 +297,10 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> struct edid *edid)
> unsigned int num_las = 1;
> u8 cap;
>
> + /* No transfer function was set, so not a DP connector */
> + if (!aux->transfer)
> + return;
> +
> #ifndef CONFIG_MEDIA_CEC_RC
> /*
>* CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
> @@ -361,6 +369,10 @@ EXPORT_SYMBOL(drm_dp_cec_set_edid);
> */
> void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
> {
> + /* No transfer function was set, so not a DP connector */
> + if (!aux->transfer)
> + return;
> +
> cancel_delayed_work_sync(>cec.unregister_work);
>
> mutex_lock(>cec.lock);
> @@ -404,6 +416,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> const char *name,
> struct device *parent)
> {
> WARN_ON(aux->cec.adap);
> + if (WARN_ON(!aux->transfer))
> + return;
> aux->cec.name = name;
> aux->cec.parent = parent;
> INIT_DELAYED_WORK(>cec.unregister_work,
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH (repost) 1/5] drm_dp_cec: check that aux has a transfer function
From: Hans Verkuil
If aux->transfer == NULL, then just return without doing
anything. In that case the function is likely called for
a non-(e)DP connector.
This never happened for the i915 driver, but the nouveau and amdgpu
drivers need this check.
The alternative would be to add this check in those drivers before
every drm_dp_cec call, but it makes sense to check it in the
drm_dp_cec functions to prevent a kernel oops.
Signed-off-by: Hans Verkuil
---
drivers/gpu/drm/drm_dp_cec.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
index 988513346e9c..1407b13a8d5d 100644
--- a/drivers/gpu/drm/drm_dp_cec.c
+++ b/drivers/gpu/drm/drm_dp_cec.c
@@ -238,6 +238,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
u8 cec_irq;
int ret;
+ /* No transfer function was set, so not a DP connector */
+ if (!aux->transfer)
+ return;
+
mutex_lock(>cec.lock);
if (!aux->cec.adap)
goto unlock;
@@ -293,6 +297,10 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
struct edid *edid)
unsigned int num_las = 1;
u8 cap;
+ /* No transfer function was set, so not a DP connector */
+ if (!aux->transfer)
+ return;
+
#ifndef CONFIG_MEDIA_CEC_RC
/*
* CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
@@ -361,6 +369,10 @@ EXPORT_SYMBOL(drm_dp_cec_set_edid);
*/
void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
{
+ /* No transfer function was set, so not a DP connector */
+ if (!aux->transfer)
+ return;
+
cancel_delayed_work_sync(>cec.unregister_work);
mutex_lock(>cec.lock);
@@ -404,6 +416,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
const char *name,
struct device *parent)
{
WARN_ON(aux->cec.adap);
+ if (WARN_ON(!aux->transfer))
+ return;
aux->cec.name = name;
aux->cec.parent = parent;
INIT_DELAYED_WORK(>cec.unregister_work,
--
2.18.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
