[PATCH] drm/sti: Remove select of CONFIG_FW_LOADER_USER_HELPER_FALLBACK

[PATCH] drm/sti: Remove select of CONFIG_FW_LOADER_USER_HELPER_FALLBACK

[Takashi Iwai]

The commit [4fdbc678fe4d: drm: sti: add HQVDP plane] added the select
of CONFIG_FW_LOADER_USER_HELPER_FALLBACK by some unwritten reason.
But this config is known to be harmful, and is present only for
compatibility reason for an old exotic system that mandates udev
interaction which isn't supposed to be selected by a driver.
Let's remove it.

Fixes: 4fdbc678fe4d ('drm: sti: add HQVDP plane')
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/sti/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/sti/Kconfig b/drivers/gpu/drm/sti/Kconfig
index fbccc105819b..a18159074b76 100644
--- a/drivers/gpu/drm/sti/Kconfig
+++ b/drivers/gpu/drm/sti/Kconfig
@@ -6,7 +6,6 @@ config DRM_STI
 	select DRM_GEM_CMA_HELPER
 	select DRM_KMS_CMA_HELPER
 	select DRM_PANEL
-	select FW_LOADER_USER_HELPER_FALLBACK
 	help
 	  Choose this option to enable DRM on STM stiH41x chipset
 
-- 
2.6.1

Re: [PATCH] drm/sti: Remove select of CONFIG_FW_LOADER_USER_HELPER_FALLBACK

[Vincent ABRIOU]

Hi Takashi,

Removing FW_LOADER_USER_HELPER_FALLBACK leads to a failure in our HQVDP 
firmware execution.
Indeed, our firmware is not built-in. It is a proprietary firmware 
uploaded into the file system that's why we need the 
USER_HELPER_FALLBACK to be able to load it once file system is available.

BR
Vincent

On 10/19/2015 08:16 AM, Takashi Iwai wrote:
> The commit [4fdbc678fe4d: drm: sti: add HQVDP plane] added the select
> of CONFIG_FW_LOADER_USER_HELPER_FALLBACK by some unwritten reason.
> But this config is known to be harmful, and is present only for
> compatibility reason for an old exotic system that mandates udev
> interaction which isn't supposed to be selected by a driver.
> Let's remove it.
>
> Fixes: 4fdbc678fe4d ('drm: sti: add HQVDP plane')
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   drivers/gpu/drm/sti/Kconfig | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sti/Kconfig b/drivers/gpu/drm/sti/Kconfig
> index fbccc105819b..a18159074b76 100644
> --- a/drivers/gpu/drm/sti/Kconfig
> +++ b/drivers/gpu/drm/sti/Kconfig
> @@ -6,7 +6,6 @@ config DRM_STI
>   	select DRM_GEM_CMA_HELPER
>   	select DRM_KMS_CMA_HELPER
>   	select DRM_PANEL
> -	select FW_LOADER_USER_HELPER_FALLBACK
>   	help
>   	  Choose this option to enable DRM on STM stiH41x chipset
>
>

Re: [PATCH] drm/sti: Remove select of CONFIG_FW_LOADER_USER_HELPER_FALLBACK

[Takashi Iwai]

On Thu, 29 Oct 2015 15:21:35 +0100,
Vincent ABRIOU wrote:
> 
> Hi Takashi,
> 
> Removing FW_LOADER_USER_HELPER_FALLBACK leads to a failure in our HQVDP 
> firmware execution.
> Indeed, our firmware is not built-in. It is a proprietary firmware 
> uploaded into the file system that's why we need the 
> USER_HELPER_FALLBACK to be able to load it once file system is available.

It's not the option a driver can set.  It's a global control option
that is applied *all* drivers.

If the fallback behavior is mandatory by a weird reason, you'd need to
adjust in a different way; e.g. modify the firmware loader code to
provide a new API for the driver to give a fallback (i.e. the another
flip of request_firmware_direct()) and use that API explicitly.

But I don't believe it's the case.  Judging from your comment, it's
your system setup that requires the user-space fallback.  So don't
make the fallback thing mandatory.


thanks,

Takashi

> 
> BR
> Vincent
> 
> On 10/19/2015 08:16 AM, Takashi Iwai wrote:
> > The commit [4fdbc678fe4d: drm: sti: add HQVDP plane] added the select
> > of CONFIG_FW_LOADER_USER_HELPER_FALLBACK by some unwritten reason.
> > But this config is known to be harmful, and is present only for
> > compatibility reason for an old exotic system that mandates udev
> > interaction which isn't supposed to be selected by a driver.
> > Let's remove it.
> >
> > Fixes: 4fdbc678fe4d ('drm: sti: add HQVDP plane')
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   drivers/gpu/drm/sti/Kconfig | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/sti/Kconfig b/drivers/gpu/drm/sti/Kconfig
> > index fbccc105819b..a18159074b76 100644
> > --- a/drivers/gpu/drm/sti/Kconfig
> > +++ b/drivers/gpu/drm/sti/Kconfig
> > @@ -6,7 +6,6 @@ config DRM_STI
> >   	select DRM_GEM_CMA_HELPER
> >   	select DRM_KMS_CMA_HELPER
> >   	select DRM_PANEL
> > -	select FW_LOADER_USER_HELPER_FALLBACK
> >   	help
> >   	  Choose this option to enable DRM on STM stiH41x chipset
> >
> >

Re: [PATCH] drm/sti: Remove select of CONFIG_FW_LOADER_USER_HELPER_FALLBACK

[Emil Velikov]

On 29 October 2015 at 14:21, Vincent ABRIOU <vincent.abriou@st.com> wrote:
> Hi Takashi,
>
> Removing FW_LOADER_USER_HELPER_FALLBACK leads to a failure in our HQVDP
> firmware execution.
> Indeed, our firmware is not built-in. It is a proprietary firmware
> uploaded into the file system that's why we need the
> USER_HELPER_FALLBACK to be able to load it once file system is available.
>
Hmm most other DRM drivers also require firmware. Whist some allow the
firmware to be picked in initrd it's not a strict requirement.
So I'm wondering how come there hasn't been (m)any reports,
considering that neither one sets USER_HELPER_FALLBACK.

Perhaps they also need it, or something in the sti module is done
differently ? Just some food for thought.

Regards,
Emil

Re: [PATCH] drm/sti: Remove select of CONFIG_FW_LOADER_USER_HELPER_FALLBACK

[Takashi Iwai]

On Thu, 29 Oct 2015 15:37:51 +0100,
Emil Velikov wrote:
> 
> On 29 October 2015 at 14:21, Vincent ABRIOU <vincent.abriou@st.com> wrote:
> > Hi Takashi,
> >
> > Removing FW_LOADER_USER_HELPER_FALLBACK leads to a failure in our HQVDP
> > firmware execution.
> > Indeed, our firmware is not built-in. It is a proprietary firmware
> > uploaded into the file system that's why we need the
> > USER_HELPER_FALLBACK to be able to load it once file system is available.
> >
> Hmm most other DRM drivers also require firmware. Whist some allow the
> firmware to be picked in initrd it's not a strict requirement.
> So I'm wondering how come there hasn't been (m)any reports,
> considering that neither one sets USER_HELPER_FALLBACK.
> 
> Perhaps they also need it, or something in the sti module is done
> differently ? Just some food for thought.

It's the option each user decides to set or not, depending on the
deployed system.  Most of PCs don't need them, and actually enabling
this option causes troubles for them.  On other embedded systems, this
might be still needed.  So, it's the system setup issue, and not the
thing a driver needs to care.

Imagine that your driver has "select EXT3_FS" because your system
requires it; without that option, it won't boot, OMG!
Is it the right thing?  Obviously no.  The same logic is applied to
this case, too.


Takashi

Re: [PATCH] drm/sti: Remove select of CONFIG_FW_LOADER_USER_HELPER_FALLBACK

[Vincent ABRIOU]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1972 bytes --]

Hi Takashi,

I made a rework to load the firmware at a different time, when file 
system is available.
I am now able to remove FW_LOADER_USER_HELPER_FALLBACK.
I will submit a new patch for this.

Vincent

On 10/29/2015 03:53 PM, Takashi Iwai wrote:
> On Thu, 29 Oct 2015 15:37:51 +0100,
> Emil Velikov wrote:
>>
>> On 29 October 2015 at 14:21, Vincent ABRIOU <vincent.abriou@st.com> wrote:
>>> Hi Takashi,
>>>
>>> Removing FW_LOADER_USER_HELPER_FALLBACK leads to a failure in our HQVDP
>>> firmware execution.
>>> Indeed, our firmware is not built-in. It is a proprietary firmware
>>> uploaded into the file system that's why we need the
>>> USER_HELPER_FALLBACK to be able to load it once file system is available.
>>>
>> Hmm most other DRM drivers also require firmware. Whist some allow the
>> firmware to be picked in initrd it's not a strict requirement.
>> So I'm wondering how come there hasn't been (m)any reports,
>> considering that neither one sets USER_HELPER_FALLBACK.
>>
>> Perhaps they also need it, or something in the sti module is done
>> differently ? Just some food for thought.
>
> It's the option each user decides to set or not, depending on the
> deployed system.  Most of PCs don't need them, and actually enabling
> this option causes troubles for them.  On other embedded systems, this
> might be still needed.  So, it's the system setup issue, and not the
> thing a driver needs to care.
>
> Imagine that your driver has "select EXT3_FS" because your system
> requires it; without that option, it won't boot, OMG!
> Is it the right thing?  Obviously no.  The same logic is applied to
> this case, too.
>
>
> Takashi
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥